pre-ipv6-only clean up patchset
v2 cover pre-ipv6-only clean up patchset
This patchset is the result of some of Gert's comment on the IPv6-only patchset.
For this reason, this patchset is expected to be applied *before* applying future versions (v3...) of the IPv6-only patchset.
This patchset is mostly about restyling the code a bit and getting rid of unused members. Hence, there is no new feature being introduced.
Patch 1 is fixing potential memory leaks resulting from not releasing some argv local variables and removes unused gc_areas.
This version 2 is basically addressing all the coments raised by Gert on the previous version.
Detailed changes are rpeorted in each patch.
Cheers,
p.s. buildbot is currently unusable due to the ongoing maintenance on the OpenVPN servers and therefor eonly local tests have been performed.
Antonio Quartulli (3):
tun: ensure gc and argv are properly handled
tun: always pass a valid tt pointer
tun: get rid of tt->did_ifconfig member
src/openvpn/init.c | 7 +- src/openvpn/tun.c | 513 ++++++++++++++++++++++----------------------- src/openvpn/tun.h | 1 - 3 files changed, 254 insertions(+), 267 deletions(-)
Signature · 1 line
2.17.1
PATCH v2 1/3 tun: ensure gc and argv are properly handled
From: Antonio Quartulli <[email protected]...>
Everytime a argv object is initialized with argv_new(), it has to be released with argv_reset() once not needed anymore.
Ensure this kind of objects are always properly released to avoid memory leaks.
At the same time, remove those gc_arena objects that are initialized but never used/released.
---
Changes from v1:
- remove gc_arena at all when not used
src/openvpn/tun.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
src/openvpn/tun.c +5 -3
@@ -2600,7 +2600,6 @@ close_tun(struct tuntap *tt) }else if (tt) { -struct gc_arena gc =gc_new ();struct argv argv =argv_new ();/* setup command, close tun dev (clears tt->actual_name!), run command
@@ -2615,6 +2614,7 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"OpenBSD 'destroy tun interface' failed (non-critical)" );free (tt); +argv_reset (&argv); } }
@@ -2685,7 +2685,6 @@ close_tun(struct tuntap *tt) }else if (tt) { -struct gc_arena gc =gc_new ();struct argv argv =argv_new ();/* setup command, close tun dev (clears tt->actual_name!), run command
@@ -2700,6 +2699,7 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"NetBSD 'destroy tun interface' failed (non-critical)" );free (tt); +argv_reset (&argv); } }
@@ -2837,6 +2837,7 @@ close_tun(struct tuntap *tt)openvpn_execve_check (&argv,NULL ,0 ,"FreeBSD 'destroy tun interface' failed (non-critical)" );free (tt); +argv_reset (&argv); } }
@@ -3308,6 +3309,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tunenv_set_add ( es,"ODMDIR=/etc/objrepos" );openvpn_execve_check (&argv, es,S_FATAL ,"AIX 'create tun interface' failed" );env_set_destroy (es); +argv_reset (&argv); }else {
@@ -3333,7 +3335,6 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tunvoid close_tun (struct tuntap *tt) { -struct gc_arena gc =gc_new ();struct argv argv =argv_new ();struct env_set *es =env_set_create (NULL );
@@ -3362,6 +3363,7 @@ close_tun(struct tuntap *tt)free (tt);env_set_destroy (es); +argv_reset (&argv); }int
Signature · 1 line
2.17.1
[Openvpn-devel] [PATCH applied] Re: tun: ensure gc and argv are properly handled
"Proper code hygiene"... thanks. "Stared at code" plus run on a few platforms, including AIX ("because I can!").
Your patch has been applied to the master branch.
commit 18225f0fd5c45164308d4d66db055a1ad3960887
Author: Antonio Quartulli Date: Wed Jun 13 20:28:22 2018 +0800
tun: ensure gc and argv are properly handled
Signed-off-by: Antonio Quartulli <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
PATCH v2 2/3 tun: always pass a valid tt pointer
In reply to Antonio QuartulliFrom: Antonio Quartulli <[email protected]...>
This patch is a small "logic restyle" which basically moves the check for "tt != NULL" outside of the various close_tun() implementations and replaces it with an ASSERT.
This way the check is done only once and the function can rely on the assumption that "tt" is always valid.
This change is mainly to improve the code style inside close_tun() implementations by removing one level of indentation.
No functional change is present.
---
Changes from v1:
- remove ASSERT from solaris_error_close()
- remove "if (tt)" block from solaris_close_tun()
- restored "/* close and destroy */" comment in FreeBSD close_tun()
src/openvpn/init.c | 7 +- src/openvpn/tun.c | 501 ++++++++++++++++++++++----------------------- 2 files changed, 252 insertions(+), 256 deletions(-)
src/openvpn/init.c +5 -2
@@ -1847,8 +1847,11 @@ static void do_close_tun_simple(struct context *c) {msg (D_CLOSE ,"Closing TUN/TAP interface" ); -close_tun (c->c1.tuntap); - c->c1.tuntap =NULL ; +if (c->c1.tuntap) + { +close_tun (c->c1.tuntap); + c->c1.tuntap =NULL ; + } c->c1.tuntap_owned = false;#if P2MP CLEAR (c->c1.pulled_options_digest_save);
src/openvpn/tun.c +247 -254
668 lines not on this page · raw message
Signature · 1 line
2.17.1
[Openvpn-devel] [PATCH applied] Re: tun: always pass a valid tt pointer
Makes the code more readable, doesn't change anything (check with "git show -w", otherwise all the whitespace change is non-helpful).
Note: there was a rebase fail in the "NetBSD" close_tun() function, bringing back the gc_arena that v2 1/3 just dropped. I've taken the liberty to remove it again - it did not want to be there.
Your patch has been applied to the master branch.
commit 0bea472c98dca8b3470ef863fd90a63b6c79fa68
Author: Antonio Quartulli Date: Wed Jun 13 20:28:23 2018 +0800
tun: always pass a valid tt pointer
Signed-off-by: Antonio Quartulli <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
PATCH v2 3/3 tun: get rid of tt->did_ifconfig member
In reply to Antonio QuartulliFrom: Antonio Quartulli <[email protected]...>
tt->did_ifconfig is currently only read by the Linux platform, but it is currently uselessly set also by every other system.
The Linux platform does not actually even need this member and can directly rely on tt->did_ifconfig_setup.
For the reasons above, remove the tt->did_ifconfig at all and use tt->did_ifconfig_setup where needed (close_tun() on Linux).
---
Changes from v1:
- fix typ0 in commit message
src/openvpn/tun.c | 12 +----------- src/openvpn/tun.h | 1 - 2 files changed, 1 insertion(+), 12 deletions(-)
src/openvpn/tun.c +1 -11
@@ -987,7 +987,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv); openvpn_execve_check(&argv, es, S_FATAL, "Linux ip -6 addr add failed"); } - tt->did_ifconfig = true;#else /* ifdef ENABLE_IPROUTE */ if (tun) {
@@ -1026,7 +1025,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv); openvpn_execve_check(&argv, es, S_FATAL, "Linux ifconfig inet6 failed"); } - tt->did_ifconfig = true;#endif /*ENABLE_IPROUTE*/ #elif defined(TARGET_ANDROID)
@@ -1186,8 +1184,6 @@ do_ifconfig(struct tuntap *tt, add_route(&r, tt, 0, NULL, es); } - tt->did_ifconfig = true; -#elif defined(TARGET_OPENBSD)in_addr_t remote_end;/* for "virtual" subnet topology */
@@ -1265,7 +1261,6 @@ do_ifconfig(struct tuntap *tt,/* and, hooray, we explicitely need to add a route... */ add_route_connected_v6_net(tt, es); } - tt->did_ifconfig = true;#elif defined(TARGET_NETBSD)
@@ -1327,7 +1322,6 @@ do_ifconfig(struct tuntap *tt,/* and, hooray, we explicitely need to add a route... */ add_route_connected_v6_net(tt, es); } - tt->did_ifconfig = true;#elif defined(TARGET_DARWIN)/*
@@ -1384,7 +1378,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, es,S_FATAL ,"Mac OS X ifconfig failed" ); - tt->did_ifconfig = true;/* Add a network route for the local tun interface */ if (!tun && tt->topology ==TOP_SUBNET )
@@ -1457,7 +1450,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, es,S_FATAL ,"FreeBSD ifconfig failed" ); - tt->did_ifconfig = true;/* Add a network route for the local tun interface */ if (!tun && tt->topology ==TOP_SUBNET )
@@ -1507,7 +1499,6 @@ do_ifconfig(struct tuntap *tt,argv_msg (M_INFO , &argv);openvpn_execve_check (&argv, aix_es,S_FATAL ,"AIX ifconfig failed" ); - tt->did_ifconfig = true;if (do_ipv6) {
@@ -1545,7 +1536,6 @@ do_ifconfig(struct tuntap *tt,break ; } - tt->did_ifconfig = true; }if (do_ipv6)
@@ -2104,7 +2094,7 @@ close_tun(struct tuntap *tt) {ASSERT (tt); -if (tt->type !=DEV_TYPE_NULL && tt->did_ifconfig) +if (tt->type !=DEV_TYPE_NULL && tt->did_ifconfig_setup) {struct argv argv =argv_new ();struct gc_arena gc =gc_new ();
src/openvpn/tun.h +0 -1
@@ -138,7 +138,6 @@ struct tuntapbool did_ifconfig_setup;bool did_ifconfig_ipv6_setup; -bool did_ifconfig;bool persistent_if;/* if existed before, keep on program end */
Signature · 1 line
2.17.1
[Openvpn-devel] [PATCH applied] Re: tun: get rid of tt->did_ifconfig member
"Nicely getting rid of dead code". Tested on Linux (which is the only platform really affected).
Your patch has been applied to the master branch.
commit 78c58cfb02c5ca543cc7e0962337003601483e39
Author: Antonio Quartulli Date: Wed Jun 13 20:28:24 2018 +0800
tun: get rid of tt->did_ifconfig member
Signed-off-by: Antonio Quartulli <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering