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

[Arne Schwabe]

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

  static void
  dhcp_option_address_parse (const char *name, const char *parm, in_addr_t 
*array, int *len, int msglevel)

#endif +#if defined(WIN32) || defined(TARGET_ANDROID)

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

smime.p7s
Description: S/MIME Kryptografische Unterschrift