patch for bug #93: up-restart env vars

here's my patch for bug #93: missing ifconfig_* env vars after up-restart. Tested with both IPv4, IPv6, topology subnet and topology net30

Hi,

Quoted 2 lines · Jan Just Keijser

here's my patch for bug #93: missing ifconfig_* env vars after up-restart. Tested with both IPv4, IPv6, topology subnet and topology net30

Reading through #93, I'm wondering - what about the other environment variables mentioned, like "route_gateway" etc.?

In other words, your patch looks like it will nicely fix the missing ifconfig_* variables (by ensuring that what close_instance() destroyed is re-initialized even if not calling init_tun() - and cleaning up that one a bit while at it :-) ), but I wonder if more needs to be done?

Quoted 11 lines · Gert Doering

Quoted 3 lines · Jan Just Keijser

here's my patch for bug #93: missing ifconfig_* env vars after up-restart. Tested with both IPv4, IPv6, topology subnet and topology net30

Reading through #93, I'm wondering - what about the other environment variables mentioned, like "route_gateway" etc.?

In other words, your patch looks like it will nicely fix the missing ifconfig_* variables (by ensuring that what close_instance() destroyed is re-initialized even if not calling init_tun() - and cleaning up that one a bit while at it :-) ), but I wonder if more needs to be done?

I did look into that - a 'restart' occurs only if 'persist-tun' is used and in that case the routes are not re-added . If you look at the flow of code of 'do_open_tun' in init.c you'll see that all routing stuff is *not* re-run, hence those environment variables are not present anymore
- but then again, the routes are still there, as the system routing tables were not affected because the tun device was never closed in the first place.

My guestimation was that there are no other *important* env vars that get lost during a soft restart.

Quoted 12 lines · Jan Just Keijser

Quoted 11 lines · Gert Doering

Quoted 3 lines · Jan Just Keijser

here's my patch for bug #93: missing ifconfig_* env vars after up-restart. Tested with both IPv4, IPv6, topology subnet and topology net30

Reading through #93, I'm wondering - what about the other environment variables mentioned, like "route_gateway" etc.?

In other words, your patch looks like it will nicely fix the missing ifconfig_* variables (by ensuring that what close_instance() destroyed is re-initialized even if not calling init_tun() - and cleaning up that one a bit while at it :-) ), but I wonder if more needs to be done?

Quoted 7 lines

I did look into that - a 'restart' occurs only if 'persist-tun' is used and in that case the routes are not re-added . If you look at the flow of code of 'do_open_tun' in init.c you'll see that all routing stuff is *not* re-run, hence those environment variables are not present anymore
- but then again, the routes are still there, as the system routing tables were not affected because the tun device was never closed in the first place.

OK, so these get lost, but are set elsewhere, and would need an extra patch...

Quoted 2 lines

My guestimation was that there are no other *important* env vars that get lost during a soft restart.

Well, what's important to some is not to others (I could care less about "ifconfig_local", as tun.c does that for me :-) )... this ticket is all about consistency, *or* documenting that --up-restart will not see the environment variables that --up has "because you do not need them"...

I'm not sure what the best path forward is, TBH...

Quoted 1 line

I'm not sure what the best path forward is, TBH...

--up-restart *is* different from --up in several ways:

- runs as the user+group that is specified in the config file
- the 'tun' device is persistent so it's not "fresh"
- routes are not reset during a "persistent" restart, so are still present

Hi,

Quoted 8 lines · Jan Just Keijser

Quoted 2 lines

I'm not sure what the best path forward is, TBH...

--up-restart *is* different from --up in several ways:

- runs as the user+group that is specified in the config file
- the 'tun' device is persistent so it's not "fresh"
- routes are not reset during a "persistent" restart, so are still present

- IP addresses are not reset (as the tun device is pesistent)

so indeed, lots of differences here.

What about this:

- take your patch as it is (because it's not very intrusive, provides variables people are asking for, and cleans up tun.c a bit :-) )
- add a patch to openvpn.8 that explains the differences in --up-restart (so people will know that the lack of route env vars is not a bug)

Quoted 2 lines

- add a patch to openvpn.8 that explains the differences in --up-restart (so people will know that the lack of route env vars is not a bug)

agreed, but do we patch openvpn.8 before or after the documentation split ;) ?

See trac #93 and the discussion starting with <[email protected]...> on the openvpn-devel mailing list.

Signed-off-by Gert Doering <[email protected]...>

---
  doc/openvpn.8 | 6 ++++++
  1 file changed, 6 insertions(+)

doc/openvpn.8 +0 -0

index b9eee0d..ef87bb7 100644
@@ -1817,6 +1817,12 @@ has been specified, the up script will be called with
  .I restart
  as the last parameter.

"make check" self-test was broken after commit db950be85d37 due to do_ifconfig_setenv() not checking whether tt->did_ifconfig_setup was set (which isn't, for "dev null" type setups)

Signed-off-by Gert Doering <[email protected]...>

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

src/openvpn/tun.c +14 -11

index 9e853c0..e479d4d 100644
@@ -403,23 +403,27 @@ void
 do_ifconfig_setenv (const struct tuntap *tt, struct env_set *es)
 {
     struct gc_arena gc = gc_new ();
-    bool tun = is_tun_p2p (tt);
     const char *ifconfig_local = print_in_addr_t (tt->local, 0, &gc);
     const char *ifconfig_remote_netmask = print_in_addr_t (tt->remote_netmask, 0, &gc);
 
     /*
      * Set environmental variables with ifconfig parameters.
      */
-    setenv_str (es, "ifconfig_local", ifconfig_local);
-    if (tun)
+    if (tt->did_ifconfig_setup)
     {
-       setenv_str (es, "ifconfig_remote", ifconfig_remote_netmask);
-    }
-    else
-    {
-       const char *ifconfig_broadcast = print_in_addr_t (tt->broadcast, 0, &gc);
-       setenv_str (es, "ifconfig_netmask", ifconfig_remote_netmask);
-       setenv_str (es, "ifconfig_broadcast", ifconfig_broadcast);
+       bool tun = is_tun_p2p (tt);
+
+       setenv_str (es, "ifconfig_local", ifconfig_local);
+       if (tun)
+       {
+           setenv_str (es, "ifconfig_remote", ifconfig_remote_netmask);
+       }
+       else
+       {
+           const char *ifconfig_broadcast = print_in_addr_t (tt->broadcast, 0, &gc);
+           setenv_str (es, "ifconfig_netmask", ifconfig_remote_netmask);
+           setenv_str (es, "ifconfig_broadcast", ifconfig_broadcast);
+       }
     }
 
     if (tt->did_ifconfig_ipv6_setup)
--

2.3.6

"make check" self-test was broken after commit db950be85d37 due to do_ifconfig_setenv() not checking whether tt->did_ifconfig_setup was set (which isn't, for "dev null" type setups)