-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Please disregard this mail.  I will resent it again without getting it
destroyed by Engimail's text wrapping.

David S.



On 13/10/16 17:52, David Sommerseth wrote:
> On 12/10/16 11:13, Arne Schwabe wrote:
>> This option was useful when Ipv6 tun support was non standard
>> and was an internal/user specified flag that tracked the Ipv6 
>> capability of the tun device.
> 
>> All supported OS support IPv6. Also tun-ipv6 is pushable by the 
>> remote so not putting tun-ipv6 does not forbid ipv6 addresses.
> 
>> This commit also clean up a bit of the ipv6 related tun.c.
>> Changes for most platforms are minimal.
> 
>> For linux a bit more cleanup is done: - Remove compatibility 
>> defines that were added 2008 - Always use IFF_NO_PI for the
>> linux tun and not only for IPv4 only tun setups (Android also
>> always IFF_NO_PI works fine with Ipv6).
> 
>> This commit also remove a non ipv6 fallback for tap driver from 
>> OpenVPN 2.2-beta or earlier and only warns.
> 
>> Patch V2: Integrate Gert's comments Patch V3: Remove tun_ipv4 
>> option. It only used for MTU discovery and there it was wrong
>> since it should on the transport protocol if at all Patch V4:
>> Completely remove support for NetBSD <= 4.0 and remove
>> NETBSD_MULTI_AF defines --- Changes.rst           |   3 ++
>> src/openvpn/forward.c | 2 +- src/openvpn/helper.c  |   2 -
>> src/openvpn/init.c    |   6 --- src/openvpn/multi.c   |   8 ++-
>> src/openvpn/openvpn.h |   5 -- src/openvpn/options.c |  11 +---
>> src/openvpn/options.h |   1 - src/openvpn/route.c   |  13 ++---
>> src/openvpn/tun.c     | 139 
>> +++++++------------------------------------------- 
>> src/openvpn/tun.h     |   2 - 11 files changed, 30
>> insertions(+), 162 deletions(-)
> 
>> diff --git a/Changes.rst b/Changes.rst index 9fcba75..2956003 
>> 100644 --- a/Changes.rst +++ b/Changes.rst @@ -135,6 +135,9 @@ 
>> User-visible Changes ciphers configured in the config file.  Use 
>> --ncp-disable if you don't want that.
> 
>> +- ALl tun devices on all platforms are considered always IPv6 
>> capable. The --tun-ipv6
> 
> Silly typo.  (I would have done this one in-flight, hadn't it been
> for a few other minor things).
> 
> [...snip...]
>> @@ -4577,7 +4569,6 @@ add_option (struct options *options, else
>> if (streq (p[0], "tun-ipv6") && !p[1]) { VERIFY_PERMISSION 
>> (OPT_P_UP); -      options->tun_ipv6 = true; }
> 
> Should we add a "Deprecated/NO-OP option used." message?
> 
> [...snip....]
> 
>> +++ b/src/openvpn/route.c @@ -1729,10 +1729,10 @@ add_route_ipv6 
>> (struct route_ipv6 *r6, const struct tuntap *tt, unsigned int fla
>>  } #endif
> 
>> -  if ( !tt->ipv6 ) +  if (!tt->did_ifconfig_ipv6_setup) { - msg(
>> M_INFO, "add_route_ipv6(): not adding %s/%d, no IPv6 on if %s", -
>> network, r6->netbits, device ); +      msg( M_INFO, 
>> "add_route_ipv6(): not adding %s/%d, no IPv6 ifconfig on if %s",
>> + network, r6->netbits, device);
> 
> Wouldn't it be nicer we said "no IPv6 address configured on 
> interface %s" ?  Just trying to be slightly more user friendly in
> the logs.
> 
>> --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -741,8 +741,8
>> @@ do_ifconfig (struct tuntap *tt,
> 
>> argv_init (&argv);
> 
>> -      msg( M_INFO, "do_ifconfig, tt->ipv6=%d, 
>> tt->did_ifconfig_ipv6_setup=%d", -              tt->ipv6, 
>> tt->did_ifconfig_ipv6_setup ); +      msg( M_INFO, "do_ifconfig, 
>> tt->did_ifconfig_ipv6_setup=%d", + tt->did_ifconfig_ipv6_setup
>> );
> 
> While we're changing log lines ... could we make this one a bit
> less technical, or at least increase the verb level if we want it
> to be so technical?  Again, just to be more user friendly
> 
> [...snip...]
> 
>> @@ -1526,7 +1513,7 @@ open_tun_generic (const char *dev, const
>> char *dev_type, const char *dev_node, bool dynamic_opened =
>> false;
> 
> 
>> -  if ( tt->ipv6 && ! ipv6_explicitly_supported ) +  if (  ! 
>> ipv6_explicitly_supported ) msg (M_WARN, "NOTE: explicit support 
>> for IPv6 tun devices is not provided for this OS");
> 
> Isn't this if() block a NOOP?
> 
> $ git grep -Hni ipv6_explicitly_supported src/openvpn/tun.c:1521:
> bool ipv6_explicitly_supported, bool dynamic, 
> src/openvpn/tun.c:1529:  if ( tt->ipv6 && !
> ipv6_explicitly_supported )
> 
> Could we kill this as well?
> 
> 
> [...snip...0
>> @@ -1977,53 +1940,13 @@ close_tun (struct tuntap *tt) int
>> write_tun (struct tuntap* tt, uint8_t *buf, int len) { -  if
>> (tt->ipv6) - { -      struct tun_pi pi; -      struct iphdr *iph;
>> -      struct iovec vect[2]; -      int ret; - -      iph =
>> (struct iphdr *)buf; - -      pi.flags = 0; - -
>> if(iph->version == 6) -      pi.proto = htons(OPENVPN_ETH_P_IPV6); -
>> else -       pi.proto = htons(OPENVPN_ETH_P_IPV4); - -
>> vect[0].iov_len = sizeof(pi); - vect[0].iov_base = &pi; -
>> vect[1].iov_len = len; - vect[1].iov_base = buf; - -      ret =
>> writev(tt->fd, vect, 2); - return(ret - sizeof(pi)); -    } -
>> else -    return write (tt->fd, buf, len); +  return write
>> (tt->fd, buf, len);
> 
> whitespace issue.
> 
>> }
> 
>> int read_tun (struct tuntap* tt, uint8_t *buf, int len) { -  if 
>> (tt->ipv6) -    { -      struct iovec vect[2]; -      struct
>> tun_pi pi; -      int ret; - -      vect[0].iov_len = sizeof(pi);
>> - vect[0].iov_base = &pi; -      vect[1].iov_len = len; - 
>> vect[1].iov_base = buf; - -      ret = readv(tt->fd, vect, 2); - 
>> return(ret - sizeof(pi)); -    } -  else -    return read
>> (tt->fd, buf, len); +  return read (tt->fd, buf, len);
> 
> whitespace issue.
> 
> [....snip....]
>> @@ -5308,11 +5208,10 @@ open_tun (const char *dev, const char 
>> *dev_type, const char *dev_node, struct tu /* usage of numeric 
>> constants is ugly, but this is really tied to * *this* version
>> of the driver */ -    if ( tt->ipv6 && tt->type == DEV_TYPE_TUN
>> && + if (tt->type == DEV_TYPE_TUN && info[0] == 9 && info[1] < 8)
>> { - msg( M_INFO, "WARNING:  Tap-Win32 driver version %d.%d does
>> not support IPv6 in TUN mode.  IPv6 will be disabled.  Upgrade
>> to Tap-Win32 9.8 (2.2-beta3 release or later) or use TAP mode to
>> get IPv6", (int) info[0], (int) info[1] ); - tt->ipv6 = false; +
>> msg( M_INFO, "WARNING:  Tap-Win32 driver version %d.%d does not
>> support IPv6 in TUN mode.  IPv6 will not work.  Upgrade to
>> Tap-Win32 9.8 (2.2-beta3 release or later) or use TAP mode to get
>> IPv6", (int) info[0], (int) info[1] );
> 
> Maybe we should review the contents of this log line as well?  Can
> at least point at the latest 2.3 release instead of a beta
> release.
> 
> 
> I see that a few of my comments are not directly related to
> removing --tun-ipv6.  But I'm not convinced there's any benefit in
> having them in a separate patch.  I'm okay with either approach.
> 
> 
> -- kind regards,
> 
> David Sommerseth
> 

- -- 
kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJX/64zAAoJEIbPlEyWcf3yOdgP/2xcR+dYM1VQLHEPhKCoFrZ1
+gl9py6z124K0jswkFjCbm4l1EfJyPmxDZt9GfvBr+j+3lT/yZoEPMNhQG7F6LM3
czYAx0vFuI51/8s6GSXfh/APoJTTQhOSprzEbkluPjeVa46fUZ8XeXeTHoJbaujD
ilF1XengPHp671yJVlEPPAL2rnNrMaNi7NMA3Fy1/K83I0fK8D7dxSVtcVrIWjIG
qlV68YbIxyYbxelM0AdLAcKsiCffZIe3Ojy+UoGjnLBKjnbbvkqMbzdHk7982Fp0
QNAwe+WwCw4Q5kl2W2DGPdZY88aU9v4EBWdEa3JF3QMwj1b1F0KY7E/9r3iHYDNe
cNBNwN4D2sjd1SdTCL1sAV2Iih+i1T8/gLYPNdXp8Zkf6HaI3nWtyCF8BEB7CeO6
zsmMBMeWc+ld6zjhaOL2JYRj94qkmuCgGtNa4khd34Ih5OAlk1E/GyrkCWdSCTAS
T7eD7V5AfGzBfx4Hu2XJVAEOpzjU6NKVtqHBCecni3PUk2Uyjj5v6Zcl1vzoiyye
AHSeYmazB5h24ywA5k0JukCLblaKP1bciStXPlGByAHtOHA7ZTPR7SUvsV1VIBOA
BMPJaEabUhYIvhK82bqA1DxUpBngPaNxFvesMkJTQgS7zE9gj1C3ATd6vuLMYHIY
E1q3yTuEYIDwaJUN0wr/
=+AHi
-----END 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

Reply via email to