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

Fix spelling errors in CMS. #3463

Closed
wants to merge 2 commits into from
Closed

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented May 14, 2017

mark CMS_R_UNKNOWN_DIGEST_ALGORITM as compat
mark CMS_R_UNSUPPORTED_RECPIENTINFO_TYPE as compat

Split from #3459

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 14, 2017
@jsoref
Copy link
Contributor Author

jsoref commented May 14, 2017

Note: this uses the same notation I'm proposing in #3460

@richsalz richsalz removed the hold: cla required The contributor needs to submit a license agreement label May 14, 2017
@jsoref jsoref mentioned this pull request May 15, 2017
@dot-asm
Copy link
Contributor

dot-asm commented May 18, 2017

For "bookkeeping" purposes I would insist on making this commit specifically about CMS and not spelling of specific word[s]. This means that ghashv8-armx.pl and d2i_X509.pod should not be part of it.

@@ -491,14 +491,16 @@ int ERR_load_CMS_strings(void);
# define CMS_R_TYPE_NOT_ENVELOPED_DATA 146
# define CMS_R_UNABLE_TO_FINALIZE_CONTEXT 147
# define CMS_R_UNKNOWN_CIPHER 148
# define CMS_R_UNKNOWN_DIGEST_ALGORIHM 149
# define CMS_R_UNKNOWN_DIGEST_ALGORITHM 149
# define CMS_R_UNKNOWN_DIGEST_ALGORITM /*compat*/ CMS_R_UNKNOWN_DIGEST_ALGORITHM
Copy link
Contributor

Choose a reason for hiding this comment

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

CMS_R_* are automatically generated and question is what does mkerr.pl do to compatibility definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any compat things must go before the BEGIN ERROR CODES comment. Otherwise they will be wiped out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just a numerical definition? Or with a comment indicating the correct spelling?

Copy link
Contributor

Choose a reason for hiding this comment

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

There can be no custom code after the BEGIN ERROR comment.

@jsoref
Copy link
Contributor Author

jsoref commented May 18, 2017

@dot-asm: sure, I can split those two out. That's perfectly reasonable.

mark CMS_R_UNKNOWN_DIGEST_ALGORITM as compat
mark CMS_R_UNSUPPORTED_RECPIENTINFO_TYPE as compat
Copy link
Contributor

@dot-asm dot-asm left a comment

Choose a reason for hiding this comment

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

Approved with following reservation. Commits will be squashed to one and commit message will be edited. My suggestion for new message is "Fix spelling errors in CMS. Unfortunately it affects public cms.h header, for which reason misspelled names are preserved for backward compatibility."

@dot-asm
Copy link
Contributor

dot-asm commented May 19, 2017

commit message will be edited.

Just in case, you're not required to do anything in this request, as it can be done upon actual commit to repository.

@dot-asm dot-asm changed the title spelling: algorithm Fix spelling errors in CMS. May 21, 2017
@dot-asm dot-asm added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels May 26, 2017
levitte pushed a commit that referenced this pull request May 27, 2017
Unfortunately it affects error code macros in public cms.h header, for
which reason misspelled names are preserved for backward compatibility.

Reviewed-by: Andy Polyakov <appro@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@openssl.org>
(Merged from #3463)
@dot-asm dot-asm removed the approval: review pending This pull request needs review by a committer label May 27, 2017
@dot-asm
Copy link
Contributor

dot-asm commented May 27, 2017

Merged. Thanks for report.

@dot-asm dot-asm closed this May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants