Initial support for std::error_code #86
Conversation
There was a problem hiding this comment.
Thank you for the invite firstly! I want to help make this project successful 😄
All in all it looks good. Just a few editorial comments.
I noticed the exceptions and now the error_codes for the "algorithms" are outside of that namespace, would you think they could be moved to that namespace?
| #if OPENSSL_VERSION_NUMBER <= 0x10100003L | ||
| std::unique_ptr<BIO, decltype(&BIO_free_all)> certbio(BIO_new_mem_buf(const_cast<char*>(certstr.data()), certstr.size()), BIO_free_all); | ||
| #else | ||
| std::unique_ptr<BIO, decltype(&BIO_free_all)> certbio(BIO_new_mem_buf(certstr.data(), static_cast<int>(certstr.size())), BIO_free_all); | ||
| #endif | ||
| std::unique_ptr<BIO, decltype(&BIO_free_all)> keybio(BIO_new(BIO_s_mem()), BIO_free_all); | ||
| if(!certbio || !keybio) { | ||
| ec = error::rsa_error::create_mem_bio_failed; |
There was a problem hiding this comment.
could be more generic than rsa_error
All the headers return an rsa_error, do any of the other algos use the helpers? I don't see any in the diff
Well since you already touched/wrote like half of the codebase and I never had any real complaints I think thats just fair.
I though about that, but if you have catch blocks moving it means users have to change their code. But thinking about it we have breaking changes for the next release anyway. I"ll look through and change your suggestions later. |
💯 it does not add to the UX either,
I don't actually see any API breaks, it might be worth reviewing to make sure. Very little test code changed.
I am certainly in favor of that! |
|
Maybe we could move all exception types to the error namespace and just typedef them in the jwt namespace ? |
namespace error
{
//rsa_expection...
//ecdsa_expection...
}
using rsa_exception = error::rsa_exception;
using ecdsa_expection = error::ecdsa_expection;🤔 That would be a great alternative! |
|
Looking at the coverage the biggest missed count goes to failed openssl calls. which is not exactly easy to test. Perhaps a few of them could be marks with these so they are not counted |
I already have a unit test in the working to test those. However it's a bit hacky, so I'd like to clean it up a bit more and do it in a different PR. Basically what I do is I override all OpenSSL functions we use and have a bitmask which decides when it should fail. If it should fail I return one of the error codes as documented by Openssl and if not I call the original function. However this method only works on linux, which is fine I guess ? |
Absolutely, if we add windows tests, that can always be |
|
@prince-chrismc Think I can merge this ? |
Initial support for error_code overloads.
@prince-chrismc Wanna take a quick look to check if I maybe missed something ?