Skip to content

Corrected 'cms' exit status when key or certificate cannot be opened #4997

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
Closed
KonstantinShemyak wants to merge 1 commit into from
Closed

Corrected 'cms' exit status when key or certificate cannot be opened #4997

KonstantinShemyak wants to merge 1 commit into from

Conversation

@KonstantinShemyak
Copy link
Contributor

@KonstantinShemyak KonstantinShemyak commented Dec 28, 2017

Fixes #4996.

Should tests be added? I do not see any testing for nonzero exit codes in tests/cms-examples.pl...

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Dec 28, 2017
@mattcaswell
Copy link
Member

mattcaswell commented Dec 29, 2017

Please either send us a CLA, or if you think the contribution is trivial add "CLA: trivial" to the commit description.

Should tests be added? I do not see any testing for nonzero exit codes in tests/cms-examples.pl...

Tests are always welcome, although historically we don't have much in the way of tests for the command line apps. Probably we should, but so far we don't. Therefore I don't think we would insist on tests (although the question does make me wonder whether we should have a policy on this)

@KonstantinShemyak
Copy link
Contributor Author

KonstantinShemyak commented Dec 29, 2017 •
edited
Loading

Please either send us a CLA [...]

Just done.

Update: got confirmation that the CLA is received and processed. Rebased onto master.

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 2, 2018
@KonstantinShemyak
Copy link
Contributor Author

KonstantinShemyak commented Jan 2, 2018 •
edited
Loading

It looks to me that Travis CI build failure is the failure of the CI itself (packages were not downloaded to the build system) - correct? I'm retriggering the build by closing/opening the pull request.

mattcaswell
mattcaswell approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Should we backport this change? Are 1.1.0 and 1.0.2 similarly affected?

@KonstantinShemyak
Copy link
Contributor Author

KonstantinShemyak commented Jan 3, 2018

Are 1.1.0 and 1.0.2 similarly affected?

Versions tagged with OpenSSL_1_1_0 and OpenSSL_1_0_2 also return 3 like the master version.

@dot-asm dot-asm added the approval: done This pull request has the required number of approvals label Jan 4, 2018
@dot-asm
Copy link
Contributor

dot-asm commented Jan 4, 2018

I'd say it would be appropriate to back-port to 1.1.0 and 1.0.2, but it won't cherry-pick. And it looks like 1.1.0 version won't cherry-pick to 1.0.2... Separate requests then?

@KonstantinShemyak
Copy link
Contributor Author

KonstantinShemyak commented Jan 5, 2018

Do you suggest that I make (seemingly unrelated) GitHub pull requests to OpenSSL_1_1_0-stable and OpenSSL_1_0_2-stable branches?

@mattcaswell
Copy link
Member

mattcaswell commented Jan 5, 2018

Do you suggest that I make (seemingly unrelated) GitHub pull requests to OpenSSL_1_1_0-stable and OpenSSL_1_0_2-stable branches?

Yes please! You can reference this PR in the description.

KonstantinShemyak added a commit to KonstantinShemyak/openssl that referenced this pull request Jan 5, 2018
@KonstantinShemyak
Corrected 'cms' exit status when key or certificate cannot be opened
4faf67f 
A backport of openssl#4997.
Fixes openssl#4996.
@KonstantinShemyak KonstantinShemyak mentioned this pull request Jan 5, 2018
Closed
KonstantinShemyak added a commit to KonstantinShemyak/openssl that referenced this pull request Jan 5, 2018
@KonstantinShemyak
Corrected 'cms' exit status when key or certificate cannot be opened
421054f 
A backport of openssl#4997.
Fixes openssl#4996.
@KonstantinShemyak KonstantinShemyak mentioned this pull request Jan 5, 2018
Closed
levitte pushed a commit that referenced this pull request Jan 6, 2018
@KonstantinShemyak @bernd-edlinger
Corrected 'cms' exit status when key or certificate cannot be opened
f20aa0a 
A backport of #4997.
Fixes #4996.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from #5021)
levitte pushed a commit that referenced this pull request Jan 6, 2018
@KonstantinShemyak
Corrected 'cms' exit status when key or certificate cannot be opened
2157adf 
A backport of #4997.
Fixes #4996.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
(Merged from #5020)
@bernd-edlinger
Copy link
Member

bernd-edlinger commented Jan 6, 2018

Oh, as this was already approved, I will go ahead and merge now.

levitte pushed a commit that referenced this pull request Jan 6, 2018
@KonstantinShemyak @bernd-edlinger
Corrected 'cms' exit status when key or certificate cannot be opened
a032ed0 
Fixes #4996.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from #4997)
@bernd-edlinger
Copy link
Member

bernd-edlinger commented Jan 6, 2018

Merged to all branches. Thanks!

@KonstantinShemyak KonstantinShemyak deleted the cms-ret-2 branch January 6, 2018 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

+1 more reviewer

@dot-asm dot-asm dot-asm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: done This pull request has the required number of approvals

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@KonstantinShemyak @mattcaswell @dot-asm @bernd-edlinger @openssl-machine