Remove redundant declarations of ERR_load_*_strings() #5150

nmathewson · 2018-01-23T14:51:54Z

In commit 52df25c, the
ERR_load_FOO_strings() functions were moved from their original
location in foo.h into new headers called fooerr.h. But they were
never removed from their original locations. This duplication
causes redundant-declaration warnings on programs that use OpenSSL's
headers with such warnings enabled.

This patch removes redundant declarations only, and adds no new code.

The text was updated successfully, but these errors were encountered:

nmathewson · 2018-01-23T14:52:32Z

(I am hoping I can skip the CLA on this one, since it only removes code.)

richsalz · 2018-01-23T15:10:56Z

Looks like we did some of them, but not all. If you think this is not copyrightable, please amend your commit message to put "CLA: trivial" in the text. I kind of think there's no originality here, the compiler will warn you and tell you what to do, so I am okay with this being trivial.

nmathewson · 2018-01-23T15:27:35Z

Amended the commit message per request; no other changes.

levitte · 2018-01-23T15:28:18Z

include/openssl/pem2.h

@@ -12,7 +12,6 @@ extern "C" {
 #endif

 #ifndef HEADER_PEM_H
-int ERR_load_PEM_strings(void);
 #endif


Please remove the guard as well

Hm. Additionally, should this file begin including pemerr.h?

Nope. On second thought, this file should be removed entirely, and additionally, crypto/err/err_all.c should be changed to include the <openssl/*err.h> headers instead of the main ones...

... and you can remove the inclusion of <openssl/pem2.h> in include/openssl/pem.h...

...

come to think of it, you're right, we have to have this file include <openssl/pemerr.h> for the sake of the occasional poor guy who does include this one directly (should never happen, but it is a public header file, so...).

What I said about crypto/err/err_all.c and include/openssl/pem.h stands, though...

I've tried to take this into account with two new commits just pushed.

levitte · 2018-01-23T15:28:18Z

include/openssl/store.h

@@ -207,7 +207,6 @@ int OSSL_STORE_do_all_loaders(void (*do_function) (const OSSL_STORE_LOADER
 /*
 * Error strings
 */


Please remove this comment too

richsalz · 2018-01-23T16:19:14Z

Sad to say that I do not believe (because of the err_all change) that this is still trivial.

nmathewson · 2018-01-23T18:18:26Z

Do you suppose you could merge the other parts in that case? I hadn't intended to refactor any code beyond the headers originally. If not, I'll try to negotiate the CLA process.

For what it's worth, the err_all change was mechanically generated: I replaced ".h>" with "err.h>", and then reverted the changes that the compiler cared about.

richsalz · 2018-01-23T18:20:28Z

Others may be inclined to view this as no-cla-needed. You can either wait for them to approve, or pull out the all_err changes, or deal with the CLA. So many fine choices to choose from :)

levitte

Apart from this little thing, this looks good to me

levitte · 2018-01-23T18:21:32Z

include/openssl/pem2.h

-#ifndef HEADER_PEM_H
-int ERR_load_PEM_strings(void);
-#endif
-
 #ifdef __cplusplus
 }
 #endif


We don't need the C++ wrapper any more, please remove it

nmathewson · 2018-01-23T18:25:17Z

I've sent some questions to legal@; I'll revisit this PR once I have a CLA in place.

levitte · 2018-01-23T18:28:20Z

Ah, now I notice the discussion... yeah, I'd say we've now walked into CLA land.

levitte

Travis told me...

levitte · 2018-01-23T18:33:44Z

crypto/err/err_all.c

+#include <openssl/pemerr.h>
+#include <openssl/x509err.h>
+#include <openssl/x509v3err.h>
+#include <openssl/conferr.h>


You need to add #include <openssl/pkcs7err.h> here. It was previously included via <openssl/x509.h>, but since that got changed to <openssl/x509err.h>, ...

nmathewson · 2018-01-23T18:46:17Z

Thanks for the fast answers. I've sent in a CLA and made the requested changes.

richsalz · 2018-01-23T18:53:45Z

