[Openvpn-devel,v2] ssl_verify: define label only when required

Message ID 20180815061240.5027-1-a@unstable.cc
State New
Headers show
Series
  • [Openvpn-devel,v2] ssl_verify: define label only when required
Related show

Commit Message

Antonio Quartulli Aug. 15, 2018, 6:12 a.m.
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(+)

Comments

Matthias Andree Aug. 16, 2018, 11:08 a.m. | #1
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
Antonio Quartulli Aug. 16, 2018, 11:17 a.m. | #2
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,
Gert Doering Aug. 16, 2018, 11:37 a.m. | #3
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
Antonio Quartulli Aug. 16, 2018, 12:05 p.m. | #4
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,
David Sommerseth Aug. 16, 2018, 8:28 p.m. | #5
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.
Gert Doering Aug. 16, 2018, 9:33 p.m. | #6
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

Patch

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;
 }