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 type_name member to provided methods and use it #14898

Closed
wants to merge 4 commits into from

Conversation

t8m
Copy link
Member

@t8m t8m commented Apr 16, 2021

Fixes #14701

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

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Apr 16, 2021
@t8m t8m added this to the 3.0.0 beta1 milestone Apr 16, 2021
@richsalz
Copy link
Contributor

can you try #14219 and flip the fetch/getbynid order as shown in 6507522 and see what happens to the tests?

doc/man3/EVP_ASYM_CIPHER_free.pod Outdated Show resolved Hide resolved
test/evp_libctx_test.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Apr 19, 2021

can you try #14219 and flip the fetch/getbynid order as shown in 6507522 and see what happens to the tests?

Note that this will cause some failures, because the main naming standard we came up with for a number of ciphers differs from the "main name" we used in legacy. The new "main names" have a more consistent pattern, so I would suggest that the tests should be adapted accordingly rather then flipping back the algorithm naming scheme.

This is an exercise for another PR, I'd say.

@t8m t8m mentioned this pull request Apr 19, 2021
@t8m
Copy link
Member Author

t8m commented Apr 21, 2021

Ping for review(s)?

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.

Might I suggest this addition?

diff --git a/doc/man7/openssl-core.h.pod b/doc/man7/openssl-core.h.pod
index 8158e3f421..866abd581d 100644
--- a/doc/man7/openssl-core.h.pod
+++ b/doc/man7/openssl-core.h.pod
@@ -60,6 +60,9 @@ identity zero and function pointer NULL.
 The algorithm names and property definitions are defined by the
 providers.
 
+The OpenSSL libraries use the first of the algorithm names as the main
+or canonical name, on a per algorithm implementation basis.
+
 =item B<OSSL_PARAM>
 
 This type is a structure that allows passing arbitrary object data

@levitte
Copy link
Member

levitte commented Apr 21, 2021

Other than that addition, this looks fine

@t8m
Copy link
Member Author

t8m commented Apr 21, 2021

Fixup adding the suggested paragraph pushed. @levitte PTAL

@levitte levitte 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 Apr 21, 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.

@levitte
Copy link
Member

levitte commented Apr 22, 2021

One of the CI jobs had failed to even start, so I decided to re-run them all. Looks much better now.

openssl-machine pushed a commit that referenced this pull request Apr 26, 2021
Fixes #14701

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #14898)
@t8m
Copy link
Member Author

t8m commented Apr 26, 2021

Merged to master. Thank you for the review.

@t8m t8m closed this Apr 26, 2021
@t8m t8m deleted the evp-type-name-first branch May 11, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing EVP_PKEY_type_name() - no way how to get a key type name for new algorithms
4 participants