Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-15 Thread Antonio Quartulli
Hi Arne,

better mark the patches with v2.
They are also both missing the signed-off-by line.

how about git format-patch -v2 -s -2 ? :)

Cheers,

On 15/08/2019 13:52, Arne Schwabe wrote:
> Clang/Android complained
> 
>  warning: address of array 'rgi6->iface' will always evaluate to 'true' 
> [-Wpointer-bool-conversion]
>   if (rgi6->iface)
> 
> iface is a char[16]; So its pointer is always true.
> 
> we do a CLEAR(rgi6) always before setting this struct and strcpy the
> name into iface. So using strlen instead of checking for the pointer
> should be the right fix.
> ---
>  src/openvpn/route.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 5f63fd34..a302746e 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -3349,7 +3349,7 @@ get_default_gateway_ipv6(struct route_ipv6_gateway_info 
> *rgi6,
>  rgi6->flags |= RGI_ADDR_DEFINED;
>  }
>  
> -if (rgi6->iface)
> +if (strlen(rgi6->iface))
>  {
>  rgi6->flags |= RGI_IFACE_DEFINED;
>  }
> 

-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-13 Thread David Sommerseth
On 13/08/2019 23:46, Steffan Karger wrote:
> Hi,
> 
> On 13-08-19 23:31, Antonio Quartulli wrote:
>> On 13/08/2019 23:26, David Sommerseth wrote:
>>> wouldn't it be better to
>>> do 'if (rgi6->iface[0])' instead?  Since the buffer should be NULL 
>>> terminated
>>> and has to be NULL terminted for strlen() to function anyhow.  But the
>>> compiled code would be a bit more efficient (even though, this isn't
>>> necessarily a performance critical code section).
>>
>> In my opinion strlen() is more readable for the casual developer
>> checking this code. Behind iface[0] we may hide other ambiguous
>> assumptions (even though this is not the case here, but we won't
>> remember in some months from now).
> 
> Antionio's answer is of course the real reason to use strlen(), but you
> nerd-sniped me by saying "compiled code would be a bit more efficient".
> As you said, that wouldn't matter here, but still: compilers nowadays do
> optimize calls to strlen, memset, memcpy, etc.
> 
> The following code
> 
> #include 
> 
> int test(const char *buf) {
> return (strlen(buf) > 0);
> }
> 
> for example compiles with -O3 on my GCC 8.3 to
> 
>  :
>0: 31 c0   xor%eax,%eax
>2: 80 3f 00cmpb   $0x0,(%rdi)
>5: 0f 95 c0setne  %al
>8: c3  retq
> 
> which indeed just compares the first byte of the buffer against zero.
> 
> So, unless there are important performance constraints, let's optimize
> code for readability, rather than performance :)

Cool!  Thanks!  I didn't run such a check, as I really didn't expect the
compiler to be _that_ smart ... but I agree with you both, and especially when
the end result is optimized for both readability *and* performance ;-)


-- 
kind regards,

David Sommerseth
OpenVPN Inc




signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-13 Thread Steffan Karger
Hi,

On 13-08-19 23:31, Antonio Quartulli wrote:
> On 13/08/2019 23:26, David Sommerseth wrote:
>> wouldn't it be better to
>> do 'if (rgi6->iface[0])' instead?  Since the buffer should be NULL terminated
>> and has to be NULL terminted for strlen() to function anyhow.  But the
>> compiled code would be a bit more efficient (even though, this isn't
>> necessarily a performance critical code section).
> 
> In my opinion strlen() is more readable for the casual developer
> checking this code. Behind iface[0] we may hide other ambiguous
> assumptions (even though this is not the case here, but we won't
> remember in some months from now).

Antionio's answer is of course the real reason to use strlen(), but you
nerd-sniped me by saying "compiled code would be a bit more efficient".
As you said, that wouldn't matter here, but still: compilers nowadays do
optimize calls to strlen, memset, memcpy, etc.

The following code

#include 

int test(const char *buf) {
return (strlen(buf) > 0);
}

