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

add compensational definition for OPENSSL_MSTR in macros.h #10236

Closed
wants to merge 3 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Oct 22, 2019

The recent commit f386632 broke include/opensll/cmp_util.h when the trace API is enabled.
This PR adds in that header file a compensational declaration.

Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

It would probably be a better idea to add them in includ/openssl/macros.h and give them a more generic name (i.e the original names)

@DDvO DDvO force-pushed the compensate_for_OPENSSL_MSTR branch from a878cd0 to 71cb60c Compare October 22, 2019 11:38
@DDvO
Copy link
Contributor Author

DDvO commented Oct 22, 2019

all right, done

@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Oct 22, 2019
@DDvO DDvO changed the title add compensational definition for OPENSSL_MSTR in cmp_util.h add compensational definition for OPENSSL_MSTR in macros.h Oct 22, 2019
@mattcaswell
Copy link
Member

These two macros were two of the "newly added but undocumented" macros that got into the 3.0 codebase before we set doc-nits to start complaining about undocumented stuff. See #9095. f386632 co-incidentally fixed that by removing the macros - so this PR introduces it again. doc-nits doesn't complain because they weren't also removed form missingmacros.txt. But we should probably document these macros.

@DDvO DDvO mentioned this pull request Nov 4, 2019
2 tasks
@DDvO
Copy link
Contributor Author

DDvO commented Nov 4, 2019

No problem to add some documentation for these two macros, but where to do this?

I would have put it wherever similar macros are documented, but also (most of) the other macros in macros.h, including OPENSSL_API_LEVEL, OPENSSL_FILE and OPENSSL_FUNC are not (yet) documented either.

@levitte
Copy link
Member

levitte commented Nov 4, 2019

Do these have to be public?

@DDvO
Copy link
Contributor Author

DDvO commented Nov 4, 2019

Currently the two macros are used only in the public header include/openssl/cmp_util.h, namely in the implementation of the OSSL_CMP_LOG_START macro. So, for technical reasons, the macro is already public, but we could #undef it after use.

On the other hand, the CCP string composition trick is a generally useful macro (and can be found online as C programming hint under various names). So we could also export it for the benefit of others.

Which way do you prefer?
If we go for exporting it, in which file should I document it?

@levitte
Copy link
Member

levitte commented Nov 4, 2019

Nah, if we use it publicaly, let's expose if publicaly.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 4, 2019

Fine, and where to document these macros?

@DDvO
Copy link
Contributor Author

DDvO commented Nov 5, 2019

Since I got no response on where to document these two macros,
I've introduced for this purpose doc/man3/OPENSSL_FILE.pod,
where I also documented OPENSSL_FILE, OPENSSL_LINE, and OPENSSL_FUNC.

@levitte
Copy link
Member

levitte commented Nov 5, 2019

Uhmmmm, this PR is suddenly a minor gazillion commits. What happened?

@DDvO DDvO force-pushed the compensate_for_OPENSSL_MSTR branch from c171f41 to b85f0f8 Compare November 6, 2019 06:54
@DDvO
Copy link
Contributor Author

DDvO commented Nov 6, 2019

Hmm, it looks like rebase went somewhat wrong. Fixed.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 7, 2019

Recent Travis CI hang resulted in false positive. This time it worked.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 8, 2019

Can someone please give a second approval on this and merge it soon?
Our CMP contribution chunk 6 depends on this.

@beldmit beldmit 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 Nov 8, 2019
@mattcaswell
Copy link
Member

Can someone please give a second approval on this and merge it soon?

Merge happens (at the earliest) 24 hours after "approval: done" is reached.

@DDvO
Copy link
Contributor Author

DDvO commented Nov 8, 2019

Thanks for the swift (re-)action!

@mspncp mspncp 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 Nov 10, 2019
@mspncp
Copy link
Contributor

mspncp commented Nov 10, 2019

ping: ready to merge

openssl-machine pushed a commit that referenced this pull request Nov 12, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #10236)
openssl-machine pushed a commit that referenced this pull request Nov 12, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #10236)
@mattcaswell
Copy link
Member

I pushed this. Note: I merged the last 2 commits into 1.

@DDvO DDvO deleted the compensate_for_OPENSSL_MSTR branch November 22, 2019 09:31
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 Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants