Skip to content

Tweak option error messages#10774

Closed
richsalz wants to merge 2 commits intoopenssl:masterfrom
richsalz:fix-unknown-message
Closed

Tweak option error messages#10774
richsalz wants to merge 2 commits intoopenssl:masterfrom
richsalz:fix-unknown-message

Conversation

@richsalz
Copy link
Copy Markdown
Contributor

@richsalz richsalz commented Jan 7, 2020

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

Fixes #10773

Should be easy to cherry-pick.

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

Fixes #10773
@mspncp
Copy link
Copy Markdown
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 Applies to OpenSSL_1_1_1-stable branch (EOL) branch: master Applies to master branch labels Jan 7, 2020
@mspncp
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

richsalz commented Jan 7, 2020

fixup commit pushed.

@richsalz richsalz closed this Jan 7, 2020
@richsalz richsalz reopened this Jan 7, 2020
Copy link
Copy Markdown
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
Copy Markdown
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 Applies to OpenSSL_1_1_1-stable branch (EOL) label Jan 8, 2020
@t-j-h
Copy link
Copy Markdown
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
Copy Markdown
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
Better messages for unknown option, unknown cipher, unknown digest.

Fixes #10773

Reviewed-by: Dmitry Belyavskiy <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
Reviewed-by: Matthias St. Pierre <[email protected]>
(Merged from #10774)
@mspncp
Copy link
Copy Markdown
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
danbev pushed a commit to nodejs/node that referenced this pull request Apr 3, 2021
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 4, 2021
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 5, 2021
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

apps/openssl: error messages for unknown command line options are inconsistent

6 participants