HI, On Tue, Jun 12, 2018 at 03:42:40PM +0800, Antonio Quartulli wrote: > From: Antonio Quartulli <anto...@openvpn.net> > > This change ensures that an interface is properly brought > up even when only IPv6 settings are configured/pushed. > > At the same time, the do_ifconfig() function has been rearranged by > splitting the logic into a v4 and a v6 specific part. Each part has > then been moved into an idependent helper that can be invoked as > needed. > > This makes the code easier to read and more "symmetric" with > respect to the two address families.
Thanks for the update with a somewhat more verbose commit message, and
including the "changes from v2" bit.
Most of the changes are good, so "close to ACK"...
Staring at the resulting code, I saw something I hadn't noticed in v1/v2 -
"MTU handling". For example, on Linux with IPROUTE, we now call this
twice, once for IPv4 and once for IPv6
/* set the MTU for the device and bring it up */
argv_printf(&argv, "%s link set dev %s up mtu %d", iproute_path, ifname,
tun_mtu);
argv_msg(M_INFO, &argv);
openvpn_execve_check(&argv, es, S_FATAL, "Linux ip link set failed");
not sure if this has adverse effects, but it looks a tad silly in the
logs... other platforms just set the MTU in the actual "ifconfig" call
(if at all) so it's no new calls.
Another thing I noticed is that we really love to set tt->did_ifconfig
(in every single platform branch in do_ifconfig_ipv6()) - maybe that
one should be *renamed* to did_ifconfig_ipv4, and set only once...
... and of course close_tun() (all of them) needs to be adapted for the
ipv6-only case. It currently does cleanup at program end, and that depends
on "tt->did_ifconfig", which is only set of there is IPv4...
Looking more closely at Linux' close_tun(), I think tt->did_ifconfig can
just go, and be replaced by
if (tt->did_ifconfig_ipv4_setup)
{ cleanup ipv4 }
if (tt->did_ifconfig_ipv6_setup)
{ cleanup ipv6 }
(in each close_tun() function, for all platforms that have cleanups)
The idea behind tt->did_ifconfig seems to have been "only cleanup for
those platforms that actually have an ifconfig strategy" - but since
nobody except linux close_tun() ever *looks* at tt->did_ifconfig, it's
just leftover bloat.
While at it, we might want to replace the big "if(tt) { ... }" wrapper
around most of the close_tun() functions with a quick exit...
close_tun(...)
{
if ( !tt )
{
return;
}
Or if we are daring, with an ASSERT(tt)... not sure if close_tun() can
ever be called without a valid tuntap pointer... - well, the normal
caller of close_tun() is "do_close_tun_simple()" in init.c, so we could
just move it as "if(c->c1.tuntap) { close_tun(c->c1.tuntap); }" there...
(there are also tuncfg() and solaris_error_close(), which both have
a valid tt, always).
Reading through this, I think it would make sense to have a "0/8" pre-patch
that gets rid of tt->did_ifconfig and gets rid of the if(tt) indentation
in close_tun(), and then rebase 1/8 on top of that, and add ipv4/ipv6
differenciation to close_tun() where needed.
Anyway, at least the interface address removal bits need a v4 of this
patch...
> Cc: Gert Doering <g...@greenie.muc.de>
And please no cc:' to me in commit messages - it slightly annoys me to
receive mails twice with different headers, and serves no useful purpose.
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
