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.

Sign up for GitHub

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

Jump to bottom

Tweak option error messages #10774

Closed
wants to merge 2 commits into from
Closed

Tweak option error messages #10774

wants to merge 2 commits into from

Conversation

richsalz
Copy link
Contributor

@richsalz richsalz commented Jan 7, 2020

Better messages for unknown option, unknown cipher, unknown digest.

Fixes #10773

Should be easy to cherry-pick.

@richsalz
Tweak option error messages
b1fc49e 
Better messages for unknown option, unknown cipher, unknown digest.

Fixes #10773
@richsalz richsalz mentioned this pull request Jan 7, 2020
Closed
@mspncp
Copy link
Contributor

mspncp commented Jan 7, 2020

A great impovement, indeed!

     11 cmd: Unknown cipher foobar
      7 cmd: Unknown message digest foobar
     35 cmd: Unknown option -foobar

But don't you think it would make the output even more readable if you would add colons?

     11 cmd: Unknown cipher: foobar
      7 cmd: Unknown message digest: foobar
     35 cmd: Unknown option: -foobar

@mspncp mspncp added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch branch: master Merge to master branch labels Jan 7, 2020
@mspncp
Copy link
Contributor

mspncp commented Jan 7, 2020

Should be easy to cherry-pick.

A cherry-pick to 1.1.1. would be o.k for me. After all, it's an improvement of the error diagnostics.

@richsalz
Copy link
Contributor Author

richsalz commented Jan 7, 2020

fixup commit pushed.

@richsalz richsalz closed this Jan 7, 2020
@richsalz richsalz reopened this Jan 7, 2020
mspncp
mspncp approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

@mspncp mspncp added the approval: review pending This pull request needs review by a committer label Jan 7, 2020
@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 8, 2020
@levitte
Copy link
Member

levitte commented Jan 8, 2020

It's at least arguable of this should go back to 1.1.1. Changed error messages in a stable release is generally not seen as stable. I would rather avoid that.

@t-j-h t-j-h removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Jan 8, 2020
@t-j-h
Copy link
Member

t-j-h commented Jan 8, 2020

I have removed the branch: 1.1.1 label - consider it a "hold" for any back fit.

@mspncp
Copy link
Contributor

mspncp commented Jan 8, 2020

It's at least arguable of this should go back to 1.1.1. Changed error messages in a stable release is generally not seen as stable. I would rather avoid that.

That's ok for me. I was already tempted to ask whether error messages are considered part of the 'API contract', but then I refrained from doing it. Anyway, if there is a non-negligible chance to break existing scripts because they parse the error messages, it's better not to do it.

@paulidale paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 10, 2020
openssl-machine pushed a commit that referenced this pull request Jan 10, 2020
@richsalz @mspncp
Tweak option error messages
e0e68f9 
Better messages for unknown option, unknown cipher, unknown digest.

Fixes #10773

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from #10774)
@mspncp
Copy link
Contributor

mspncp commented Jan 10, 2020

Merged to master as e0e68f9, thanks!

@mspncp mspncp closed this Jan 10, 2020
@richsalz richsalz deleted the fix-unknown-message branch January 10, 2020 23:35
@richardlau richardlau mentioned this pull request Apr 1, 2021
Closed
danbev pushed a commit to nodejs/node that referenced this pull request Apr 3, 2021
@richardlau @danbev
test: fix test-tls-no-sslv3 for OpenSSL 3
0828a96 
OpenSSL 3 has changed the format of the error message for an unknown
option to the CLI. Update the test to allow for the older and newer
message formats.

PR-URL: #38027
Refs: openssl/openssl#10774
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 4, 2021
@richardlau @MylesBorins
test: fix test-tls-no-sslv3 for OpenSSL 3
4b1345e 
OpenSSL 3 has changed the format of the error message for an unknown
option to the CLI. Update the test to allow for the older and newer
message formats.

PR-URL: #38027
Refs: openssl/openssl#10774
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 5, 2021
@richardlau @MylesBorins
test: fix test-tls-no-sslv3 for OpenSSL 3
b02c352 
OpenSSL 3 has changed the format of the error message for an unknown
option to the CLI. Update the test to allow for the older and newer
message formats.

PR-URL: #38027
Refs: openssl/openssl#10774
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mspncp mspncp mspncp approved these changes

@beldmit beldmit beldmit approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

apps/openssl: error messages for unknown command line options are inconsistent
6 participants
@richsalz @mspncp @levitte @t-j-h @beldmit @paulidale