Use adapter index instead of name

PATCH Superseded Lev Stipakov 2 participants 12 messages Nov 9, 2015 – Dec 13, 2015

Share & tools

Lev Stipakov

author

2015-11-09 14:35 UTC #0 lore raw

PATCH v2 1/1 Use adapter index instead of name

v2:
* Remove netsh call which uses adapter name. After thoughtful testing
turns out that "adapter name" code branch is never used.

Some windows machines get weird issues with netsh when using adapter name on "netsh.exe interface ipv6 set address" command.

Changed logic to get adapter index and use it instead of adapter name for netsh set address command. if unable to get adapter index, try with adapter name.

Signed-off-by Olli Mannisto <[email protected]...>

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/tun.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

src/openvpn/tun.c +5 -5

index 24a61ec..0347a52 100644
@@ -1301,18 +1301,18 @@ do_ifconfig (struct tuntap *tt,
     if ( do_ipv6 )
       {
 	char * saved_actual;
+	const DWORD idx = get_adapter_index_flexible(actual);
 
 	if (!strcmp (actual, "NULL"))
 	  msg (M_FATAL, "Error: When using --tun-ipv6, if you have more than one TAP-Windows adapter, you must also specify --dev-node");
 
-	/* example: netsh interface ipv6 set address MyTap 2001:608:8003::d store=active */
+	/* example: netsh interface ipv6 set address 42 2001:608:8003::d store=active */
 	argv_printf (&argv,
-		    "%s%sc interface ipv6 set address %s %s store=active",
+		     "%s%sc interface ipv6 set address %u %s store=active",
 		     get_win_sys_path(),
 		     NETSH_PATH_SUFFIX,
-		     actual,
-		     ifconfig_ipv6_local );
-
+		     idx,
+		     ifconfig_ipv6_local);
 	netsh_command (&argv, 4);
 
 	/* explicit route needed */

Signature · 1 line

1.9.1

Gert Doering

2015-11-09 14:55 UTC #1 lore raw

Hi,

Quoted 3 lines · Lev Stipakov

v2:
* Remove netsh call which uses adapter name. After thoughtful testing
turns out that "adapter name" code branch is never used.

Not there yet :-) - but thanks for picking this up.

As I said for v1, if we change this for the "set address" part of the code, because "it will sometimes not work otherwise", we should also change it for the "set route" code - consistent usage of netsh, easier to understand when reading the log, etc.

It should actually be not very hard - we should be able to set "tt->actual" to read "interface=nnn", and then it should work automagically without even touching route.c at all

src/openvpn/tun.c do_ifconfig (struct tuntap *tt, · line 1309

Quoted 2 lines

src/openvpn/tun.c +1 -1

-	/* example: netsh interface ipv6 set address MyTap 2001:608:8003::d store=active */
+	/* example: netsh interface ipv6 set address 42 2001:608:8003::d store=active */

What does surprise me, though, is that it works for you with just specifying the interface index, without "IF" or "interface=" before it.

When setting IPv6 routes for non-tun/tap gateways (route.c, line 1770, in git master) I use "interface=<ifindex>" and that is what the online help told me to use.

      struct buffer out = alloc_buf_gc (64, &gc);
      buf_printf (&out, "interface=%d", r6->adapter_index );
      device = buf_bptr(&out);
...
  argv_printf (&argv, "%s%sc interface ipv6 add route %s/%d %s",
               get_win_sys_path(),
               NETSH_PATH_SUFFIX,
               network,
               r6->netbits,
               device);

is the syntax different for "set address"?

(IPv4 code uses "IF <index>"... hrmph)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Lev Stipakov

author

2015-11-11 08:05 UTC #2 lore raw

Hi,

Quoted 4 lines

It should actually be not very hard - we should be able to set "tt->actual" to read "interface=nnn", and then it should work automagically without even touching route.c at all

Setting "interface=<idx>" to "tt->actual_name" will affect all code branches which use that value, for example "netsh_enable_dhcp". In this particular case we cannot use index and must use interface name, according to MS (https://technet.microsoft.com/en-us/library/cc731521(v=ws.10).aspx#BKMK_setaddress)

Same documentation claims that "set/delete address" accepts interface index only for ipv6. That said, "add/delete route" accepts index for both ipv4/ipv6. Besides, ipv4 syntax for "set address" is "[ name =] InterfaceName" and ipv6 is "[[ interface=] String]".

So, if we want to use index also for "add/del route", I'd gently modify add/del_route_ipv6 and make it use "interface=<idx>" (without breaking "vpn server special route" case).

Quoted 3 lines

What does surprise me, though, is that it works for you with just specifying the interface index, without "IF" or "interface=" before it.

MS says "[[ interface=] String] Specifies an interface name or index" for "set address" and "add route". If I read it right, "interface" prefix can be omitted. But let's use it for consistency with existing code.

-Lev

Gert Doering

2015-11-11 08:16 UTC #3 lore raw

Hi,

Quoted 10 lines · Lev Stipakov

Quoted 4 lines
It should actually be not very hard - we should be able to set "tt->actual" to read "interface=nnn", and then it should work automagically without even touching route.c at all
Setting "interface=<idx>" to "tt->actual_name" will affect all code branches which use that value, for example "netsh_enable_dhcp". In this particular case we cannot use index and must use interface name, according to MS
(https://technet.microsoft.com/en-us/library/cc731521(v=ws.10).aspx#BKMK_setaddress)

Uh. Thanks for investigating. Why, oh why... it was naive of me to expect consistency here, I should have known better :-(

[..]

Quoted 3 lines

So, if we want to use index also for "add/del route", I'd gently modify add/del_route_ipv6 and make it use "interface=<idx>" (without breaking "vpn server special route" case).

For consistency, I think we should do that. What I'd avoid is to do the adapter_index lookup for every single route - which would imply adding the index to struct tuntap_options or something like that which is available in add/del_route[_ipv6]() already. But this is less elegant than I hoped for, and might have to look different for 2.3 and master.

So maybe make this a separate patch?

Quoted 7 lines

Quoted 3 lines
What does surprise me, though, is that it works for you with just specifying the interface index, without "IF" or "interface=" before it.
MS says "[[ interface=] String] Specifies an interface name or index" for "set address" and "add route". If I read it right, "interface" prefix can be omitted. But let's use it for consistency with existing code.

... and make this ("interface=<idx>") a v3 patch, only for "set address" for now, to move forward.

thanks,

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Lev Stipakov

author

2015-11-11 11:48 UTC #4 lore raw

PATCH v3 1/1 Use adapter index instead of name

v3:
* Use interface=<idx> syntax.
* Add forward declaration of get_adapter_index_flexible to get rid of warning.

v2:
* Remove netsh call which uses adapter name. After thoughtful testing
turns out that "adapter name" code branch is never used.

Some windows machines get weird issues with netsh when using adapter name on "netsh.exe interface ipv6 set address" command.

Changed logic to get adapter index and use it instead of adapter name for netsh set address command. if unable to get adapter index, try with adapter name.

Signed-off-by Olli Mannisto <[email protected]...>

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/tun.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

src/openvpn/tun.c +9 -5

index 24a61ec..070fd18 100644
@@ -67,6 +67,8 @@ static void netsh_command (const struct argv *a, int n);
 
 static const char *netsh_get_id (const char *dev_node, struct gc_arena *gc);
 
+static DWORD get_adapter_index_flexible (const char *name);
+
 #endif
 
 #ifdef TARGET_SOLARIS

@@ -1301,18 +1303,20 @@ do_ifconfig (struct tuntap *tt,
     if ( do_ipv6 )
       {
 	char * saved_actual;
+	char iface[64];
 
 	if (!strcmp (actual, "NULL"))
 	  msg (M_FATAL, "Error: When using --tun-ipv6, if you have more than one TAP-Windows adapter, you must also specify --dev-node");
 
-	/* example: netsh interface ipv6 set address MyTap 2001:608:8003::d store=active */
+	openvpn_snprintf(iface, sizeof(iface), "interface=%lu", get_adapter_index_flexible(actual));
+
+	/* example: netsh interface ipv6 set address interface=42 2001:608:8003::d store=active */
 	argv_printf (&argv,
-		    "%s%sc interface ipv6 set address %s %s store=active",
+		     "%s%sc interface ipv6 set address %s %s store=active",
 		     get_win_sys_path(),
 		     NETSH_PATH_SUFFIX,
-		     actual,
-		     ifconfig_ipv6_local );
-
+		     iface,
+		     ifconfig_ipv6_local);
 	netsh_command (&argv, 4);
 
 	/* explicit route needed */

Signature · 1 line

1.9.1

Gert Doering

2015-11-11 19:40 UTC #5 lore raw

Hi,

src/openvpn/tun.c do_ifconfig (struct tuntap *tt, · line 1311

Quoted 11 lines · Lev Stipakov

src/openvpn/tun.c +2 -1

@@ -1301,18 +1303,20 @@ do_ifconfig (struct tuntap *tt,
     if ( do_ipv6 )
       {
 	char * saved_actual;
+	char iface[64];
 
 	if (!strcmp (actual, "NULL"))
 	  msg (M_FATAL, "Error: When using --tun-ipv6, if you have more than one TAP-Windows adapter, you must also specify --dev-node");
 
-	/* example: netsh interface ipv6 set address MyTap 2001:608:8003::d store=active */
+	openvpn_snprintf(iface, sizeof(iface), "interface=%lu", get_adapter_index_flexible(actual));

Uh. I'm afraid if I speak up once more, you'll stop talking to me... (or not give me any beer next time we meet :) )

src/openvpn/tun.c do_ifconfig (struct tuntap *tt, · line 1315

Quoted 9 lines

src/openvpn/tun.c +2 -4

+	/* example: netsh interface ipv6 set address interface=42 2001:608:8003::d store=active */
 	argv_printf (&argv,
-		    "%s%sc interface ipv6 set address %s %s store=active",
+		     "%s%sc interface ipv6 set address %s %s store=active",
 		     get_win_sys_path(),
 		     NETSH_PATH_SUFFIX,
-		     actual,
-		     ifconfig_ipv6_local );
-

... but wouldn't

argv_printf (&argv,
"%s%sc interface ipv6 set address interface=%lu %s store=active",
get_win_sys_path(),
NETSH_PATH_SUFFIX,

		     get_adapter_index_flexible(actual),
		     ifconfig_ipv6_local);

(without storing into a temp variable) work "just fine"?

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

2015-11-13 07:56 UTC #6 lore raw

Hi,

Quoted 10 lines · Gert Doering

... but wouldn't
argv_printf (&argv,
"%s%sc interface ipv6 set address interface=%lu %s store=active",
get_win_sys_path(),
NETSH_PATH_SUFFIX,
		     get_adapter_index_flexible(actual),
		     ifconfig_ipv6_local);
(without storing into a temp variable) work "just fine"?

Turns out this does not work, as argv_printf() does not implement a generic printf() parser, but splits up the string at whitespace boundaries and then handles each element explicitly - and '%-parsing' is only done for known cases, and then the % part has to stand on its own, with some special exceptions...

          else if (!strcmp (term, "%s/%d"))
...
          else if (!strcmp (term, "%s%sc"))

...

given that, I think the v3 patch is as good as it's going to get without extending argv_printf() - IOW, ACK, and I'll merge later. Thanks.

(I'll add a comment to the commit message to explain why a temporary string has to be used, in case I forget and start wondering a year from now...)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

2015-11-13 08:50 UTC #7 lore raw

In reply to Lev Stipakov

ACK, and thanks for your patience.

Your patch has been applied to the master and release/2.3 branch.

I have reformatted and updated the commit message a bit for readability, and added a NOTE: about the temporary variable.

commit efeaf947c9c5c88d77d16ac4917c1350c447c8dc (master) commit e2afbdb61392e750cfc54cc2ac9d08e5b6db4794 (release/2.3)

Author: Lev Stipakov
Date:   Wed Nov 11 13:48:07 2015 +0200

     Use adapter index instead of name for windows IPv6 interface config

     Signed-off-by: Olli Mannisto <[email protected]...>
     Signed-off-by: Lev Stipakov <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10484
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Lev Stipakov

author

2015-12-11 19:41 UTC #8 lore raw

In reply to Gert Doering

Hi,

Quoted 9 lines

Quoted 3 lines
So, if we want to use index also for "add/del route", I'd gently modify add/del_route_ipv6 and make it use "interface=<idx>" (without breaking "vpn server special route" case).
For consistency, I think we should do that. What I'd avoid is to do the adapter_index lookup for every single route - which would imply adding the index to struct tuntap_options or something like that which is available in add/del_route[_ipv6]() already. But this is less elegant than I hoped for, and might have to look different for 2.3 and master.

Adding the index - you probably meant to "struct tuntap"?

Master has member "adapter_index", which seems to be used for "special case", release/2.3 does not have that.

Do you think it is safe to store adapter index there (would have to add it to 2.3) without breaking "special case"? I personally not so sure, since that "adapter_index" is obtained via "adapter_index_of_ip" in tun.c, which looks quite different from "get_adapter_index_flexible".

We might have a semantic problem here.

-Lev

Gert Doering

2015-12-11 20:07 UTC #9 lore raw

Hi,

Quoted 6 lines · Lev Stipakov

Hi,
Quoted 4 lines
Quoted 3 lines
So, if we want to use index also for "add/del route", I'd gently modify add/del_route_ipv6 and make it use "interface=<idx>" (without breaking "vpn server special route" case).

Quoted 7 lines

Quoted 5 lines
For consistency, I think we should do that. What I'd avoid is to do the adapter_index lookup for every single route - which would imply adding the index to struct tuntap_options or something like that which is available in add/del_route[_ipv6]() already. But this is less elegant than I hoped for, and might have to look different for 2.3 and master.
Adding the index - you probably meant to "struct tuntap"?

Sort of. Struct tuntap has a sub-struct "struct tuntap_options options" which is mostly empty for all platforms but WIN32, and on WIN32 contains stuff like

  /* --ip-win32 options */
  bool ip_win32_defined;
  int ip_win32_type;

already (tun.h). So adding "tuntap_adapter_index" or something like this here would be fully in line with the existing style of packing windows specific bits nicely away, but having them available where needed...

route_add() gets passed a "struct tuntap *tt", so the option would be in tt->options.tuntap_adapter_index...

Quoted 2 lines

Master has member "adapter_index", which seems to be used for "special case", release/2.3 does not have that.

Now that's an interesting finding. I never noticed that, but it basically *is* what I propose above - not in tuntap_options but in the "main" struct tuntap - and I had no idea that this one existed (I tend to not look very closely at #ifdef WIN32 bits unless I have to).

It *is* in 2.3, btw :-) - as per "git blame tun.h" it was introduced into James' svn repo before we even had a git repo...

Quoted 6 lines

Do you think it is safe to store adapter index there (would have to add it to 2.3) without breaking "special case"? I personally not so sure, since that "adapter_index" is obtained via "adapter_index_of_ip" in tun.c, which looks quite different from "get_adapter_index_flexible".
We might have a semantic problem here.

adapter_index_of_ip() seems to be a "find connect routes" thing, not tied to the tuntap adapter - but that doesn't seem to be connected to tt->adapter_index.

Don't confuse with "rgi->adapter_index" - *that* is for routing information, and might point elsewhere.

tt->adapter_index seems to be set only in two places, namely

tun.c: tt->adapter_index = TUN_ADAPTER_INDEX_INVALID;

and

tun.c: tt->adapter_index = get_adapter_index (device_guid);
(in open_tun())

... and that one seems to be sort of like "get_adapter_index_flexible() lite" - the latter can search for guid *or name*, while the former only searches by guid.

So... I think we can just *use* that in add/del_route*(), and be happy everafter :-) - it's not available when we do "netsh... set address", unfortunately, as open_tun() happens *after* do_ifconfig() on windows...

Definitely worth a try.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Lev Stipakov

author

2015-12-11 20:29 UTC #10 lore raw

Thanks, I think (I think!) I got it now.

1) Since we have tt->adapter_index (which temporarily disappeared from my perception of reality), no need to add new member to tuntap or tuntap_options.

2) tt->adapter_index has nothing to do with rgi->adapter_index, first one is windows adapter index (which we pass to netsh), last one is for routing (which we pass to netsh too in special case).

3) In add/del_route* we (try to) use tt->adapter_index.

Regarding master's "special case" code. Should it be something like this:

  if ( r6->adapter_index )		/* vpn server special route */
    {
      struct buffer out = alloc_buf_gc (64, &gc);
      buf_printf (&out, "interface=%d", r6->adapter_index );
      device = buf_bptr(&out);
      gateway_needed = true;
    }
  else
    {
      /* device = interface=tt->adapter_index */
    }

-Lev

Gert Doering

2015-12-13 20:41 UTC #11 lore raw

Hi,

Quoted 11 lines · Lev Stipakov

Thanks, I think (I think!) I got it now.
1) Since we have tt->adapter_index (which temporarily disappeared from my perception of reality), no need to add new member to tuntap or tuntap_options.
2) tt->adapter_index has nothing to do with rgi->adapter_index, first one is windows adapter index (which we pass to netsh), last one is for routing (which we pass to netsh too in special case).
3) In add/del_route* we (try to) use tt->adapter_index.

Right :-)

Quoted 13 lines

Regarding master's "special case" code. Should it be something like this:

   if ( r6->adapter_index )		/* vpn server special route */
     {
       struct buffer out = alloc_buf_gc (64, &gc);
       buf_printf (&out, "interface=%d", r6->adapter_index );
       device = buf_bptr(&out);
       gateway_needed = true;
     }
   else
     {
       /* device = interface=tt->adapter_index */
     }

Already committed ;-) - but yes, exactly like this.

So if we need to have route that points elsewhere (/128 to existing default gateway to enable overlapping v6-in-v6), r6->adapter_index is set. If not, the tap index is used.

... and it works (except that my win7 vm started to really misbehave regarding IPv6 today, so my tests took much longer than planned... grrr)

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...