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".
> +#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"?
> +#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?
[..]
> + 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'" :-)
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
pgpzp3N1XozIV.pgp
Description: PGP signature