One last hurdle. Either change the commit email to be your MIT address, or we need a corporate CLA from Torproject. Or remove the err_all changes. Sorry for all the hassle.

nmathewson · 2018-01-23T18:59:47Z

Ok. Technically, I'm an officer of the corporation, but I ought to ask the director first. I'll get back to you when I hear more

nmathewson · 2018-01-23T19:45:18Z

The corporate CLA has been sent.

levitte · 2018-01-23T20:26:45Z

Hmmm, Travis is still unhappy, but that's because util/mkerr.pl needs a change, which I think is for another PR.

Either way, you need to do a make update and include the resulting changes in this PR

nmathewson · 2018-01-23T20:31:53Z

Done. Any better now?

levitte · 2018-01-23T20:34:19Z

Travis is probably happier... I'm not entirely, but that's not your fault, and I'll fix in a separate PR

levitte · 2018-01-23T20:36:05Z

... or well, the following patch will make the result somewhat better. Feel free to apply it and do another make update.

diff --git a/util/mkdef.pl b/util/mkdef.pl
index 98cdae537..d62b5fc08 100755
--- a/util/mkdef.pl
+++ b/util/mkdef.pl
@@ -302,7 +302,7 @@ $crypto.=" include/internal/err.h";
 $crypto.=" include/internal/rand.h";
 foreach my $f ( glob(catfile($config{sourcedir},'include/openssl/*.h')) ) {
     my $fn = "include/openssl/" . lc(basename($f));
-    $crypto .= " $fn" if !defined $skipthese{$fn} && $f !~ m@/[a-z]+err\.h$@;
+    $crypto .= " $fn" if !defined $skipthese{$fn};
 }
 
 my $symhacks="include/openssl/symhacks.h";

nmathewson · 2018-01-23T20:49:20Z

I've tried doing what you said.

richsalz · 2018-02-07T00:38:58Z

it couldn't hurt if you rebase against a fresh master and push.

nmathewson · 2018-02-07T14:06:20Z

Done! The patch from @levitte is now redundant, so I removed it during the rebase, and I did a fresh "make update" run. I should have a CLA on file; I'm not sure why the bot is saying I don't.

mattcaswell · 2018-02-07T14:14:20Z

Close/Reopen to kick the bot

mattcaswell · 2018-02-07T14:14:52Z

There was an error in our CLA records. Fixed now.

levitte · 2018-02-07T18:12:02Z

Argh... the removal of those module specific key words at the end of lines in util/libcrypto.num worry me. I'll have another PR up to fix that. Please hold on...

levitte · 2018-02-07T18:54:35Z

#5275 corrects the issues I'm seeing.

nmathewson · 2018-02-09T13:15:27Z

I see #5275 is merged. Should I rebase again?

levitte · 2018-02-09T13:30:59Z

Yes, please, and a new make update

In commit 52df25c, the ERR_load_FOO_strings() functions were moved from their original location in foo.h into new headers called fooerr.h. But they were never removed from their original locations. This duplication causes redundant-declaration warnings on programs that use OpenSSL's headers with such warnings enabled.

- pem2.h is empty, so pem.h doesn't need to include it. - pem2.h once declared ERR_load_PEM_strings(), so it should now include pemerr.h

nmathewson · 2018-02-09T14:03:38Z

Done, as requested.

levitte

Looks good to me.

Thanks for your patience!

levitte · 2018-02-09T15:39:03Z

@richsalz, your approval still valid, I take it?

richsalz · 2018-02-09T15:44:31Z

yes, still valid.

nmathewson · 2018-02-09T16:13:04Z

Thanks for your patience!

No worries. The first time I tried to get a nontrivial patch into OpenSSL, I had to revise it something like 18-25 times. :)

levitte · 2018-02-09T16:49:44Z

Merged. a5d0d6b to 83739b3

In commit 52df25c, the ERR_load_FOO_strings() functions were moved from their original location in foo.h into new headers called fooerr.h. But they were never removed from their original locations. This duplication causes redundant-declaration warnings on programs that use OpenSSL's headers with such warnings enabled. Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5150)

- pem2.h is empty, so pem.h doesn't need to include it. - pem2.h once declared ERR_load_PEM_strings(), so it should now include pemerr.h Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5150)

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5150)

openssl-machine added the hold: cla required label Jan 23, 2018

richsalz approved these changes Jan 23, 2018

View changes

richsalz self-assigned this Jan 23, 2018

richsalz added the branch: master label Jan 23, 2018

nmathewson force-pushed the redundant_err_load branch from 4361fac to 074f516 Jan 23, 2018

openssl-machine removed the hold: cla required label Jan 23, 2018

levitte requested changes Jan 23, 2018

View changes

include/openssl/pem2.h Outdated

#ifndef HEADER_PEM_H

int ERR_load_PEM_strings(void);

#endif

#ifdef __cplusplus

}

#endif

Copy link

Member

levitte Jan 23, 2018

We don't need the C++ wrapper any more, please remove it

levitte requested changes Jan 23, 2018

View changes

nmathewson force-pushed the redundant_err_load branch from bcc4f71 to 6f7309a Jan 23, 2018

openssl-machine added the hold: cla required label Jan 23, 2018

openssl-machine removed the hold: cla required label Jan 23, 2018

levitte mentioned this pull request Jan 24, 2018

util/mkdef.pl: Trust configdata.pm #5157

Closed

nmathewson force-pushed the redundant_err_load branch from 8b606bc to b246148 Feb 7, 2018

openssl-machine added the hold: cla required label Feb 7, 2018

mattcaswell closed this Feb 7, 2018

mattcaswell reopened this Feb 7, 2018

openssl-machine removed the hold: cla required label Feb 7, 2018

levitte mentioned this pull request Feb 7, 2018

mkerr.pl: When writing internal FOOerr.h, pay attention to disablable modules #5275

Closed

nmathewson added 4 commits Feb 9, 2018

Improve #include structure of pem{,2}.h

bf02ca7

- pem2.h is empty, so pem.h doesn't need to include it. - pem2.h once declared ERR_load_PEM_strings(), so it should now include pemerr.h

In err_all.c, include the *err.h headers.

5c7b5aa

Run "make update"

22fbfd5

nmathewson force-pushed the redundant_err_load branch from b246148 to 22fbfd5 Feb 9, 2018

levitte approved these changes Feb 9, 2018

View changes

levitte closed this Feb 9, 2018

levitte added a commit that referenced this issue Feb 9, 2018

In err_all.c, include the *err.h headers.

41e9afa

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5150)

levitte added a commit that referenced this issue Feb 9, 2018

Run "make update"

83739b3

Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5150)

openssl / openssl Public

Remove redundant declarations of ERR_load_*_strings() #5150

Remove redundant declarations of ERR_load_*_strings() #5150

Conversation

nmathewson commented Jan 23, 2018

nmathewson commented Jan 23, 2018

richsalz commented Jan 23, 2018

nmathewson commented Jan 23, 2018

richsalz commented Jan 23, 2018

nmathewson commented Jan 23, 2018

richsalz commented Jan 23, 2018

levitte left a comment

nmathewson commented Jan 23, 2018

levitte commented Jan 23, 2018

levitte left a comment

nmathewson commented Jan 23, 2018

richsalz commented Jan 23, 2018

nmathewson commented Jan 23, 2018

nmathewson commented Jan 23, 2018

levitte commented Jan 23, 2018

nmathewson commented Jan 23, 2018

levitte commented Jan 23, 2018

levitte commented Jan 23, 2018

nmathewson commented Jan 23, 2018

richsalz commented Feb 7, 2018

nmathewson commented Feb 7, 2018

mattcaswell commented Feb 7, 2018

mattcaswell commented Feb 7, 2018

levitte commented Feb 7, 2018

levitte commented Feb 7, 2018

nmathewson commented Feb 9, 2018

levitte commented Feb 9, 2018

nmathewson commented Feb 9, 2018

levitte left a comment

levitte commented Feb 9, 2018

richsalz commented Feb 9, 2018

nmathewson commented Feb 9, 2018

levitte commented Feb 9, 2018