| Message ID | 20180815061240.5027-1-a@unstable.cc |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,v2] ssl_verify: define label only when required | expand |
Am 15.08.2018 um 08:12 schrieb Antonio Quartulli: > The "cleanup" label in ssl_verify.c:verify_user_pass_plugin() is used > only when PLUGIN_DEF_AUTH is defined, therefore make the label > definition dependent on the same define. > > Fixes the following warning when PLUGIN_DEF_AUTH is not defined: > > ssl_verify.c: In function 'verify_user_pass_plugin': > ssl_verify.c:1223:1: warning: label 'cleanup' defined but not used [-Wunused-label] > cleanup: > ^~~~~~~ > > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > src/openvpn/ssl_verify.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index 61872251..5b46ea72 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -1220,7 +1220,9 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up, > msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username"); > } > > +#ifdef PLUGIN_DEF_AUTH > cleanup: > +#endif > return retval; > } > NAK. This is useless #ifdef noise in the code and makes it harder to read, for compile-time cosmetics. If you don't want the warnings, feed a proper -Wno-unused-label to your compiler and move on. ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 16/08/18 19:08, Matthias Andree wrote: > Am 15.08.2018 um 08:12 schrieb Antonio Quartulli: >> The "cleanup" label in ssl_verify.c:verify_user_pass_plugin() is used >> only when PLUGIN_DEF_AUTH is defined, therefore make the label >> definition dependent on the same define. >> >> Fixes the following warning when PLUGIN_DEF_AUTH is not defined: >> >> ssl_verify.c: In function 'verify_user_pass_plugin': >> ssl_verify.c:1223:1: warning: label 'cleanup' defined but not used [-Wunused-label] >> cleanup: >> ^~~~~~~ >> >> Signed-off-by: Antonio Quartulli <a@unstable.cc> >> --- >> src/openvpn/ssl_verify.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c >> index 61872251..5b46ea72 100644 >> --- a/src/openvpn/ssl_verify.c >> +++ b/src/openvpn/ssl_verify.c >> @@ -1220,7 +1220,9 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up, >> msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username"); >> } >> >> +#ifdef PLUGIN_DEF_AUTH >> cleanup: >> +#endif >> return retval; >> } >> > > NAK. > > This is useless #ifdef noise in the code and makes it harder to read, > for compile-time cosmetics. > > If you don't want the warnings, feed a proper -Wno-unused-label to your > compiler and move on. Thanks for taking the time to look at this! Actually we are in the process of getting rid of the various ifdef around the code or at least we are trying to bring that to a more reasonable state (it takes time....and right now we are more focussed on the networking and platform side of things). I totally agree with you that more ifdefs are just ugly and make the code worse. However this "new" ifdef is just part of some logic that is already there (a few lines above). This said, in the past months we have tried moving towards a 0-warnings-tolerance, so that we could catch as many bugs as possible hidden by "just annoying warnings". For this reason using "-Wno-unused-label" is not an option (especially in CI/automated build tests where this was spotted). Imho we should just get this patch through and then try to get rid of the whole #ifdef PLUGIN_DEF_AUTH logic at once. Cheers,
Hi, On Thu, Aug 16, 2018 at 07:17:09PM +0800, Antonio Quartulli wrote: > I totally agree with you that more ifdefs are just ugly and make the > code worse. However this "new" ifdef is just part of some logic that is > already there (a few lines above). It is a new #ifdef, two more lines of code, breaking the flow of reading. So I tend to agree with Mathias that this might not be the best approach here. > This said, in the past months we have tried moving towards a > 0-warnings-tolerance, so that we could catch as many bugs as possible > hidden by "just annoying warnings". > For this reason using "-Wno-unused-label" is not an option (especially > in CI/automated build tests where this was spotted). I see "unused label" as a minor nuisance (unlike other warnings there is nothing like "it could be an integer overflow" or "watch out for the signs of your variables" here), so -Wno-unused-label would work for me... Warnings are generally good, but adding extra lines just to silence the compiler in a specific combination of flags for a warning that is otherwise harmless is not always a good way forward. > Imho we should just get this patch through and then try to get rid of > the whole #ifdef PLUGIN_DEF_AUTH logic at once. If the idea is to get rid of PLUGIN_DEF_AUTH, then let's do so, not add extra #ifdef :-) Anyway. Trying to understand the logic here - PLUGIN_DEF_AUTH is always defined if ENABLE_DEF_AUTH is set, which seems to be the default setting. So, questions - is there actually some benefit in --disable-def-auth? code size, performance, platform compatibility? - why do we have it in the first place? - can we get rid of it? gert
Hi, On 16/08/18 19:37, Gert Doering wrote: >> Imho we should just get this patch through and then try to get rid of >> the whole #ifdef PLUGIN_DEF_AUTH logic at once. > > If the idea is to get rid of PLUGIN_DEF_AUTH, then let's do so, not > add extra #ifdef :-) Good! Let's focus on that then :) > > > Anyway. Trying to understand the logic here - PLUGIN_DEF_AUTH is always > defined if ENABLE_DEF_AUTH is set, which seems to be the default setting. > > So, questions > > - is there actually some benefit in --disable-def-auth? code size, > performance, platform compatibility? > > - why do we have it in the first place? > to me this feels like the classic pattern of "let's introduce this new feature, but let's add a compile time option in case people have troubles with it or don't want it for some reason". I have seen this often also in newer code (like ovpn3). > - can we get rid of it? > Does any distro package openvpn with --disable-def-auth selected? If not, I guess we can get rid of it. But maybe David knows something we don't :-) Cheers,
On 16/08/18 13:37, Gert Doering wrote: [...snip...] I do see the arguments on both sides here. And in particular where these warnings halts the compilation (which I think I read some where that newer GCC compilers would do by default). But ... Gert asks a few reasonable questions > Anyway. Trying to understand the logic here - PLUGIN_DEF_AUTH is always > defined if ENABLE_DEF_AUTH is set, which seems to be the default setting. > > So, questions > > - is there actually some benefit in --disable-def-auth? code size, > performance, platform compatibility? > > - why do we have it in the first place? I tend to say no. ENABLE_DEF_AUTH is about deferred authentication. This has been enabled by default for so long, that I hardly think it would be an issue killing the possibility to turn off this feature. All it essentially does is to allow plug-ins to return OPENVPN_PLUGIN_FUNC_DEFERRED instead of OPENVPN_PLUGIN_FUNC_ERROR or OPENVPN_PLUGIN_FUNC_SUCCESS. This feature will also improve the authentication performance by not locking up other connected clients if the authentication backend is slow *and* implements support for deferred authentication. So there is a performance gain for authentication plugins making use of this feature. Otherwise, I do not expect much performance difference. > - can we get rid of it? I am definitely leaning towards "Yes!". This does grow the resulting binary by some bytes (I don't know how much; haven't checked), as it adds code which prepares a file the plug-in will use to write the final verdict of the authentication; plus the code paths related to polling this file the core OpenVPN process. If the If the code grows enough to validate the need for this macro, I am not so sure of. The only users I would expect might do builds with --disable-def-auth are the builds for embedded devices. But I don't know how much of a concern this is any more; this feature shouldn't double or half the size of the resulting code. There is also a related ./configure option: --enable-async-push. This adds inotify support, which reworks the polling model to be a notification based push model instead (iirc). But this I think makes sense to have as it is today for the time being. We might want to consider flipping it to be enabled by default, though.
Hi, thanks for your thoughts so far, so it seems we just want to get rid of these conditionals for 2.5 :-) One thing, though: On Thu, Aug 16, 2018 at 10:28:05PM +0200, David Sommerseth wrote: > There is also a related ./configure option: --enable-async-push. This adds > inotify support, which reworks the polling model to be a notification based > push model instead (iirc). But this I think makes sense to have as it is > today for the time being. We might want to consider flipping it to be enabled > by default, though. This carries along some platform dependencies, as "inotify" is not unversally available (I was about to write "Linux only", but I bet one of the BSDs has it, too, by now :-) - but definitely not all supported platforms). Google points to "fswatch", which is "A cross-platform file change monitor with multiple backends: Apple OS X File System Events, *BSD kqueue, Solaris/Illumos File Events Notification, Linux inotify, ..." so yeah - this has to stay an #ifdef for a while (and more esoteric platforms like *cough* Windows or AIX might not have anything reasonably compatible at all) gert
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 61872251..5b46ea72 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1220,7 +1220,9 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up, msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username"); } +#ifdef PLUGIN_DEF_AUTH cleanup: +#endif return retval; }
The "cleanup" label in ssl_verify.c:verify_user_pass_plugin() is used only when PLUGIN_DEF_AUTH is defined, therefore make the label definition dependent on the same define. Fixes the following warning when PLUGIN_DEF_AUTH is not defined: ssl_verify.c: In function 'verify_user_pass_plugin': ssl_verify.c:1223:1: warning: label 'cleanup' defined but not used [-Wunused-label] cleanup: ^~~~~~~ Signed-off-by: Antonio Quartulli <a@unstable.cc> --- src/openvpn/ssl_verify.c | 2 ++ 1 file changed, 2 insertions(+)