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

man EVP_PKEY_CTX_set_params: document params is a list #23986

Closed
wants to merge 1 commit into from

Conversation

tomato42
Copy link
Contributor

Make it clear in the man page that the parameters field in EVP_PKEY_CTX_set_params are a pointer to a list, not to a single struct.

Checklist
  • documentation is added or updated

Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM though it looks obvious to me

@beldmit beldmit added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Mar 27, 2024
@@ -23,6 +23,8 @@ The EVP_PKEY_CTX_get_params() and EVP_PKEY_CTX_set_params() functions allow
transfer of arbitrary key parameters to and from providers.
Not all parameters may be supported by all providers.
See L<OSSL_PROVIDER(3)> for more information on providers.
The B<params> field is a pointer to a list of OSSL_PARAM structures, terminated
Copy link
Member

Choose a reason for hiding this comment

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

Use I<params> and B<OSSL_PARAM> please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, fixed

@t8m t8m added triaged: documentation The issue/pr deals with documentation (errors) branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 and removed approval: review pending This pull request needs review by a committer labels Mar 27, 2024
Copy link
Member

@slontis slontis left a comment

Choose a reason for hiding this comment

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

transfer of 'arbitrary key parameters' gives a good indication that it is a list.

We should probably do this generically because there are a lot of these setter/getters.

In multiple places this is handled as
'See L<OSSL_PARAM(3)> for information about passing parameters'

@tom-cosgrove-arm
Copy link
Contributor

@t8m are you reviewing this? I think this is useful if only for the "terminated with a L<OSSL_PARAM_END(3)> struct" bit

@tomato42
Copy link
Contributor Author

I've proposed this change because it wasn't obvious to my colleague and to me, and had to actually experiment to understand how to use the API. So, if not being explicit about this is stuff in openssl man pages is the policy, I'd say it's a bad policy for documentation aimed at people beginning programming with OpenSSL API.

@slontis
Copy link
Member

slontis commented Apr 2, 2024

If you added what I suggested and then read it, it should be understandable. My point is that there are a lot of set and get params calls. Fixing it in one place its less than ideal.

@tomato42
Copy link
Contributor Author

tomato42 commented Apr 2, 2024

@slontis sorry, given that the man page already has See L<OSSL_PARAM(3)> for more information on parameters. I didn't read your comment as a suggestion to change it to See L<OSSL_PARAM(3)> for information about passing parameters. Are you saying I should make this change too?

@t8m t8m added the approval: review pending This pull request needs review by a committer label Apr 3, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>
@tomato42
Copy link
Contributor Author

tomato42 commented Apr 3, 2024

@slontis Fixed (BTW: I don't think that a 👍 shows up in notifications)

@t8m t8m requested a review from beldmit April 4, 2024 13:28
@beldmit
Copy link
Member

beldmit commented Apr 4, 2024

Still LGTM

@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 Apr 4, 2024
@tomato42
Copy link
Contributor Author

tomato42 commented Apr 5, 2024

the failure doesn't look at all to be related to the changes:

libcrypto.a(libcrypto-lib-cmp_util.o): in function `ossl_cmp_log_parse_metadata':
cmp_util.c:(.text+0x22e): relocation truncated to fit: R_68K_GOT16O against `.LC9'
libcrypto.a(libcrypto-lib-cmp_util.o): in function `ossl_cmp_X509_STORE_add1_certs':
cmp_util.c:(.text+0x6f6): relocation truncated to fit: R_68K_GOT16O against `.LC9'
libcrypto.a(libcrypto-lib-cmp_util.o): in function `ossl_cmp_asn1_octet_string_set1':
cmp_util.c:(.text+0x7f2): relocation truncated to fit: R_68K_GOT16O against `.LC9'
libcrypto.a(libcrypto-lib-cmp_util.o): in function `ossl_cmp_asn1_octet_string_set1_bytes':
cmp_util.c:(.text+0x8ae): relocation truncated to fit: R_68K_GOT16O against `.LC9'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:30816: test/ssl_old_test] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:3680: build_sw] Error 2

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@beldmit beldmit 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 Apr 5, 2024
@t8m
Copy link
Member

t8m commented Apr 10, 2024

Merged to all the active branches. Thank you.

@t8m t8m closed this Apr 10, 2024
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23986)

(cherry picked from commit 9b87c5a)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23986)

(cherry picked from commit 9b87c5a)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23986)

(cherry picked from commit 9b87c5a)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23986)
openssl-machine pushed a commit that referenced this pull request Apr 10, 2024
Signed-off-by: Hubert Kario <hkario@redhat.com>

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23986)

(cherry picked from commit 9b87c5a)
@tomato42 tomato42 deleted the evp-pkey-ctx-set-params branch April 10, 2024 09:54
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 branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants