Skip to contentSkip to conversation

Make error non-fatal while deleting address using netsh

PATCH In review Selva Nair 4 participants 9 messages May 13, 2016 – May 16, 2016

lore
Share & tools
lore

Selva Nair

author
#0 lore raw

PATCH 1/1 Make error non-fatal while deleting address using netsh

Fixes Trac #71 (see comments 37 to 43)

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/tun.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
src/openvpn/tun.c +9 -9
index b70410d..11027dd 100644
@@ -62,7 +62,7 @@ static void netsh_ifconfig (const struct tuntap_options *to,
 			    const in_addr_t ip,
 			    const in_addr_t netmask,
 			    const unsigned int flags);
-static void netsh_command (const struct argv *a, int n);
+static void netsh_command (const struct argv *a, int n, int msglevel);
 
 static const char *netsh_get_id (const char *dev_node, struct gc_arena *gc);
 

@@ -1246,7 +1246,7 @@ do_ifconfig (struct tuntap *tt,
 		     NETSH_PATH_SUFFIX,
 		     win32_version_info() == WIN_XP ? actual : iface,
 		     ifconfig_ipv6_local);
-	netsh_command (&argv, 4);
+	netsh_command (&argv, 4, M_FATAL);
 
 	/* explicit route needed */
 	/* on windows, OpenVPN does ifconfig first, open_tun later, so

@@ -4246,7 +4246,7 @@ dhcp_renew (const struct tuntap *tt)
  */
 
 static void
-netsh_command (const struct argv *a, int n)
+netsh_command (const struct argv *a, int n, int msglevel)
 {
   int i;
   for (i = 0; i < n; ++i)

@@ -4261,7 +4261,7 @@ netsh_command (const struct argv *a, int n)
 	return;
       openvpn_sleep (4);
     }
-  msg (M_FATAL, "NETSH: command failed");
+  msg (msglevel, "NETSH: command failed");
 }
 
 void

@@ -4411,7 +4411,7 @@ netsh_ifconfig_options (const char *type,
 		   NETSH_PATH_SUFFIX,
 		   type,
 		   flex_name);
-      netsh_command (&argv, 2);
+      netsh_command (&argv, 2, M_FATAL);
     }
 
   /* add new DNS/WINS settings to TAP interface */

@@ -4432,7 +4432,7 @@ netsh_ifconfig_options (const char *type,
 			 type,
 			 flex_name,
 			 print_in_addr_t (addr_list[i], 0, &gc));
-	    netsh_command (&argv, 2);
+	    netsh_command (&argv, 2, M_FATAL);
 	  
 	    ++count;
 	  }

@@ -4507,7 +4507,7 @@ netsh_ifconfig (const struct tuntap_options *to,
 		       print_in_addr_t (ip, 0, &gc),
 		       print_in_addr_t (netmask, 0, &gc));
 
-	  netsh_command (&argv, 4);
+	  netsh_command (&argv, 4, M_FATAL);
 	}
     }
 

@@ -4553,7 +4553,7 @@ netsh_enable_dhcp (const struct tuntap_options *to,
 	       NETSH_PATH_SUFFIX,
 	       actual_name);
 
-  netsh_command (&argv, 4);
+  netsh_command (&argv, 4, M_FATAL);
 
   argv_reset (&argv);
 }

@@ -5269,7 +5269,7 @@ close_tun (struct tuntap *tt)
 		     tt->actual_name,
 		     ifconfig_ipv6_local );
 
-	  netsh_command (&argv, 1);
+	  netsh_command (&argv, 1, M_WARN);
           argv_reset (&argv);
 	}
 #if 1
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 1/1 Make error non-fatal while deleting address using netsh

Fixes Trac #71 (see comments 37 to 43)

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/tun.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
src/openvpn/tun.c +9 -9
index c87fb36..f79d067 100644
@@ -68,7 +68,7 @@ static void netsh_ifconfig (const struct tuntap_options *to,
 			    const in_addr_t ip,
 			    const in_addr_t netmask,
 			    const unsigned int flags);