for example compiles with -O3 on my GCC 8.3 to

 :
   0:   31 c0   xor%eax,%eax
   2:   80 3f 00cmpb   $0x0,(%rdi)
   5:   0f 95 c0setne  %al
   8:   c3  retq

which indeed just compares the first byte of the buffer against zero.

So, unless there are important performance constraints, let's optimize
code for readability, rather than performance :)

FM [0]

-Steffan

[0] https://xkcd.com/356/


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-13 Thread Antonio Quartulli
Hi,

On 13/08/2019 23:26, David Sommerseth wrote:
> wouldn't it be better to
> do 'if (rgi6->iface[0])' instead?  Since the buffer should be NULL terminated
> and has to be NULL terminted for strlen() to function anyhow.  But the
> compiled code would be a bit more efficient (even though, this isn't
> necessarily a performance critical code section).

In my opinion strlen() is more readable for the casual developer
checking this code. Behind iface[0] we may hide other ambiguous
assumptions (even though this is not the case here, but we won't
remember in some months from now).

Cheers,

-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-13 Thread David Sommerseth
On 13/08/2019 11:03, Antonio Quartulli wrote:
> Hi Arne,
> 
> On 12/08/2019 15:45, Arne Schwabe wrote:
>> Clang/Android complained
>>
>>  warning: address of array 'rgi6->iface' will always evaluate to 'true' 
>> [-Wpointer-bool-conversion]
>>   if (rgi6->iface)
>>
>> iface is a char[16]; So its pointer is always true.
>>
>> we do a CLEAR(rgi6) always before setting this struct and strcpy the
>> name into iface. So using strlen instead of checking for the pointer
>> should be the right fix.
> 
> Thanks for fixing this!
> 
> However you're missing the signed off line.
> 
> 
>> ---
>>  src/openvpn/route.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
>> index 5f63fd34..a302746e 100644
>> --- a/src/openvpn/route.c
>> +++ b/src/openvpn/route.c
>> @@ -3349,7 +3349,7 @@ get_default_gateway_ipv6(struct 
>> route_ipv6_gateway_info *rgi6,
>>  rgi6->flags |= RGI_ADDR_DEFINED;
>>  }
>>  
>> -if (rgi6->iface)
>> +if (strlen(rgi6->iface))
> 
> how about adding a "> 0"? I know it's basically the same here, but I
> think that's the style we use everywhere.
Agreed, but just thinking aloud ... since we use CLEAR() and this is a static
allocated buffer; constant size always "readable" - wouldn't it be better to
do 'if (rgi6->iface[0])' instead?  Since the buffer should be NULL terminated
and has to be NULL terminted for strlen() to function anyhow.  But the
compiled code would be a bit more efficient (even though, this isn't
necessarily a performance critical code section).


-- 
kind regards,

David Sommerseth
OpenVPN Inc



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set

2019-08-13 Thread Antonio Quartulli
Hi Arne,

On 12/08/2019 15:45, Arne Schwabe wrote:
> Clang/Android complained
> 
>  warning: address of array 'rgi6->iface' will always evaluate to 'true' 
> [-Wpointer-bool-conversion]
>   if (rgi6->iface)
> 
> iface is a char[16]; So its pointer is always true.
> 
> we do a CLEAR(rgi6) always before setting this struct and strcpy the
> name into iface. So using strlen instead of checking for the pointer
> should be the right fix.

Thanks for fixing this!

However you're missing the signed off line.


> ---
>  src/openvpn/route.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index 5f63fd34..a302746e 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -3349,7 +3349,7 @@ get_default_gateway_ipv6(struct route_ipv6_gateway_info 
> *rgi6,
>  rgi6->flags |= RGI_ADDR_DEFINED;
>  }
>  
> -if (rgi6->iface)
> +if (strlen(rgi6->iface))

how about adding a "> 0"? I know it's basically the same here, but I
think that's the style we use everywhere.

Cheers,

>  {
>  rgi6->flags |= RGI_IFACE_DEFINED;
>  }
> 

-- 
Antonio Quartulli


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel