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

OSSL_CMP_CTX: rename get/set functions for trustedStore #17277

Closed
wants to merge 4 commits into from
Closed

OSSL_CMP_CTX: rename get/set functions for trustedStore #17277

wants to merge 4 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Dec 15, 2021 •
edited

Rename OSSL_CMP_CTX_get0_trustedStore() to OSSL_CMP_CTX_get0_trusted() and
OSSL_CMP_CTX_set0_trustedStore() to OSSL_CMP_CTX_set0_trusted(),
adding name macros for backward compatibility.

This makes the get/set function naming more consistent
(in particular with OSSL_CMP_CTX_{get0,set1}_untrusted()) and
removes much redundancy form the definition of many getters and setters.

Checklist
  • documentation is added or updated
  • tests are added or updated

@DDvO DDvO added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member triaged: refactor The issue/pr requests/implements refactoring and removed approval: otc review pending This pull request needs review by an OTC member labels Dec 15, 2021
@t8m
Copy link
Member

t8m commented Dec 15, 2021 •
edited

This would be 4.0 change only.

@DDvO
Copy link
Contributor Author

DDvO commented Dec 15, 2021 •
edited

This would be 4.0 change only.

Why not 3.1?
The API is 'changed' essentially only by adding two function name aliases,
which is fully backward compatible, and no recompilation needed.

@DDvO
Copy link
Contributor Author

DDvO commented Dec 15, 2021

Ah yes, and I changed a pointless const int parameter type to int.
If this may hinder API compatibility by confusing compilers, I'd revert it.

@t8m
Copy link
Member

t8m commented Dec 15, 2021

Why not 3.1? Because this is an ABI break. You cannot remove functions to replace them with other functions and not break ABI even if you provide compat macros.

@t8m t8m added this to the 4.0.0 milestone Dec 15, 2021
@DDvO DDvO mentioned this pull request Dec 15, 2021
Closed
@DDvO DDvO changed the title OSSL_CMP_CTX: rename get/set fn for trustedStore, simplify cmp_ctx.c OSSL_CMP_CTX: rename get/set functions for trustedStore Dec 15, 2021
@DDvO DDvO added triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly and removed triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly labels Dec 15, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Dec 15, 2021

I've split this PR into the renaming part, since this has not been allowed for 3.1,
and the refactoring part, which is now in #17284.

@DDvO
Copy link
Contributor Author

DDvO commented Dec 21, 2021

Why not 3.1? Because this is an ABI break. You cannot remove functions to replace them with other functions and not break ABI even if you provide compat macros.

How if I do the 'renaming' the other way round -
keep the original names and add two macros for the improved names as aliases?

@t8m
Copy link
Member

t8m commented Dec 21, 2021

How if I do the 'renaming' the other way round -
keep the original names and add two macros for the improved names as aliases?

That would be of course OK for 3.1 in terms of API/ABI compatibility. I am still unsure whether we want to introduce such macro aliases, but probably yes.

@DDvO
OSSL_CMP_CTX: rename get/set function for trustedStore
05fed18 
This makes the naming more consistent, in a backward-compatible way
@DDvO
fixup! OSSL_CMP_CTX: rename get/set function for trustedStore
b113ae0 
This makes the naming more consistent, in a backward-compatible way
@DDvO
Copy link
Contributor Author

DDvO commented Dec 21, 2021

How if I do the 'renaming' the other way round -
keep the original names and add two macros for the improved names as aliases?

That would be of course OK for 3.1 in terms of API/ABI compatibility.
I am still unsure whether we want to introduce such macro aliases, but probably yes.

I've just done so in a fixup.

t8m
t8m requested changes Dec 21, 2021
View reviewed changes
util/other.syms Outdated Show resolved Hide resolved
util/other.syms Outdated Show resolved Hide resolved
doc/man3/OSSL_CMP_CTX_new.pod Outdated Show resolved Hide resolved
@DDvO DDvO removed this from the 4.0.0 milestone Dec 21, 2021
@DDvO DDvO requested a review from t8m December 21, 2021 17:48
@DDvO DDvO added the approval: otc review pending This pull request needs review by an OTC member label Dec 22, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Dec 22, 2021

@t8m, is this okay now?

t8m
t8m requested changes Dec 28, 2021
View reviewed changes
doc/man3/OSSL_CMP_CTX_new.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_CMP_CTX_new.pod Outdated Show resolved Hide resolved
doc/man3/OSSL_CMP_CTX_new.pod Outdated Show resolved Hide resolved
include/openssl/cmp.h.in Outdated Show resolved Hide resolved
@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Dec 28, 2021
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but this PR has failing CI tests. Once the tests pass it will get moved to 'approval: ready to merge' automatically, alternatively please review and set the label manually.

@t8m t8m 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 Dec 29, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Dec 30, 2021

Merged - thanks @t8m

openssl-machine pushed a commit that referenced this pull request Dec 30, 2021
@DDvO
OSSL_CMP_CTX: rename get/set function for trustedStore
6be83cc 
This makes the naming more consistent, in a backward-compatible way

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17277)
@DDvO DDvO closed this Dec 30, 2021
DDvO added a commit to mpeylo/cmpossl that referenced this pull request Aug 5, 2022
@DDvO
OSSL_CMP_CTX: rename get/set function for trustedStore
2deaf0a 
This makes the naming more consistent, in a backward-compatible way

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#17277)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m 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 triaged: refactor The issue/pr requests/implements refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DDvO @t8m @openssl-machine