-static void netsh_command (const struct argv *a, int n);
+static void netsh_command (const struct argv *a, int n, int msglevel);
 
 static const char *netsh_get_id (const char *dev_node, struct gc_arena *gc);
 

@@ -1398,7 +1398,7 @@ do_ifconfig (struct tuntap *tt,
 			 NETSH_PATH_SUFFIX,
 			 iface,
 			 ifconfig_ipv6_local );
-	    netsh_command (&argv, 4);
+	    netsh_command (&argv, 4, M_FATAL);
 	  }
 
 	/* explicit route needed */

@@ -4597,7 +4597,7 @@ dhcp_renew (const struct tuntap *tt)
  */
 
 static void
-netsh_command (const struct argv *a, int n)
+netsh_command (const struct argv *a, int n, int msglevel)
 {
   int i;
   for (i = 0; i < n; ++i)

@@ -4612,7 +4612,7 @@ netsh_command (const struct argv *a, int n)
 	return;
       openvpn_sleep (4);
     }
-  msg (M_FATAL, "NETSH: command failed");
+  msg (msglevel, "NETSH: command failed");
 }
 
 void

@@ -4762,7 +4762,7 @@ netsh_ifconfig_options (const char *type,
 		   NETSH_PATH_SUFFIX,
 		   type,
 		   flex_name);
-      netsh_command (&argv, 2);
+      netsh_command (&argv, 2, M_FATAL);
     }
 
   /* add new DNS/WINS settings to TAP interface */

@@ -4783,7 +4783,7 @@ netsh_ifconfig_options (const char *type,
 			 type,
 			 flex_name,
 			 print_in_addr_t (addr_list[i], 0, &gc));
-	    netsh_command (&argv, 2);
+	    netsh_command (&argv, 2, M_FATAL);
 	  
 	    ++count;
 	  }

@@ -4858,7 +4858,7 @@ netsh_ifconfig (const struct tuntap_options *to,
 		       print_in_addr_t (ip, 0, &gc),
 		       print_in_addr_t (netmask, 0, &gc));
 
-	  netsh_command (&argv, 4);
+	  netsh_command (&argv, 4, M_FATAL);
 	}
     }
 

@@ -4904,7 +4904,7 @@ netsh_enable_dhcp (const struct tuntap_options *to,
 	       NETSH_PATH_SUFFIX,
 	       actual_name);
 
-  netsh_command (&argv, 4);
+  netsh_command (&argv, 4, M_FATAL);
 
   argv_reset (&argv);
 }

@@ -5648,7 +5648,7 @@ close_tun (struct tuntap *tt)
                            tt->actual_name,
                            ifconfig_ipv6_local);
 
-              netsh_command (&argv, 1);
+              netsh_command (&argv, 1, M_WARN);
               argv_reset (&argv);
             }
 	}
Signature · 1 line
1.7.10.4
#2 lore raw
In reply to Selva Nair
Quoted 1 line · Selva Nair

Fixes Trac #71 (see comments 37 to 43)

Nit pick on the commit message. This message isn't really helpful. And it presumes that Trac will be available "forever". We should always ensure commit messages do not purely depend on external resources.

I can recommend this blog post on this topic:
<http://chris.beams.io/posts/git-commit/>

Signature · 3 lines
kind regards,

David Sommerseth
author
#4 lore raw
In reply to David Sommerseth

Hi,

Quoted 4 lines · David Sommerseth
Quoted 1 line · Selva Nair

Fixes Trac #71 (see comments 37 to 43)

Nit pick on the commit message.

Thanks for responding to the patch.

Quoted 2 lines

This message isn't really helpful. And
it presumes that Trac will be available "forever".

Good point. Will do a v2 with a more helpful "why the change"..

Selva

author
#5 lore raw

PATCH v2 1/1 Make error non-fatal while deleting address using netsh

In reply to Selva Nair

During windows power events such as sleep or suspend, the TUN/TAP I/O aborts and openvpn signals SIGHUP so as to automatically reconnect on resume (since commit ea66a2b5cdb2..). During the SIGHUP processing operations such as address and route deletion are expected to fail. Such failures should be treated as non-fatal to allow for this automatic recovery logic to work. Currently, when the address deletion is handled by netsh, errors are treated as M_FATAL. This patch changes the error level to M_WARN.

