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

Add script to convert XXXerr() to XXXerror #9441

Closed
wants to merge 4 commits into from

Conversation

@richsalz
Copy link
Contributor

richsalz commented Jul 23, 2019

As discussed in the comment thread at #9391 (comment), this adds new XXXerror() defines that gets rid of the now-unused function name and deprecates the XXXerr() defines.

This is FYI to give a "taste" of what the conversion would be like. In numeric order, the ones remaining are shown below.

Does the OMC want to do this? If so, I'll continue with the conversion.

14 PROPerr(
16 OBJerr(
18 ASYNCerr(
34 PROVerr(
36 UIerr(
41 OCSPerr(
47 FUNCerr(
49 CONFerr(
50 CRYPTOerr(
50 DSAerr(
51 RANDerr(
57 CTerr(
58 CRMFerr(
60 DHerr(
63 OSSL_STOREerr(
63 PKCS12err(
64 KDFerr(
73 BNerr(
73 SM2err(
77 TSerr(
86 ENGINEerr(
86 PEMerr(
99 DSOerr(
108 BIOerr(
118 PKCS7err(
151 X509err(
184 CMSerr(
185 RSAerr(
195 X509V3err(
283 EVPerr(
295 ASN1err(
323 SSLerr(
470 ECerr(
@t8m

This comment has been minimized.

Copy link
Member

t8m commented Jul 23, 2019

Should there be underscore between MODULE and error to better align with other identifiers? Like BUF_error.

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Jul 23, 2019

I think this is much better. I do have a worry about the pain it will cause us when backporting fixes from master to 1.1.1. One option would be to do this - but delay it until shortly before we release 3.0. Or alternatively maybe it won't be so bad and we should just get on with it. I'm not sure.

@mattcaswell

This comment has been minimized.

Copy link
Member

mattcaswell commented Jul 23, 2019

Travis failure is not relevant.

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Jul 23, 2019

I'm inclined to agree with @t8m that FOO_error is better than FOOerr; thoughts?

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Jul 23, 2019

I'll wait a few days and see what the other committers and OMC think before I do more work on this.
(And if it moves forward, I expect to do the complete solution, including an editing script)

@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Jul 23, 2019

Added commit 08a8821 which includes the editing script and commit 14fb890 which shows what things look like using CRPTO_error style.

@beldmit

This comment has been minimized.

Copy link
Member

beldmit commented Jul 23, 2019

I agree with Tomas about undescore and Matt about delay.

@richsalz richsalz force-pushed the richsalz:err-to-error branch 2 times, most recently from 3f675e2 to ea50802 Jul 24, 2019
@richsalz richsalz force-pushed the richsalz:err-to-error branch 3 times, most recently from 3c6781d to 989ae49 Jul 24, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Jul 30, 2019

Since #9452 is likely to go in, I have removed variations and made this based purely on that.

@richsalz richsalz force-pushed the richsalz:err-to-error branch from c99abaf to 9e767b3 Jul 31, 2019
@richsalz richsalz force-pushed the richsalz:err-to-error branch from 9e767b3 to 3567512 Jul 31, 2019
include/openssl/err.h Outdated Show resolved Hide resolved
@richsalz richsalz force-pushed the richsalz:err-to-error branch 4 times, most recently from 32daed9 to 0531a69 Jul 31, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Aug 2, 2019

Taking this out of WIP. It is reasonable to do the conversion (very) late in the development cycle, so I made it easier to do so. This PR now adds the conversion script, and it shows how the old XXXerr defines will get deprecated, by a hack in err.h that should be removed then the script is run.

@richsalz richsalz changed the title WIP: Convert XXXerr() to XXXerror Add script to convert XXXerr() to XXXerror Aug 2, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Aug 2, 2019

I changed the title, removed the WIP.

@richsalz richsalz force-pushed the richsalz:err-to-error branch 3 times, most recently from 517be83 to a5dde70 Aug 2, 2019
@richsalz richsalz force-pushed the richsalz:err-to-error branch from fde1a8d to 4544903 Aug 9, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Aug 9, 2019

I'd appreciate feedback on the comments in the script, starting at around line 14 of https://github.com/openssl/openssl/pull/9441/files#diff-101f5c3e883943d0b61879f71fa9d71fR14

@richsalz richsalz force-pushed the richsalz:err-to-error branch from 4544903 to 2afc9b1 Aug 12, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Aug 12, 2019

I added a script to merge XXXerr() calls that are broken across two lines. It think it's feasible to merge this PR now and then, when the project decides to do the conversion, just run the scripts and review their output.

@richsalz richsalz force-pushed the richsalz:err-to-error branch 3 times, most recently from d0bb193 to 5a3478b Aug 13, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Aug 16, 2019

I pushed another commit to reformat some code. When I do that, I can run the automated scripts and build without errors:

; config no-deprecated
; perl -pi -e 's/1 \|\|//;'  include/openssl/err.h 
; git grep -l '[A-Z0-9]err([^)]*$' | xargs perl -pi util/merge-err-lines
; git ls-files | grep '\.c$' | xargs perl -pi util/err-to-raise
; make -sj10
; make test

I think this PR, which does not do the big conversion should be reviewed and merged. And then, when the project team is ready, they can run the scripts to do the conversion and merge it.

# define TSerr(f, r) ERR_raise_data(ERR_LIB_TS, (r), NULL)
# define UIerr(f, r) ERR_raise_data(ERR_LIB_UI, (r), NULL)
# define X509V3err(f, r) ERR_raise_data(ERR_LIB_X509V3, (r), NULL)
# define X509err(f, r) ERR_raise_data(ERR_LIB_X509, (r), NULL)

This comment has been minimized.

Copy link
@levitte

levitte Aug 17, 2019

Member

It seems a little unnecessary to make them into ERR_raise_data...

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 17, 2019

Author Contributor

Well, it's hidden behind a macro. If I leave it as ERR_raise that will then get turned into ERR_raise_data so I figured it was more useful to developers to not have to chase down multiple define's. But if you want it changed, okay.

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 17, 2019

Author Contributor

(By which I mean, be expilcit and say "please change it" :)

This comment has been minimized.

Copy link
@levitte

levitte Aug 18, 2019

Member

(I only made a comment, not a change request. I'll try to remember to be explicit about that)

This comment has been minimized.

Copy link
@richsalz

richsalz Aug 18, 2019

Author Contributor

It's not a big deal; I think we sometimes forget that discussion is one of the most important things about a PR. :)

@richsalz richsalz force-pushed the richsalz:err-to-error branch from 5a3478b to 8e2213f Aug 19, 2019
@richsalz richsalz force-pushed the richsalz:err-to-error branch from 8e2213f to 810661e Sep 9, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 9, 2019

Related to #9333 (comment) this PR can be merged new, safely, and then the steps in #9441 (comment) can be done just before one of the pre-official releases.

richsalz added 4 commits Jul 24, 2019
Actually, for transition, they're not really deprecated.  Remove the
"1 ||" from the ifdef line (in include/openssl/err.h) when ready to
do this in production/"for real"
It either makes the flow of control simpler and more obvious, or it is
just a "cleanup" so that the editing scripts will find and fixup things.
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 19, 2019

This is the infrastructure, and includes steps to do the work, for converting the old XXXerr calls. It's been a month. Ping for review.

@richsalz richsalz force-pushed the richsalz:err-to-error branch from 810661e to 2e85061 Sep 19, 2019
if (ret == -2)
EVPerr(EVP_F_EVP_CIPHER_PARAM_TO_ASN1, ASN1_R_UNSUPPORTED_CIPHER);
else if (ret <= 0)
EVPerr(EVP_F_EVP_CIPHER_PARAM_TO_ASN1, EVP_R_CIPHER_PARAMETER_ERROR);

This comment has been minimized.

Copy link
@beldmit

beldmit Sep 19, 2019

Member

Sorry for being out of context, but is it OK that here we leave EVPerr(), and in https://github.com/openssl/openssl/pull/9441/files#diff-9f773e58c4cc40339f694534e55f6cb3 we replace EVPerr with ERR_raise()?

This comment has been minimized.

Copy link
@levitte

levitte Sep 19, 2019

Member

I think these are changes are made to make life easier with util/merge-err-lines, as that only handles this kind of split:

    BLARGerr(BLARG_F_FOO,
             BLARG_R_BAR);

This comment has been minimized.

Copy link
@beldmit

beldmit Sep 19, 2019

Member

Got it.

if (ret == -2)
EVPerr(EVP_F_EVP_CIPHER_ASN1_TO_PARAM, EVP_R_UNSUPPORTED_CIPHER);
else if (ret <= 0)
EVPerr(EVP_F_EVP_CIPHER_ASN1_TO_PARAM, EVP_R_CIPHER_PARAMETER_ERROR);

This comment has been minimized.

Copy link
@beldmit

beldmit Sep 19, 2019

Member

Similar question.

This comment has been minimized.

Copy link
@levitte

levitte Sep 19, 2019

Member

Same answer

Copy link
Member

levitte left a comment

I just made a test run, these scripts appear to do what they promise

levitte pushed a commit that referenced this pull request Sep 19, 2019
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9441)
levitte pushed a commit that referenced this pull request Sep 19, 2019
Actually, for transition, they're not really deprecated.  Remove the
"1 ||" from the ifdef line (in include/openssl/err.h) when ready to
do this in production/"for real"

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9441)
levitte pushed a commit that referenced this pull request Sep 19, 2019
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9441)
levitte pushed a commit that referenced this pull request Sep 19, 2019
It either makes the flow of control simpler and more obvious, or it is
just a "cleanup" so that the editing scripts will find and fixup things.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #9441)
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 19, 2019

Merged.

0f17ac2 Add script convert XXerr to ERR_raise
f6aca23 Deprecate XXXerr() macros
8c0e768 Add merge-err-lines script
51ba9eb Avoid ?: construct in XXXerr calls

@levitte levitte closed this Sep 19, 2019
@richsalz

This comment has been minimized.

Copy link
Contributor Author

richsalz commented Sep 19, 2019

THANK YOU!

@richsalz richsalz deleted the richsalz:err-to-error branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.