Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set
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
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
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
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 2/2] Adjust Android code after sitnl patch merge
Hi Arne,
On 12/08/2019 15:45, Arne Schwabe wrote:
> It turns out that the only part of Android that still shares routing
> code with Linux is the get_default_ipv6 method.
>
> Instead of fixing a method that makes little sense on Android anyway,
> have a method that returns a fake ipv6 gateway like for ipv4.
you've missed the Signed-off line here too :)
I can't comment much on the Android code as I have never built openvpn
for it, however, when playing with platform-ifdef I always recommend to
run your branch through the buildbot.
This way we can easily see if we have made big mistakes in regards to
other platforms.
Cheers,
> ---
> src/openvpn/route.c | 66 ++---
> src/openvpn/tun.c | 9 +--
> 2 files changed, 51 insertions(+), 24 deletions(-)
>
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index a302746e..9af88f00 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -1065,7 +1065,8 @@ redirect_default_route_to_vpn(struct route_list *rl,
> const struct tuntap *tt,
> tt,
> flags,
> >rgi,
> -es);
> +es,
> +ctx);
>
> #else
> if (rl->flags & RG_DEF1)
> @@ -3169,7 +3170,48 @@ show_routes(int msglev)
> gc_free();
> }
>
> -#elif defined(TARGET_LINUX) || defined(TARGET_ANDROID)
> +#elif defined(TARGET_ANDROID)
> +
> +void
> +get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx)
> +{
> +/* Android, set some pseudo GW, addr is in host byte order,
> + * Determining the default GW on Android 5.0+ is non trivial
> + * and serves almost no purpose since OpenVPN only uses the
> + * default GW address to add routes for networks that should
> + * NOT be routed over the VPN. Using a well known address
> + * (127.'d'.'g'.'w') for the default GW make detecting
> + * these routes easier from the controlling app.
> + */
> +CLEAR(*rgi);
> +
> +rgi->gateway.addr = 127 << 24 | 'd' << 16 | 'g' << 8 | 'w';
> +rgi->flags = RGI_ADDR_DEFINED | RGI_IFACE_DEFINED;
> +strcpy(rgi->iface, "android-gw");
> +
> +/* Skip scanning/fetching interface from loopback interface we do
> + * normally on Linux.
> + * It always fails and "ioctl(SIOCGIFCONF) failed" confuses users
> + */
> +
> +}
> +
> +void
> +get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
> + const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
> +{
> +/* Same for ipv6 */
> +
> +CLEAR(*rgi6);
> +
> +/* Use a fake link-local address */
> +ASSERT(inet_pton(AF_INET6, "fe80::ad", >addrs->addr_ipv6) == 1);
> +rgi6->addrs->netbits_ipv6 = 64;
> +rgi6->flags = RGI_ADDR_DEFINED | RGI_IFACE_DEFINED;
> +strcpy(rgi6->iface, "android-gw");
> +}
> +
> +#elif defined(TARGET_LINUX)
>
> void
> get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx)
> @@ -3181,7 +3223,6 @@ get_default_gateway(struct route_gateway_info *rgi,
> openvpn_net_ctx_t *ctx)
> CLEAR(*rgi);
> CLEAR(best_name);
>
> -#ifndef TARGET_ANDROID
> /* get default gateway IP addr */
> if (net_route_v4_best_gw(ctx, NULL, >gateway.addr, best_name) == 0)
> {
> @@ -3191,25 +3232,6 @@ get_default_gateway(struct route_gateway_info *rgi,
> openvpn_net_ctx_t *ctx)
> rgi->flags |= RGI_ON_LINK;
> }
> }
> -#else /* ifndef TARGET_ANDROID */
> -/* Android, set some pseudo GW, addr is in host byte order,
> - * Determining the default GW on Android 5.0+ is non trivial
> - * and serves almost no purpose since OpenVPN only uses the
> - * default GW address to add routes for networks that should
> - * NOT be routed over the VPN. Using a well known address
> - * (127.'d'.'g'.'w') for the default GW make detecting
> - * these routes easier from the controlling app.
> - */
> -rgi->gateway.addr = 127 << 24 | 'd' << 16 | 'g' << 8 | 'w';
> -rgi->flags |= RGI_ADDR_DEFINED;
> -strcpy(best_name, "android-gw");
> -
> -/*
> - * Skip scanning/fetching interface from loopback interface
> - * It always fails and "ioctl(SIOCGIFCONF) failed" confuses users
> - */
> -goto done;
> -#endif /* ifndef TARGET_ANDROID */
>
> /* scan adapter list */
> if (rgi->flags & RGI_ADDR_DEFINED)
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 8f8f7c6c..1db459f8 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -906,9 +906,13 @@ do_ifconfig_ipv6(struct tuntap *tt, const char *ifname,
> int tun_mtu,
> #elif defined(TARGET_ANDROID)
> char out6[64];
>
> +const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0,
> NULL);
> openvpn_snprintf(out6, sizeof(out6), "%s/%d %d",
> - ifconfig_ipv6_local,tt->netbits_ipv6, tun_mtu);
> +
Re: [Openvpn-devel] [PATCH 1/2] Fix check if iface name is set
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