Resolves Trac #71 (comments 37 to 43)

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/tun.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
src/openvpn/tun.c +9 -9
index b70410d..11027dd 100644
@@ -62,7 +62,7 @@ static void netsh_ifconfig (const struct tuntap_options *to,
 			    const in_addr_t ip,
 			    const in_addr_t netmask,
 			    const unsigned int flags);
-static void netsh_command (const struct argv *a, int n);
+static void netsh_command (const struct argv *a, int n, int msglevel);
 
 static const char *netsh_get_id (const char *dev_node, struct gc_arena *gc);
 

@@ -1246,7 +1246,7 @@ do_ifconfig (struct tuntap *tt,
 		     NETSH_PATH_SUFFIX,
 		     win32_version_info() == WIN_XP ? actual : iface,
 		     ifconfig_ipv6_local);
-	netsh_command (&argv, 4);
+	netsh_command (&argv, 4, M_FATAL);
 
 	/* explicit route needed */
 	/* on windows, OpenVPN does ifconfig first, open_tun later, so

@@ -4246,7 +4246,7 @@ dhcp_renew (const struct tuntap *tt)
  */
 
 static void
-netsh_command (const struct argv *a, int n)
+netsh_command (const struct argv *a, int n, int msglevel)
 {
   int i;
   for (i = 0; i < n; ++i)

@@ -4261,7 +4261,7 @@ netsh_command (const struct argv *a, int n)
 	return;
       openvpn_sleep (4);
     }
-  msg (M_FATAL, "NETSH: command failed");
+  msg (msglevel, "NETSH: command failed");
 }
 
 void

@@ -4411,7 +4411,7 @@ netsh_ifconfig_options (const char *type,
 		   NETSH_PATH_SUFFIX,
 		   type,
 		   flex_name);
-      netsh_command (&argv, 2);
+      netsh_command (&argv, 2, M_FATAL);
     }
 
   /* add new DNS/WINS settings to TAP interface */

@@ -4432,7 +4432,7 @@ netsh_ifconfig_options (const char *type,
 			 type,
 			 flex_name,
 			 print_in_addr_t (addr_list[i], 0, &gc));
-	    netsh_command (&argv, 2);
+	    netsh_command (&argv, 2, M_FATAL);
 	  
 	    ++count;
 	  }

@@ -4507,7 +4507,7 @@ netsh_ifconfig (const struct tuntap_options *to,
 		       print_in_addr_t (ip, 0, &gc),
 		       print_in_addr_t (netmask, 0, &gc));
 
-	  netsh_command (&argv, 4);
+	  netsh_command (&argv, 4, M_FATAL);
 	}
     }
 

@@ -4553,7 +4553,7 @@ netsh_enable_dhcp (const struct tuntap_options *to,
 	       NETSH_PATH_SUFFIX,
 	       actual_name);
 
-  netsh_command (&argv, 4);
+  netsh_command (&argv, 4, M_FATAL);
 
   argv_reset (&argv);
 }

@@ -5269,7 +5269,7 @@ close_tun (struct tuntap *tt)
 		     tt->actual_name,
 		     ifconfig_ipv6_local );
 
-	  netsh_command (&argv, 1);
+	  netsh_command (&argv, 1, M_WARN);
           argv_reset (&argv);
 	}
 #if 1
Signature · 1 line
1.7.10.4
author
#6 lore raw

PATCH v2 1/1 Make error non-fatal while deleting address using netsh

During windows power events such as sleep or suspend, the TUN/TAP I/O aborts and openvpn signals SIGHUP so as to automatically reconnect on resume (since commit ea66a2b5cdb2..). During the SIGHUP processing operations such as address and route deletion are expected to fail. Such failures should be treated as non-fatal to allow for this automatic recovery logic to work. Currently, when the address deletion is handled by netsh, errors are treated as M_FATAL. This patch changes the error level to M_WARN.

Resolves Trac #71 (comments 37 to 43)

Signed-off-by Selva Nair <[email protected]...>
---
 src/openvpn/tun.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
