EVP: Centralise fetching error reporting #12857
Conversation
Instead of sometimes, and sometimes not reporting an error in the caller of EVP_XXX_fetch(), where the error may or may not be very accurate, it's now centralised to the inner EVP fetch functionality. It's made in such a way that it can determine if an error occured because the algorithm in question is not there, or if something else went wrong, and will report EVP_R_UNSUPPORTED_ALGORITHM for the former, and EVP_R_FETCH_FAILED for the latter. This helps our own test/evp_test.c when it tries to figure out why an EVP_PKEY it tried to load failed to do so.
levitte
added
branch: master
Is that fine grainedness actually useful? I was thinking that differentiation between "algorithm not available / unsupported" and "er, something else" was enough. "er, something else" is most likely about memory allocation most of the time... |
|
When producing error messages, I believe that the finer grained the messages the better -- it allows the specific fault to be located even across line number changes. That written, I did go too far. Still, distinguishing "unknown", "not found", "bad argument" would be the minimum. |
|
Okie, so that adds "bad argument". Sure, I can deal with that. |
|
I believe I've addressed all your concerns. |
| */ | ||
| if (name_id != 0 && (meth_id = evp_method_id(name_id, operation_id)) == 0) | ||
| if (name_id != 0 && (meth_id = evp_method_id(name_id, operation_id)) == 0) { |
There was a problem hiding this comment.
The name_id can come from the user which shouldn't be an internal error should it?
There was a problem hiding this comment.
Actually, it is, because all the fetch by number functions are internal. So, the conditions for failure is that we have let the OSSL_OP_ numbers run too high (i.e. past 255), or that there are sooo many algorithms that their numbers (name_id) go past 2^24-1.
There was a problem hiding this comment.
I look forward to the day where name_id breaks the 2^24-1 barrier 😉
paulidale
added
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
|
Merged ec0ce18 EVP: Centralise fetching error reporting |
Instead of sometimes, and sometimes not reporting an error in the caller of EVP_XXX_fetch(), where the error may or may not be very accurate, it's now centralised to the inner EVP fetch functionality. It's made in such a way that it can determine if an error occured because the algorithm in question is not there, or if something else went wrong, and will report EVP_R_UNSUPPORTED_ALGORITHM for the former, and EVP_R_FETCH_FAILED for the latter. This helps our own test/evp_test.c when it tries to figure out why an EVP_PKEY it tried to load failed to do so. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #12857)
Instead of sometimes, and sometimes not reporting an error in the
caller of EVP_XXX_fetch(), where the error may or may not be very
accurate, it's now centralised to the inner EVP fetch functionality.
It's made in such a way that it can determine if an error occured
because the algorithm in question is not there, or if something else
went wrong, and will report EVP_R_UNSUPPORTED_ALGORITHM for the
former, and EVP_R_FETCH_FAILED for the latter.
This helps our own test/evp_test.c when it tries to figure out why an
EVP_PKEY it tried to load failed to do so.