-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix usage & error reporting in SSL programs #7844
Conversation
Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
All options have reasonable default so the programs don't need arguments to do something useful. It is widely accepted for programs that can work without arguments need not insist on the user passing arguments, see 'ls', 'wc', 'sort', 'more' and any number of POSIX utilities that all work without arguments. It is also the historical behaviour of those programs, and something relied one by at least a few team members. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Every now and then, I see of these programs failing with a super-long usage message that gives no clue as to what went wrong. (Recently it happened with a test case in ssl-opt.sh with a fairly long command line that was entirely correct, except some options were not valid in this config - the test should have been skipped but wasn't due to some other bug. It took me longer to figure out than it should have, and could have if the program had simply reported which param was not recognized.) Also, have an explicit "help" command, separate "help_ciphersuites", and have default usage message that's not multiple screens long. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
The error reporting looks clean now, and identifying the issue is much easier. Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The external CI jobs seem to have some issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for pushing this through!
I've pushed a merge of the target branch to get fresh CI results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hereby assert that the commit I pushed to this branch is a merge of the target branch. This can easily be checked on GitHub.
Description
This is mostly trying to improve on things which have been annoying me regularly. (I've already written the part about reporting what exactly went wrong more that once locally.) The goal is not for everything to be perfect, just better than the status quo.
PR checklist