src/openvpn/tun.c +9 -9
index c87fb36..f79d067 100644
@@ -68,7 +68,7 @@ static void netsh_ifconfig (const struct tuntap_options *to,
 			    const in_addr_t ip,
 			    const in_addr_t netmask,
 			    const unsigned int flags);
-static void netsh_command (const struct argv *a, int n);
+static void netsh_command (const struct argv *a, int n, int msglevel);
 
 static const char *netsh_get_id (const char *dev_node, struct gc_arena *gc);
 

@@ -1398,7 +1398,7 @@ do_ifconfig (struct tuntap *tt,
 			 NETSH_PATH_SUFFIX,
 			 iface,
 			 ifconfig_ipv6_local );
-	    netsh_command (&argv, 4);
+	    netsh_command (&argv, 4, M_FATAL);
 	  }
 
 	/* explicit route needed */

@@ -4597,7 +4597,7 @@ dhcp_renew (const struct tuntap *tt)
  */
 
 static void
-netsh_command (const struct argv *a, int n)
+netsh_command (const struct argv *a, int n, int msglevel)
 {
   int i;
   for (i = 0; i < n; ++i)

@@ -4612,7 +4612,7 @@ netsh_command (const struct argv *a, int n)
 	return;
       openvpn_sleep (4);
     }
-  msg (M_FATAL, "NETSH: command failed");
+  msg (msglevel, "NETSH: command failed");
 }
 
 void

@@ -4762,7 +4762,7 @@ netsh_ifconfig_options (const char *type,
 		   NETSH_PATH_SUFFIX,
 		   type,
 		   flex_name);
-      netsh_command (&argv, 2);
+      netsh_command (&argv, 2, M_FATAL);
     }
 
   /* add new DNS/WINS settings to TAP interface */

@@ -4783,7 +4783,7 @@ netsh_ifconfig_options (const char *type,
 			 type,
 			 flex_name,
 			 print_in_addr_t (addr_list[i], 0, &gc));
-	    netsh_command (&argv, 2);
+	    netsh_command (&argv, 2, M_FATAL);
 	  
 	    ++count;
 	  }

@@ -4858,7 +4858,7 @@ netsh_ifconfig (const struct tuntap_options *to,
 		       print_in_addr_t (ip, 0, &gc),
 		       print_in_addr_t (netmask, 0, &gc));
 
-	  netsh_command (&argv, 4);
+	  netsh_command (&argv, 4, M_FATAL);
 	}
     }
 

@@ -4904,7 +4904,7 @@ netsh_enable_dhcp (const struct tuntap_options *to,
 	       NETSH_PATH_SUFFIX,
 	       actual_name);
 
-  netsh_command (&argv, 4);
+  netsh_command (&argv, 4, M_FATAL);
 
   argv_reset (&argv);
 }

@@ -5648,7 +5648,7 @@ close_tun (struct tuntap *tt)
                            tt->actual_name,
                            ifconfig_ipv6_local);
 
-              netsh_command (&argv, 1);
+              netsh_command (&argv, 1, M_WARN);
               argv_reset (&argv);
             }
 	}
Signature · 1 line
1.7.10.4
#7 lore raw

ACK, thanks for taking this on.

I've stared at the patch and compiled on MinGW - did not actually test the suspend/resume/tap gone/address deletion fail situation (hard to reproduce with my current test setups), but the code looks sane and it compiles fine.

Your patch has been applied to the master branch.

commit e3420d5683ffcc4386f78485bae3288a48f5cc17 (master)

Author: Selva Nair
Date:   Fri May 13 20:31:24 2016 -0400
     Make error non-fatal while deleting address using netsh

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11656
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#8 lore raw
In reply to Selva Nair

ACK.

The patch is effectively exactly the same changes as for master, but the context is different enough (due to the XP-related stuff in 2.3) that I'm not sure if cherrypicking would work - so, just taking your 2.3 patch :-)

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

commit d368abbecf0a67ba5c29dec28fc724655629ef70 (release/2.3)

Author: Selva Nair
Date:   Fri May 13 20:31:23 2016 -0400
     Make error non-fatal while deleting address using netsh

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11655
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering