Make ERR_str_reasons in err.c consistent again with err.h#17056
Make ERR_str_reasons in err.c consistent again with err.h#17056DDvO wants to merge 2 commits intoopenssl:masterfrom
ERR_str_reasons in err.c consistent again with err.h#17056Conversation
DDvO
added
branch: master
paulidale
added
approval: done
|
I strongly dislike this solution. It doesn't address the actual problem, that flags become part of the index where they shouldn't. That makes it difficult re maintenance, as error codes in the header file become sorted differently than those in the source file, among other things. The real issue is that |
Right - I'll provide a fixup that rectifies this, which will make |
DDvO
force-pushed
the
fix_printing_generic_err_reasons
branch
from
9fe3b77 to
1bc0b73
Compare
Done. After some pitfalls, seems to be fine now. |
|
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. |
|
Actually, all that should be needed is to add the missing strings in Now, I did try printing the two codes, and here's what I got: $ ./util/wrap.pl apps/openssl errstr 1E8C0102
error:1E8C0102:HTTP routines::passed a null parameter
$ ./util/wrap.pl apps/openssl errstr 1E880106
error:1E880106:HTTP routines::reason(524550)And indeed, there is no text string for Why you got no text reason for 1E8C0102, I cannot tell. |
|
Regarding the flags, I'm backing off what I said earlier. Considering that we're using the same reason macros everywhere, it really doesn't matter if we peal off the flags or not, as they should be the same anyway... that was a red herring. |
|
I just submitted #17069, which only adds the missing reason string. To be noted that it's missing all the way back to 1.1.1. The rest of this PR should probably be discussed a little further, because as noted in the modified test, it changes a previous behavior, something I didn't think of initially. That's probably not well suited for 3.0 or 1.1.1. |
levitte
left a comment
levitte
left a comment
There was a problem hiding this comment.
Why are you lowercasing the library names? They are uppercase to match the API prefixes.
(and yes, one might argue that "STORE lib" should be "OSSL_STORE lib")
|
Yes, the comment on the indexing/order was misleading; it works regardless. |
|
Indeed the problem was that some entries were missing - but not only
|
|
I added a commit that exempts flags from the fallback decimal reason code printing, |
|
I also added a tweak to |
DDvO
force-pushed
the
fix_printing_generic_err_reasons
branch
from
4dbecc0 to
58a3525
Compare
DDvO
force-pushed
the
fix_printing_generic_err_reasons
branch
from
58a3525 to
5116e5f
Compare
DDvO
added
approval: otc review pending
and removed
approval: done
Fixes printing generic reason strings, e.g., 'reason(524550)' vs. 'passed an invalid argument'
DDvO
force-pushed
the
fix_printing_generic_err_reasons
branch
from
a05236f to
62590fb
Compare
I cannot reproduce this part of the issue (regarding and after the improvements by this PR: |
|
Thanks for your patience |
t8m
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
Fixes printing generic reason strings, e.g., 'reason(524550)' vs. 'passed an invalid argument' Reviewed-by: Richard Levitte <[email protected]> (Merged from #17056)
Reviewed-by: Richard Levitte <[email protected]> (Merged from #17056)
Fixes printing generic reason strings, e.g., 'reason(524550)' vs. 'passed an invalid argument' Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #17056)
Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #17056)
|
Merged to master and 3.0 - thanks @paulidale , @t8m and @levitte. |
5 participants
For various generic reason codes, their string conversion goes wrong, e.g.:
I found that this is because some entries defined in
include/openssl/err.h{,in}are missing fromERR_str_reasonsincrypto/err/err.cand that their order easily goes wrong there because they need to be sorted by their numerical reason code including any flags such as
ERR_RFLAG_COMMON.After fixing the entries, the above becomes: