| Message ID | 20190812134513.20758-1-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/2] Fix check if iface name is set | expand |
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; > } >
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).
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,
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 <string.h> int test(const char *buf) { return (strlen(buf) > 0); } for example compiles with -O3 on my GCC 8.3 to 0000000000000000 <test>: 0: 31 c0 xor %eax,%eax 2: 80 3f 00 cmpb $0x0,(%rdi) 5: 0f 95 c0 setne %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 :) FOOOOM [0] -Steffan [0] https://xkcd.com/356/
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 <string.h> > > int test(const char *buf) { > return (strlen(buf) > 0); > } > > for example compiles with -O3 on my GCC 8.3 to > > 0000000000000000 <test>: > 0: 31 c0 xor %eax,%eax > 2: 80 3f 00 cmpb $0x0,(%rdi) > 5: 0f 95 c0 setne %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 ;-)
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; }
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(-)