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
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: 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
