-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Rename the field 'provctx and data' to 'algctx' inside some EVP objects. #15275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data
is too nondescript in my view, could just as well be xyzzy
. I understand wanting to avoid the confusion with the provider context, though.
So here's another idea.
Side note: I like what the checksums github action does. Nice work @t8m! |
…ontaining pointers to provider size algorithm contexts. Fixes openssl#14284 The gettable_ctx_params methods were confusingly passing a 'provctx' and a provider context which are completely different objects. Some objects such as EVP_KDF used 'data' while others such as EVP_MD used 'provctx'. For libcrypto this 'ctx' is an opaque ptr returned when a providers algorithm implementation creates an internal context using a new_ctx() method. Hence the new name 'algctx'.
6a72770
to
9de3780
Compare
Changed all of them to use algctx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this, I'm happy, and especially that you fixed the MAC code accordingly!
/* | ||
* Opaque ctx returned from a providers digest algorithm implementation | ||
* OSSL_FUNC_digest_newctx() | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems a bit bloated at this point. How about /* Algorithm-specific ctx */
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the longer comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why only here and not everywhere where we have the algctx field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to make them all more verbose noted.
I find it amazing, though, that extended verbosity is chosen for this item specifically. I kinda understand it considering that there seems to be a lot of confusion... but in a year, will we still feel the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I'm not going to block on terms of verbosity, just wanted to express my surprise over this choice, this time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be trying to be kind to others trying to understand what it is.
/* | ||
* Opaque ctx returned from a providers cipher algorithm implementation | ||
* OSSL_FUNC_cipher_newctx() | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems a bit bloated at this point. How about /* Algorithm-specific ctx */
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup!
This pull request is ready to merge |
…jects containing pointers to provider size algorithm contexts.
Changed the comments.. |
…jects containing pointers to provider size algorithm contexts.
Merged to master. |
…ontaining pointers to provider size algorithm contexts. Fixes #14284 The gettable_ctx_params methods were confusingly passing a 'provctx' and a provider context which are completely different objects. Some objects such as EVP_KDF used 'data' while others such as EVP_MD used 'provctx'. For libcrypto this 'ctx' is an opaque ptr returned when a providers algorithm implementation creates an internal context using a new_ctx() method. Hence the new name 'algctx'. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from #15275)
Thanks Pauli.. |
…ontaining pointers to provider size algorithm contexts. Fixes openssl#14284 The gettable_ctx_params methods were confusingly passing a 'provctx' and a provider context which are completely different objects. Some objects such as EVP_KDF used 'data' while others such as EVP_MD used 'provctx'. For libcrypto this 'ctx' is an opaque ptr returned when a providers algorithm implementation creates an internal context using a new_ctx() method. Hence the new name 'algctx'. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> (Merged from openssl#15275)
Fixes #14284
This was a bit confusing since some objects need an actual provider
context to be passed (e.g. asym_cipher_gettable_ctx_params)
This 'data' field is an opaque ptr returned from new_ctx() operations. The
ptr points to data created inside a providers implementation of an
algorithm. This ptr is then passed to the provider algorithm for all
subsequent operations (such as init(), update(), and free()).
Checklist