openssl / openssl

TLS/SSL and crypto library
https://www.openssl.org
Apache License 2.0
25.47k stars 10.06k forks source link

Overreliance on e_os.h #4174

Open dot-asm opened 7 years ago

dot-asm commented 7 years ago

e_os.h is private and quite "heavy-weight" header. It's meant to be used in "exceptional" situations to resolve real system dependencies. And it being private arguably disqualifies it for use in apps/*. In sense that it's more than appropriate if apps* set an example of adhering to exclusively public headers. In other words, an overhaul is due...

levitte commented 7 years ago

I agree.

levitte commented 7 years ago

Especially, I agree that it shouldn't be used in apps/*

levitte commented 7 years ago

I also find it questionable that we send the same C flags to the build of apps/* as for the library.

paulidale commented 7 years ago

apps looks like a bit of a nightmare in this regard. Lots broke when I tried removing e_os.h from apps.h.

paulidale commented 7 years ago

Might it make sense to split the functionality in e_os.h into separate include files (in an include/<_something_> directory) and include them piecemeal? Where something is neither internal nor openssl. There is already a define that can be used pre-inclusion of e_os.h to indicate networking is required. This would be the first natural split.

Splitting would provide a better indication as to what is being asked for and reduce the unnecessary weight. Many files only want one thing from e_os.h. e.g. many just wanted OSSL_NELEM and quite a few only require strcasecmp for Windows.

paulidale commented 7 years ago

I also find it questionable that we send the same C flags to the build of apps/* as for the library.

I agree.

richsalz commented 7 years ago

Yes, I think splitting is a good idea. openssl/port or something.

If we're going to say apps should not include internals, that's fine. But we've never really enforced that before.

levitte commented 7 years ago

I don't think that it's a good idea to move the e_os.h stuff to a public header (which is what @richsalz seems to suggest). Some of it is really application stuff, so could be moved to apps/apps.h, while other parts could be moved to become internal headers to be used by the stuff in crypto/ and in ssl/.

Also, thinking back to earlier team discussions, we might want to consider an internal headers directory for architecture dependent stuff... include/arch, maybe?

richsalz commented 7 years ago

The apps run everywhere that openssl builds. If the apps cannot use the openssl headers, etc., that we have created so that our code looks and feels portable, then large parts of some internal files will have to be replicated into the apps directory. I think that is moving backwards. We never said apps is the model for how to build any portable openssl application. We have only said it's the best example for how to use the OpenSSL API's. The fact that it uses internal header files does not change that assertion.

dot-asm commented 7 years ago

If we're going to say apps should not include internals, that's fine. But we've never really enforced that before.

But we haven't claimed that it has to be that way either. I mean there are things the way they are, but it's not necessarily the way they should or ought to be. If given a choice or opportunity to do something about it, which one would one choose? Make things worse or better?

dot-asm commented 7 years ago

parts of some internal files will have to be replicated into the apps directory.

Trouble is that it might only look that way. I mean same kind of things might be appropriate in crypto/* and apps/* contexts, but for completely different reasons. It's easiest to explain with an example. On Windows we have a number of defines that add underscore to some POSIX calls, e.g. stat is re-defined as _stat. In crypto/* context it is done in order to ensure that object binaries can be used in either context, shared or static. it is and should be up to application to choose how to link object files. While in apps/* context on the other hand, it's about shutting up compiler warnings, nothing else. So should header that redefines stat as _stat be shared? I'd argue no...

richsalz commented 7 years ago

I'm not opposed to making apps be more like an external program. It's really only worth doing if someone with good windows experience and knowledge wants to take it in. Yes, looking at you... :)

mattcaswell commented 6 years ago

Moving this to post-1.1.1 milestone.

richsalz commented 6 years ago

See also https://github.com/openssl/openssl/pull/5438 discussion.

nhorman commented 3 months ago

Marking as inactive, to be closed at the end of 3.4 dev barring further input