Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write CLI diagnostic messages to stderr #2103

Closed

Conversation

lhark
Copy link

@lhark lhark commented Jul 10, 2018

Description

All of the diagnostic and status messages in keepassxc-cli are now output to stderr instead of stdout.
Only meaningful data (data that might be piped to another process) like generated passwords or attributes retrieved with show is sent to stdout.

Motivation and context

Writing the prompt to stderr greatly helps with the use of keepassxc-cli in automated scripts or as a password provider for programs like mutt or borg.
This is done in accordance with POSIX that specifies that diagnostic messages be sent to stderr.
This commit should be a first step in solving the following issues: #831, #1221

How has this been tested?

Before the change, even the prompt goes to stdout:

$ keepassxc-cli show ~/test.kdbx "General/test" > /dev/null

After the change, only the attributes are sent to stdout:

$ build/src/cli/keepassxc-cli show ~/test.kdbx "General/test" > /dev/null
Insert password to unlock /home/lhark/test.kdbx:

To confirm I tried to use the new version to unlock a borg repo and it worked without any problems:

 $ BORG_PASSCOMMAND='build/src/cli/keepassxc-cli show /home/lhark/test.kdbx "General/borg" -a Password' borg list ~/test-repo
Insert password to unlock /home/lhark/test.kdbx:
2018-07-08T17:45:15                  Sun, 2018-07-08 17:45:20 [...]

Types of changes

  • ✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Copy link
Contributor

@TheZ3ro TheZ3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhark
Copy link
Author

lhark commented Jul 12, 2018

Reading the doc you provided, I get the feeling this function is more meant for debugging purposes. But now that you mention it, I see that qCritical() is used elsewhere in the code. At this point it is more a question of style than anything. For example, shoulld the password prompt be considered a debug output ?

I used QTextStream for stderr to stay consistent with the QTextStream used for stdout and stdin.

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Jul 12, 2018

If we want to be meticulous we should use qCritical for errors and qInfo for other diagnostic messages. Anyway I think we can surpass this and approve the PR as is

TheZ3ro
TheZ3ro previously approved these changes Jul 12, 2018
@phoerious
Copy link
Member

What gets actually printed to the screen also depends on your system's Qt debugging settings.

@droidmonkey
Copy link
Member

OK, so we merged the new CLI STDIN/STDOUT/STDERR changes that @phoerious made. This directly conflicts with this entire PR. Can you please look at the new architecture and update your PR to fit?

I think it would be wise to redirect to STDERR only if a flag is set (ie, change the STDOUT to STDERR if --stderr flag is set). That way we can have the best of both worlds. Script friendly and user friendly.

@lhark
Copy link
Author

lhark commented Oct 20, 2018

I can make the changes, however I fail to see how using stdout by default is more user friendly. When an user is using the shell interactively, all messages from stderr are displayed on the terminal. And as I mentioned above only the actual result of a command is supposed to be output to stdout.

@droidmonkey
Copy link
Member

droidmonkey commented Oct 20, 2018

That is fine, I agree with you.

@droidmonkey droidmonkey dismissed TheZ3ro’s stale review October 20, 2018 01:18

invalid due to recent changes

@phoerious
Copy link
Member

phoerious commented Oct 20, 2018

I don't think everything should go to stderr directly if it's not an error or a warning. A command that has output which may be piped must at least have an option for this behaviour (although I think that should rather be suppressing everything else), but just printing all diagnostics to stderr is probably not advisable. Various tools use stderr to detect errors in script execution (apart from checking the return value, which on its own may be misleading) and highlight stderr output differently from stout.

Suppressing diagnostics may also be in implicit option where the process checks if the target is a tty or not. If not, any non-result output is suppressed and password prompts should go to stderr, or course. Errors and warnings always go to stderr.

lhark added 2 commits October 20, 2018 19:26
Writing the prompt to stderr greatly helps with the use of keepassxc-cli
in automated scripts or as a password provider for programs like mutt or
borg.

This is done in accordance with POSIX that specifies that diagnostic
messages be sent to stderr.

This commit should be a first step in solving the following
issues: keepassxreboot#831, keepassxreboot#1221
As mentioned in 1dce5b0, POSIX specifies that all diagnostic and error
messages are to be sent to stderr, only meaningful data shall be output
to stdout (for example: attributes retrieved via keepassxc-cli show).

Stream variables were renamed using the <stream>TextStream convention
for consistency's sake.

It should be noted that stderr is supposed to be unbuffered, some calls
to flush() might not be useful anymore.
@lhark lhark force-pushed the feature/cli-diagnostics-stderr branch from 9354c5b to f6b88cd Compare October 21, 2018 03:09
@lhark
Copy link
Author

lhark commented Oct 21, 2018

I didn't realize that the CLI tests were hidden with the GUI ones ^^
I can't finish fixing them right now, it'll be done in a few days.

@phoerious
Copy link
Member

The CLI tests depend on some GUI routines for clipboard actions, so they need am X server to execute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants