AW: Cleaning up include file inconsistencies
FYI, I opened pull request #9333 on GitHub today, which demonstrates some of the ideas which I discussed in this thread. https://github.com/openssl/openssl/pull/9333 Looking forward to your feedback. Matthias
Re: Cleaning up include file inconsistencies
I am beginning to wonder whether it is a good idea to make the fact
whether a private header file contains exported symbols as selection
criterion for where the header file needs to go in the source tree:
> > - headers in `include/internal`
>
> For internal things that we want to make available for both libcrypto
> and libssl (i.e. they are exported in libcrypto.so)
>
> > - headers in `crypto/include/internal`
>
> For internal things that we only want available inside libcrypto
> (i.e. they are NOT exported in libcrypto.so)
If you take a look at commit d5e5e2ffafc7 (Move digests to providers)
then you will notice that the `sm3.h` header file was moved:
~/src/openssl$ git show --stat d5e5e2ffafc7 | grep sm3.h
{crypto/include => include}/internal/sm3.h | 7 +-
So it seems like whenever the first symbol of a libcrypto-only private header
file
needs to be exported then we also have to move the entire header file?
This is tedious and also doesn't really make sense. Because now the files sm2.h
and sm3.h happen to be in two different directories:
crypto/include/internal/sm2.h
include/internal/sm3.h
To be precise: that's only in master, because in 1.1.1 they are still in the
same directory.
(That's how I noticed it in the first place, because I compared source trees.)
My suggestion instead would be to store all internal header files which are
shared by
more than one sub-system either in
include/internal/crypto ( #include "crypto/filename.h")
include/internal/ssl ( #include "ssl/filename.h")
depending on where the code is *implemented*, not where it is *used*.
The directory `include/internal` itself should not contain any include files.
Whether an internal libcrypto function can be used in libssl should depend only
on libcrypto.num and nothing else.
Matthias
AW: Cleaning up include file inconsistencies
A good idea just occurred to me. I will rework #9274 and create two new pull requests from it: - PR 1: restructure the internal headers and fix the internal include guards. - PR 2: fix the include guards for the public header files PR 1 could be backported to 1.1.1 which would be advantageous for cherry-picking. That's important IMHO because 1.1.1 is an LTS release. PR 2 would only go to master. What do you think about it? Matthias
AW: Cleaning up include file inconsistencies
> > > That, to me, is much clearer than the "_int" suffix. > > > > This sounds like an excellent idea to me. > > "Someone" should create a PR... I wouldn't mind doing it alongside the other changes in #9274, but I'd prefer my alternative proposal, which I just posted before. That is, if you agree of course. If you insist on doing it as a separate PR, I don't mind. But it does not make sense to start on it before #9274 has been accepted and merged. Matthias
AW: Cleaning up include file inconsistencies
> > Me, I'm wondering if it wouldn't be clearer if we renamed > > crypto/include/internal -> crypto/include/crypto, and thereby did > > this: > > > > #include "crypto/evp.h" > > > > That, to me, is much clearer than the "_int" suffix. > > This sounds like an excellent idea to me. Wouldn't it even be better to move `crypto/include/internal` to `include/internal/crypto` and include it as #include "internal/crypto/evp.h" this would have the advantage that _all_ shared include files can be found in the `include` folder, the public ones inside `include/openssl` and the internal ones in `include/internal`. Also, it would be a very consistent structure and easy to remember. #include "internal/foo.h" /* shared between libcrypto and libssl */ #include "internal/crypto/bar.h" /* shared by libcrypto modules only */ #include "internal/ssl/bar.h"/* shared by libssl modules only */ ... and so on: ... #include "internal/engines/baz.h" /* shared by engine modules */ Matthias
Re: Cleaning up include file inconsistencies
On Sat, 06 Jul 2019 19:03:27 +0200, Dr. Matthias St. Pierre wrote: > > > For things that are private to that sub-system (sorry, "package" doesn't > > sound right) > > Neither does it to me, apologies. I was looking for the right word, but > nothing except > "package" came to my mind. And I was too lazy to search for it in the docs. It's not in the docs. The original term is "library", and that's because SSLeay and early OpenSSL were designed to make it possible to build a separate library from any of those subdirectories. Traces of this is still seen in the error codes. "sub-system" is a term that some of us have started using, mostly on github. I find it catchy enough that I've started using it regularly. > > Me, I'm wondering if it wouldn't be clearer if we renamed > > crypto/include/internal -> crypto/include/crypto, and thereby did > > this: > > > > #include "crypto/evp.h" > > > > That, to me, is much clearer than the "_int" suffix. > > This sounds like an excellent idea to me. "Someone" should create a PR... Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
Re: Cleaning up include file inconsistencies
> > ./crypto/include/internal/store.h > > ./crypto/include/internal/store_int.h > ... > > I have *no* idea why there are two header files. I must have > forgotten about one of them when creating the other... > > They should be merged into one. Ok, I can take care of it. Matthias
Re: Cleaning up include file inconsistencies
> For things that are private to that sub-system (sorry, "package" doesn't > sound right) Neither does it to me, apologies. I was looking for the right word, but nothing except "package" came to my mind. And I was too lazy to search for it in the docs. > Me, I'm wondering if it wouldn't be clearer if we renamed > crypto/include/internal -> crypto/include/crypto, and thereby did > this: > > #include "crypto/evp.h" > > That, to me, is much clearer than the "_int" suffix. This sounds like an excellent idea to me. Matthias
Re: AW: Cleaning up include file inconsistencies
On Sat, 06 Jul 2019 12:20:11 +0200, Dr. Matthias St. Pierre wrote: > > > Having such a finegrained distinction is not the problem, but (at least to > > me) > > it is not entirely clear which include file goes into which directory. > > Note: the high score seems to lie at four different header files for the same > package, > not counting the generated error header file: > > ~/src/openssl$ find -name 'store*.h' > ... > ./crypto/include/internal/store.h > ./crypto/include/internal/store_int.h ... I have *no* idea why there are two header files. I must have forgotten about one of them when creating the other... They should be merged into one. Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
Re: AW: Cleaning up include file inconsistencies
On Sat, 06 Jul 2019 11:50:48 +0200, Dr. Matthias St. Pierre wrote: > > There are more oddities in the organization of the internal header files. > > 1) It appears to me that there are three different levels of internal header > files > > - headers in `include/internal` For internal things that we want to make available for both libcrypto and libssl (i.e. they are exported in libcrypto.so) > - headers in `crypto/include/internal` For internal things that we only want available inside libcrypto (i.e. they are NOT exported in libcrypto.so) > - headers in `crypto/` along with the source files For things that are private to that sub-system (sorry, "package" doesn't sound right) > Having such a finegrained distinction is not the problem, but (at least to me) > it is not entirely clear which include file goes into which directory. > > 2) Some of the header files carry an `_int.h` suffix while others not, > for seemingly no particular reason. > > ~/src/openssl$ find -name '*_int.h' > ./crypto/crmf/crmf_int.h Should have been named crmf_locl.h > ./crypto/include/internal/err_int.h > ./crypto/include/internal/ess_int.h > ./crypto/include/internal/ec_int.h > ./crypto/include/internal/rand_int.h > ./crypto/include/internal/store_int.h > ./crypto/include/internal/asn1_int.h > ./crypto/include/internal/modes_int.h > ./crypto/include/internal/cryptlib_int.h > ./crypto/include/internal/bn_int.h > ./crypto/include/internal/evp_int.h > ./crypto/include/internal/x509_int.h The reason for this is to avoid confusion between headers in crypto/include/internal and headers in include/internal. It's not quite as necessary as all these names seem to suggest, but we do have these as an example: : ; find . -name 'err*.h' | grep /include/internal/ ./include/internal/err.h ./crypto/include/internal/err_int.h > ./crypto/cmp/cmp_int.h Should have been named cmp_locl.h > ./crypto/x509/pcy_int.h Should have been named pcy_locl.h > In particular for the headers the `include/internal` folder this suffix > is superfluous, because these files are (or should be) included via > relative paths. So instead of > > #include > #include "internal/evp_int.h" > > It could just be > > #include > #include "internal/evp.h" In most cases, you're right. I suspect "_int" was habitually added to ensure we know exactly where that header comes from. Me, I'm wondering if it wouldn't be clearer if we renamed crypto/include/internal -> crypto/include/crypto, and thereby did this: #include "crypto/evp.h" That, to me, is much clearer than the "_int" suffix. Cheers, Richard -- Richard Levitte levi...@openssl.org OpenSSL Project http://www.openssl.org/~levitte/
AW: Cleaning up include file inconsistencies
> Having such a finegrained distinction is not the problem, but (at least to me) > it is not entirely clear which include file goes into which directory. Note: the high score seems to lie at four different header files for the same package, not counting the generated error header file: ~/src/openssl$ find -name 'store*.h' ./crypto/store/store_locl.h ./crypto/include/internal/store.h ./crypto/include/internal/store_int.h ./include/openssl/store.h ./include/openssl/storeerr.h Matthias
AW: Cleaning up include file inconsistencies
There are more oddities in the organization of the internal header files. 1) It appears to me that there are three different levels of internal header files - headers in `include/internal` - headers in `crypto/include/internal` - headers in `crypto/` along with the source files Having such a finegrained distinction is not the problem, but (at least to me) it is not entirely clear which include file goes into which directory. 2) Some of the header files carry an `_int.h` suffix while others not, for seemingly no particular reason. ~/src/openssl$ find -name '*_int.h' ./crypto/crmf/crmf_int.h ./crypto/include/internal/err_int.h ./crypto/include/internal/ess_int.h ./crypto/include/internal/ec_int.h ./crypto/include/internal/rand_int.h ./crypto/include/internal/store_int.h ./crypto/include/internal/asn1_int.h ./crypto/include/internal/modes_int.h ./crypto/include/internal/cryptlib_int.h ./crypto/include/internal/bn_int.h ./crypto/include/internal/evp_int.h ./crypto/include/internal/x509_int.h ./crypto/cmp/cmp_int.h ./crypto/x509/pcy_int.h In particular for the headers the `include/internal` folder this suffix is superfluous, because these files are (or should be) included via relative paths. So instead of #include #include "internal/evp_int.h" It could just be #include #include "internal/evp.h" Matthias
Re: Cleaning up include file inconsistencies
Hello, I'd like either _lcl.h or _local.h. _locl.h seems weird to me :) On Sat, Jul 6, 2019 at 10:32 AM Dr. Matthias St. Pierre < matthias.st.pie...@ncp-e.com> wrote: > Hi all, > > pull request #9274 started out as a task to clean up inconsistencies in > the naming > of the include guards. It turned out that there are also some > inconsistencies in the > naming of the include files. > > Please take a look at the general discussion starting at > https://github.com/openssl/openssl/pull/9274#issuecomment-508824668 > between Bernd and me. > > In particular, in > https://github.com/openssl/openssl/pull/9274#issuecomment-508826903 and > following the question was raised whether all local `*_lcl.h` files should > be renamed > to `*_locl.h` for consistency reasons, and the pros and cons discussed. > The latter choice was suggested by a source tree vote: > >~/src/openssl$ find -name '*_lcl.h' | wc -l >19 > ~/src/openssl$ find -name '*_locl.h' | wc -l > 30 > > What's your opinion about renaming of those files? > > Matthias > > -- SY, Dmitry Belyavsky
Cleaning up include file inconsistencies
Hi all, pull request #9274 started out as a task to clean up inconsistencies in the naming of the include guards. It turned out that there are also some inconsistencies in the naming of the include files. Please take a look at the general discussion starting at https://github.com/openssl/openssl/pull/9274#issuecomment-508824668 between Bernd and me. In particular, in https://github.com/openssl/openssl/pull/9274#issuecomment-508826903 and following the question was raised whether all local `*_lcl.h` files should be renamed to `*_locl.h` for consistency reasons, and the pros and cons discussed. The latter choice was suggested by a source tree vote: ~/src/openssl$ find -name '*_lcl.h' | wc -l 19 ~/src/openssl$ find -name '*_locl.h' | wc -l 30 What's your opinion about renaming of those files? Matthias
