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,
    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...

    if ( !tt )

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 

> 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.

"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

Attachment: 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

Reply via email to