Make --enable-async-push depend on --enable-plugins
PATCH 1/1 Make --enable-async-push depend on --enable-plugins
Async push functionality makes sense only with deferred authentication, which requires plugins.
Trac #783
--- configure.ac | 4 ++++ 1 file changed, 4 insertions(+)
configure.ac +4 -0
@@ -1204,6 +1204,10 @@ if test "${enable_plugin_auth_pam}" = "yes"; then fi if test "${enable_async_push}" = "yes"; then + if test "${enable_plugins}" = "no"; then + AC_MSG_ERROR([--enable_async_push requires --enable-plugins]) + fi + AC_CHECK_HEADERS( [sys/inotify.h], AC_DEFINE([ENABLE_ASYNC_PUSH], [1], [Enable async push]),
Signature · 1 line
1.9.1
Quoted 16 lines · Lev Stipakov
Async push functionality makes sense only with deferred authentication, which requires plugins.
Trac #783
Signed-off-by Lev Stipakov <[email protected]...>--- configure.ac | 4 ++++ 1 file changed, 4 insertions(+)configure.ac +0
-0
configure.ac if test "${enable_plugin_auth_pam}" = "yes"; then · line 1213
Quoted 10 lines
configure.ac +4
-0@@ -1204,6 +1204,10 @@ if test "${enable_plugin_auth_pam}" = "yes"; then fi if test "${enable_async_push}" = "yes"; then + if test "${enable_plugins}" = "no"; then + AC_MSG_ERROR([--enable_async_push requires --enable-plugins]) + fi + AC_CHECK_HEADERS( [sys/inotify.h], AC_DEFINE([ENABLE_ASYNC_PUSH], [1], [Enable async push]),
Coupling --enable-async-push with --enable-plugins makes a lot of sense.
By the way, there is a typo in the errror message though
(--enable_async_push should be --enable-async-push).
But I am wondering if it would be better to just do this in syshead.h instead:
#if !defined(ENABLE_PLUGIN) && defined(ENABLE_ASYNC_PUSH) #undef ENABLE_ASYNC_PUSH #endif
Or another more verbose patch would be to have all #ifdef ENABLE_ASYNC_PUSH updated to also check for defined(ENABLE_PLUGIN), but I think that's overdoing it a bit.
My point is that there's no real point of screaming about missing --enable-plugins if you have added --enable-auth-push. If you build without plugins support, --plugin in configurations won't work regardless. I think it is more likely that those adding --disable-plugins (it is enabled by default) believe --enable-async-push does something completely different. Rather have in the help line of --enable-async-push that it depends on the plug-in feature.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
13.12.2016, 02:15, David Sommerseth kirjoitti:
Quoted 3 lines
Coupling --enable-async-push with --enable-plugins makes a lot of sense.
By the way, there is a typo in the errror message though
(--enable_async_push should be --enable-async-push).
Yes :(
I wonder if actual committer could fix it or should I send V2.
Quoted 4 lines
regardless. I think it is more likely that those adding --disable-plugins (it is enabled by default) believe --enable-async-push does something completely different. Rather have in the help line of --enable-async-push that it depends on the plug-in feature.
I do not have a strong opinion about that, but IMO having both --enable-async-push (1) and --disable-plugins (2) is a configuration error, since feature (1) depends on feature (2) and warn about it during build phase is more clear rather than silently disable feature (1).
I could send V2 with fixed error message and clarified async-push help line, or alternatively V2 with clarified help line and fix in syshead.h you've suggested :)
-Lev
Hi,
Quoted 6 lines · David Sommerseth
But I am wondering if it would be better to just do this in
syshead.hinstead:#if !defined(ENABLE_PLUGIN) && defined(ENABLE_ASYNC_PUSH) #undef ENABLE_ASYNC_PUSH #endif
We discussed this on IRC yesterday, and I like Lev's approach more.
Your approach will "silently not do what the user requested" - the user configured "--enable-async-push", and the resulting binary will not have that functionality because we turned it off again.
So erroring out in configure if incompatible build options are requested sounds like a logical way to tell the caller "this is not what you want".
[..]
Quoted 2 lines
My point is that there's no real point of screaming about missing --enable-plugins if you have added --enable-auth-push.
Someone added --enable-async-push to the configure command line, and they expect this to have a different effect than "not putting it there".
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Hi,
Quoted 6 lines · Gert Doering
Quoted 2 lines · David Sommerseth
My point is that there's no real point of screaming about missing --enable-plugins if you have added --enable-auth-push.
Someone added --enable-async-push to the configure command line, and they expect this to have a different effect than "not putting it there".
Exactly this. This combination makes no sense, so it's likely a mistake. The best thing to do is to make that clear to the user. (Especially in a configure script, which is somewhat advanced usage. Assuming we know what the user wants is probably a mistake.)
-Steffan
Quoted 10 lines · Gert Doering
Hi,
Quoted 6 lines · David Sommerseth
But I am wondering if it would be better to just do this in
syshead.hinstead:#if !defined(ENABLE_PLUGIN) && defined(ENABLE_ASYNC_PUSH) #undef ENABLE_ASYNC_PUSH #endif
Quoted 8 lines
We discussed this on IRC yesterday, and I like Lev's approach more.
Your approach will "silently not do what the user requested" - the user configured "--enable-async-push", and the resulting binary will not have that functionality because we turned it off again.
So erroring out in configure if incompatible build options are requested sounds like a logical way to tell the caller "this is not what you want".
autotools and ./configure's task is primarily to *probe the system*, not to safe-guard the user against invalid feature combinations. It should ensure your system have what is needed to do a good build.
Which is why I don't think such a check belongs in configure.ac. Because --enable-async-push should be ignored if the user have disabled support for plugins, becuase async push is a sub-feature of plugins.
If async-push is enabled or disabled have nothing to do with if the system have all it needs to do an openvpn build or not. It is purely an openvpn internal switch.
Quoted 5 lines
Quoted 2 lines
My point is that there's no real point of screaming about missing --enable-plugins if you have added --enable-auth-push.
Someone added --enable-async-push to the configure command line, and they expect this to have a different effect than "not putting it there".
If someone added --disable-plugins, they don't need --enable-async-push.
So no reason to scream about it. That it fails to build with that
combo is actually more or less something which slipped through the ACK of the initial patch (mostly because very few disables plugins at all).
Regardless, I won't NAK this pactch as it will resolve an issue. But it also won't get an ACK from me, because I do think this is an abuse of ./configure.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
PATCH v2 1/1 Disable async_push if plugins are disabled
Async push is a sub-feature of plugins.
Trac #783
--- src/openvpn/syshead.h | 7 +++++++ 1 file changed, 7 insertions(+)
src/openvpn/syshead.h +7 -0
@@ -702,4 +702,11 @@ socket_defined (const socket_descriptor_t sd)#define ENABLE_MEMSTATS #endif +/* +* Disable async-push if plugins are disabled +*/ +#if !defined(ENABLE_PLUGIN ) && defined(ENABLE_ASYNC_PUSH ) +#undef ENABLE_ASYNC_PUSH +#endif +#endif
Signature · 1 line
1.9.1
Re: [Openvpn-devel] [PATCH v2] Disable async_push if plugins are disabled
Hi,
src/openvpn/syshead.h socket_defined (const socket_descriptor_t sd) · line 711
Quoted 7 lines · Lev Stipakov
src/openvpn/syshead.h +7
-0+/* +* Disable async-push if plugins are disabled +*/ +#if !defined(ENABLE_PLUGIN ) && defined(ENABLE_ASYNC_PUSH ) +#undef ENABLE_ASYNC_PUSH +#endif +
That one gets a NAK from me, for the reasons given.
As long as we *have* --enable/--disable options in configure, they should be validated *there* and not silently ignored.
David's argument about "configure is for finding system dependent things and compiler settings" has some merits, but then we should get rid of all compile-time --enable/--disable settings.
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...Re: [Openvpn-devel] [PATCH v2] Disable async_push if plugins are disabled
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 7 lines · Gert Doering
Hi,
Quoted 3 lines · Lev Stipakov
+/* + * Disable async-push if plugins are disabled + */ +#if !defined(ENABLE_PLUGIN) &&
defined(ENABLE_ASYNC_PUSH)+#undefENABLE_ASYNC_PUSH+#endif +
Quoted 8 lines
That one gets a NAK from me, for the reasons given.
As long as we *have* --enable/--disable options in configure, they should be validated *there* and not silently ignored.
David's argument about "configure is for finding system dependent things and compiler settings" has some merits, but then we should get rid of all compile-time --enable/--disable settings.
By this logic should we enforce users to add --disable-def-auth if
- --disable-plugin is set? If so, then we should probably also do something with --disable-plugin-auth-pam and
- --disable-plugin-down-root. And with --disable-plugin-auth-pam, we need to look at --enable-pam-dlopen as well.
And to put things in context of the async-push stuff. In the C code we already check if --disable-def-auth is enabled or not.
If we truly start to look into this for the other options, we will have quite a good job ahead of us. Otherwise we end up with even more mess where we are not even consistent.
I say it again, configure.ac is *NOT* the place to do sanity checks on which openvpn features depends on each-other. We have not done that before, and we should not do it now. It just provides far more non-production code to maintain, and more places to look for issues when code we expect to to be compiled in isn't being compiled in. This way leads is a rats nest and will cause much more maintenance burden in the future, which I will not accept lightly.
Yes, I see the short-term convenience to solve that specific Trac ticket. But it really isn't the right long-term solution.
- --
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJYUWe4AAoJEIbPlEyWcf3yX8QQAMDl+us2tVSCCABm0RZ77srQ kRNIrxitehRgzlJi6/Hsod4lDtUxiJ9Q70EjFswHeRkAw9jAD1C69h2Ht4z7etb7 zdqPO+qH44YEA+lDeOu8dcP73q9Ff3izEJdF35ripHCbNymrKkoT+4+OkYDj3VIl xRMLnem3MWfpmdqxzhGdkU/NkSMUMiGF8S7xAJDakvZ7CGvNOfUByWlG7dmUXU9U oRxb0oyJdnSuj8SWDS5sQb6VsXyqAGnKKnE82YL6KZxTYOmri/SkO3HcfBT61dlq bR/CypjuDCppP6bTRegV0xP0V7qNyjm3F6VzTu7Wt5TyFw60AN9xsKV6B71vwUzp h2dhe3QEYQHYJ2m+vcwxRI0CfzO1zQM7UiJ23OjMgYeyMu7Y01fpm/Y1MixTVMct rSeXOpJQkNFCs3v/NPrA8a85IIptDa8cLiOzz3Hca3hIESRmPhZgPxSeAGz+cqNQ Ql8pZ2YMroFUQZIGcsJblXO/s/G8ud4urruXPGnrCj26mGUNBjmht0iLIZs7EJK1 bhviQSkPXeoUSWuonw9TN+hx3w8YRJocHaUkVz26+F9HnLJbma8Sew7BK+pqs1g6 2yNpxYUQtlChbItP+q7dYCYm23m5uRs28sP52u1AKMfeIbA5FPGpqBB/Kjz0ueQn sh5T6CFecL9NAGIxJf9h
=26tU
-----END PGP SIGNATURE-----
Re: [Openvpn-devel] [PATCH v2] Disable async_push if plugins are disabled
Quoted 30 lines · David Sommerseth
Quoted 8 lines
That one gets a NAK from me, for the reasons given.
As long as we *have* --enable/--disable options in configure, they should be validated *there* and not silently ignored.
David's argument about "configure is for finding system dependent things and compiler settings" has some merits, but then we should get rid of all compile-time --enable/--disable settings.
By this logic should we enforce users to add --disable-def-auth if
- --disable-plugin is set? If so, then we should probably also do something with --disable-plugin-auth-pam and
- --disable-plugin-down-root. And with --disable-plugin-auth-pam, we need to look at --enable-pam-dlopen as well.And to put things in context of the async-push stuff. In the C code we already check if --disable-def-auth is enabled or not.
If we truly start to look into this for the other options, we will have quite a good job ahead of us. Otherwise we end up with even more mess where we are not even consistent.
I say it again, configure.ac is *NOT* the place to do sanity checks on which openvpn features depends on each-other. We have not done that before, and we should not do it now. It just provides far more non-production code to maintain, and more places to look for issues when code we expect to to be compiled in isn't being compiled in. This way leads is a rats nest and will cause much more maintenance burden in the future, which I will not accept lightly.
Quoted 2 lines
Yes, I see the short-term convenience to solve that specific Trac ticket. But it really isn't the right long-term solution.
I agree with the spirit of this argument, though I do not know the details of all those options.
But:
When I do
./configure --enable-xyz
I expect the feature to be either enabled or spew out an error saying 'inconsistent options, check configure --help' at the very least.
The build process silently changing the option behind my back and producing a package with no xyz is not good.
Selva
Re: [Openvpn-devel] [PATCH v2] Disable async_push if plugins are disabled
In reply to David SommersethQuoted 17 lines · David Sommerseth
Quoted 7 lines · Gert Doering
Hi,
Quoted 3 lines · Lev Stipakov
+/* + * Disable async-push if plugins are disabled + */ +#if !defined(ENABLE_PLUGIN) &&
defined(ENABLE_ASYNC_PUSH)+#undefENABLE_ASYNC_PUSH+#endif +Quoted 8 lines
That one gets a NAK from me, for the reasons given.
As long as we *have* --enable/--disable options in configure, they should be validated *there* and not silently ignored.
David's argument about "configure is for finding system dependent things and compiler settings" has some merits, but then we should get rid of all compile-time --enable/--disable settings.
Quoted 5 lines
By this logic should we enforce users to add --disable-def-auth if --disable-plugin is set? If so, then we should probably also do something with --disable-plugin-auth-pam and --disable-plugin-down-root. And with --disable-plugin-auth-pam, we need to look at --enable-pam-dlopen as well.
No, unless the user specified --enable-def-auth. And yes, I think it would be nice if we would throw an error if the user did. "Fail-fast" is a really good habit.
Quoted 6 lines
And to put things in context of the async-push stuff. In the C code we already check if --disable-def-auth is enabled or not.
If we truly start to look into this for the other options, we will have quite a good job ahead of us. Otherwise we end up with even more mess where we are not even consistent.
Sure, but "The other stuff is broken" is no argument to not add something that is not broken.
Quoted 7 lines
I say it again, configure.ac is *NOT* the place to do sanity checks on which openvpn features depends on each-other. We have not done that before, and we should not do it now. It just provides far more non-production code to maintain, and more places to look for issues when code we expect to to be compiled in isn't being compiled in. This way leads is a rats nest and will cause much more maintenance burden in the future, which I will not accept lightly.
Yes we do. Just check the mbed TLS pkcs11 checking. That check has saved me some hours of re-running builds at the least. I wish we had more such checks.
Quoted 2 lines
Yes, I see the short-term convenience to solve that specific Trac ticket. But it really isn't the right long-term solution.
I respectfully disagree. I believe this *is* the right long-term fix, and we should aim at doing it this way.
-Steffan
Re: [Openvpn-devel] [PATCH v2] Disable async_push if plugins are disabled
Quoted 24 lines · Steffan Karger
Quoted 17 lines · David Sommerseth
Quoted 7 lines · Gert Doering
Hi,
Quoted 3 lines · Lev Stipakov
+/* + * Disable async-push if plugins are disabled + */ +#if !defined(ENABLE_PLUGIN) &&
defined(ENABLE_ASYNC_PUSH)+#undefENABLE_ASYNC_PUSH+#endif +Quoted 8 lines
That one gets a NAK from me, for the reasons given.
As long as we *have* --enable/--disable options in configure, they should be validated *there* and not silently ignored.
David's argument about "configure is for finding system dependent things and compiler settings" has some merits, but then we should get rid of all compile-time --enable/--disable settings.
Quoted 5 lines
By this logic should we enforce users to add --disable-def-auth if --disable-plugin is set? If so, then we should probably also do something with --disable-plugin-auth-pam and --disable-plugin-down-root. And with --disable-plugin-auth-pam, we need to look at --enable-pam-dlopen as well.
Quoted 3 lines
No, unless the user specified --enable-def-auth. And yes, I think it would be nice if we would throw an error if the user did. "Fail-fast" is a really good habit.
Well, there is still --enable-pam-dlopen --disable-plugin-auth-pam ... and what else can we stubmble accross if we try --disable-server? Or you don't want to bother about that kind of consistency?
Quoted 9 lines
Quoted 6 lines
And to put things in context of the async-push stuff. In the C code we already check if --disable-def-auth is enabled or not.
If we truly start to look into this for the other options, we will have quite a good job ahead of us. Otherwise we end up with even more mess where we are not even consistent.
Sure, but "The other stuff is broken" is no argument to not add something that is not broken.
But it is broken. The v1 patch compiles just fine with --enable-async-push --disable-def-auth
$ grep DEF_AUTH ./config.h /* #undef ENABLE_DEF_AUTH */
And then have a look at multi.c:2214. This is a key code path for this feature. This is behaving exactly how you do not want. If so, the current v1 patch need a NAK.
And if you have --enable-async-push --disable-server, that should also fail. I want to have some consistency.
Which is why I am saying we do not do such sanity checking in configure.ac today.
Quoted 11 lines
Quoted 7 lines
I say it again, configure.ac is *NOT* the place to do sanity checks on which openvpn features depends on each-other. We have not done that before, and we should not do it now. It just provides far more non-production code to maintain, and more places to look for issues when code we expect to to be compiled in isn't being compiled in. This way leads is a rats nest and will cause much more maintenance burden in the future, which I will not accept lightly.
Yes we do. Just check the mbed TLS pkcs11 checking. That check has saved me some hours of re-running builds at the least. I wish we had more such checks.
And *that* is a correct check, not comparable to --enable-async-push at all.
This check detects that your mbedtls library does not support PKCS#11, thus it fails to build as it needs that support by default. The mbedtls library isn't something you normally build yourself, as that is most commonly shipped by the distro - so fixing that either requires disabling PKCS#11 support in OpenVPN or building mbedtls yourself. Failing to do so will _not_ result in a complete build.
But fair enough, I can fully understand the argument that it shouldn't turn off things behind your back, and I can understand the "fail fast" argument too (even though I don't care too much about it in such a small project as OpenVPN). But then lets fix it *properly* in a more consistent way. And we have #error and #warning which may be used in such scenarios.
If the general attitude is that we should do all sanity checking via configure.ac so that ./configure fails on inconsistency, then we need to do something far better than just fixing --enable-async-push --disable-{plugins,def-auth,server} variants. Otherwise we will end up needing to clean-up configure.ac once again [1].
Perhaps it then is better to have some C code which ./configure builds at the very end - where we do all kind of #if defined(...) checks based on ENABLE_* macros defined and bails out with #error or something similar when something is bad. Just so we do this sanity check *one single place* and in a simple manner. Currently we have some 20 ENABLE_* macros handled by config.h alone.
[1] Last massive clean-up:
$ git log --stat 8a4eaf5aa6debaca434..0e4b6c455e0236a4eb4 \
--follow -- configure.acSignature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Quoted 16 lines · Lev Stipakov
Async push functionality makes sense only with deferred authentication, which requires plugins.
Trac #783
Signed-off-by Lev Stipakov <[email protected]...>--- configure.ac | 4 ++++ 1 file changed, 4 insertions(+)configure.ac +0
-0
configure.ac if test "${enable_plugin_auth_pam}" = "yes"; then · line 1213
Quoted 10 lines
configure.ac +4
-0@@ -1204,6 +1204,10 @@ if test "${enable_plugin_auth_pam}" = "yes"; then fi if test "${enable_async_push}" = "yes"; then + if test "${enable_plugins}" = "no"; then + AC_MSG_ERROR([--enable_async_push requires --enable-plugins]) + fi + AC_CHECK_HEADERS( [sys/inotify.h], AC_DEFINE([ENABLE_ASYNC_PUSH], [1], [Enable async push]),
I followed the discussion, gave the arguments some more thought, but still believe this is The Right Thing. So, ACK.
-Steffan