Remove engine.num and make helper functions for engine loading stubs.#29215
Remove engine.num and make helper functions for engine loading stubs.#29215mbroz wants to merge 1 commit intoopenssl:feature/engineremovalfrom
Conversation
github-actions
bot
added
the
severity: fips change
esyr
left a comment
esyr
left a comment
There was a problem hiding this comment.
If this commit is a preparatory for removing dynamic engine loading support altogether, it's fine, otherwise, I don't see the point of leaving those macros and typedefs, even as stubs, as it's clearly only for usage within an engine.
esyr
added
branch: feature
Then I'm for removing them altogether |
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]>
mbroz
force-pushed
the
remove-engine-num
branch
from
6c35f1e to
01c8a3f
Compare
Yes, but the idea is that engine is often part of a project, and failure in build means failure in build for the whole project. This will allow compile "broken" engine that will not work, but allows compilation to finish. |
Okay, seems reasonable. |
Sashan
left a comment
Sashan
left a comment
There was a problem hiding this comment.
Just let me know if you don't like my suggestions and I'll immediately approve those changes. what I'm suggesting are just random thoughts which can land in different PR.
thanks.
| int bind_engine(ENGINE *e, const char *id, const dynamic_fns *fns); \ | ||
| OPENSSL_EXPORT \ | ||
| int bind_engine(ENGINE *e, const char *id, const dynamic_fns *fns) \ | ||
| { \ |
There was a problem hiding this comment.
just a thought. would it make sense to add a ossl_assert(1) here so it aborts when in debug mode?
There was a problem hiding this comment.
All this is available only when OPENSSL_ENGINE_STUBS is defined, otherwise it should fail in compilation. I think assert will not help much here. The compiled engine code would be completely unusable with this header anyway.
| # include <openssl/opensslconf.h> | ||
|
|
||
| /* | ||
| * There is no OPENSSL_NO_NO_ENGINE, Engines are permanently disabled |
There was a problem hiding this comment.
I think this comment needs better wording. I would go for something like:
/*
* Engine support is gone. Definitions here are provided for the source code
* compatibility only. They are meant to keep compilation working for legzcy
* projects which, for whatever reason, can not remove/disable old legacy code.
*
* We deliberately keep OPENSSL_NO_ENGINE macro around as it is supplied
* when build is configured with `no-egine` option. OpenSSL 4.0 keeps `no-engine`
* option around.
*
* Note we have to use compile-time message to warn only if API is really used.
* To avoid complicated macros we kind-of abuse existing OSSL_DEPRACATED macros.
*/
But perhaps this suggestion should be moved to separate PR. What I really want to get rid off is this:
There is no OPENSSL_NO_NO_ENGINE, Engines are permanently disabled
this is not helpful at all.
thanks.
There was a problem hiding this comment.
But it should make @bob-beck keep smiling ;-)
There was a problem hiding this comment.
OpenSSL 4.0 keeps
no-engineoption around.
No, this does not work this way, I remove this option is other PR. It keeps only defines in configure.h, but you cannot use no-engine in config. I am not sure we decided how to do that (only discussed that #define).
There was a problem hiding this comment.
@Sashan as I do not touch this part here, isn't better if I create a new tak to "redefine stub use documentation in header" and move this to there? It also needs to fine-tune approach to Configure options (see #29219 where I change no-engine option).
IOW approve this, and discuss wording above later.
Sashan
left a comment
Sashan
left a comment
There was a problem hiding this comment.
I agree the changes here are good enough to go in. thanks.
Sashan
added
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
t8m
added
triaged: feature
|
Merged to the feature branch. Thank you. |
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from #29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
Fixes openssl/project#1420 Signed-off-by: Milan Broz <[email protected]> Reviewed-by: Eugene Syromiatnikov <[email protected]> Reviewed-by: Saša Nedvědický <[email protected]> (Merged from openssl#29215)
6 participants
Removes no longer used util/engines.num and modifies macros to abort engine load (as engines are no longer supported).
Fixes openssl/project#1420