Re: [Openvpn-devel] [PATCH 4/5] Android platform specific changes.

[Arne S. <ar...@rf...>]

Am 25.04.13 20:56, schrieb Gert Doering:
> Hi,
>
> On Sat, Apr 20, 2013 at 04:22:47PM +0200, Arne Schwabe wrote:
>> index 05c6da2..9fdfd88 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -1125,7 +1125,9 @@ show_tuntap_options (const struct tuntap_options *o)
>>   }
>>   
>>   #endif
>> +#endif
>>   
>> +#if defined(WIN32) || defined(TARGET_ANDROID)
>>   static void
>>   dhcp_option_address_parse (const char *name, const char *parm, in_addr_t *array, int *len, int msglevel)
> I'm not sure if I understand these bits, will have to look at the patched
> result.  It looks a bit confusing why there would be anything you need
> to have "on WIN32 and Android, and nowhere else".
The dhcp-options are only parsed currently if OpenVPN is compiled for 
windows. Since I want to support dhcp-options dns I need to enable these 
options. The  tt->options.domain and tt->dns options which are used to 
pass DNS/Domain via management interface are only available when these 
options are parsed.
>
>> +#elif defined (TARGET_ANDROID)
>> +
>> +    struct user_pass up;
>> +    struct buffer out = alloc_buf_gc (64, &gc);
>> +
>> +    buf_printf (&out, "%s %s", network, netmask);
>> +
>> +    strcpy(up.username, buf_bptr(&out));
>> +    management_query_user_pass(management, &up , "ROUTE", GET_USER_PASS_NEED_OK,(void*) 0);
>> +
>> +
> But this is what I want to comment on.  I can see what you're doing, and
> I'm fine with that - but I strongly don't like the resulting code - what
> do you think about hiding this behind a wrapper
>
>    management_android_control( management, buf_bptr(&out), "ROUTE" );
>
> or such, which takes a "char *" or a "struct buffer *", and does the
> same thing internally?  Having "struct user_pass" all over the code,
> using it as a generic "transport this string to the management interface"
> container will mightily confuse people looking for "ok, so what is this
> code doing, and what does it have to do with username and password
> queries on the management interface"?
That is a good idea. I will update the patch with that.
>> +#elif defined(TARGET_ANDROID)
>> +  msg (M_NONFATAL, "Sorry, deleting routes on Android is not possible. The VpnService API allows routes to be set on connect only.");
> For this message, I'm wondering how useful it is.  Isn't this printed on
> *every* disconnect?
>
> [..]
Hm I could go for a better warning message. But this message not shown 
that often iirc. Have to check the code again. It over one year since I 
wrote that part :)
>> +    if(tt->options.domain) {
>> +        strcpy(up.username , tt->options.domain);
>> +        management_query_user_pass(management, &up , "DNSDOMAIN", GET_USER_PASS_NEED_OK,(void*) 0);
>> +    }
> The wrapper mentioned above could even have a check for "if the pointer
> is NULL, silently don't do anything", so this would collapse to
>
>    management_android_control( management, tt->options.domain, "DNSDOMAIN" );
>
> "just sayin'" :-)
>
>
I will look into it.

Arne