Hi, On Tue, Jun 12, 2018 at 03:42:40PM +0800, Antonio Quartulli wrote: > This change ensures that an interface is properly brought > up even when only IPv6 settings are configured/pushed.
And more review, this time for actual code :-)
> +do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,
> + const struct env_set *es)
...
> +#elif defined(TARGET_SOLARIS)
...
> + const char *ifconfig_ipv6_remote = print_in6_addr(tt->remote_ipv6, 0,
> + gc);
As discussed on IRC, I find the dangling "gc);" more ugly than the
old variant - so please keep the old formatting for now.
We might revisit this and rename all these long variables towards
"local_ip" and "remote_ip", or ditch all of tun.c in the sitnl-internal-API-
rewrite. We'll see. But not today, or this month.
> +#elif defined(TARGET_OPENBSD) || defined(TARGET_NETBSD) \
> + || defined(TARGET_DARWIN) || defined(TARGET_FREEBSD) \
> + || defined(TARGET_DRAGONFLY) || defined(TARGET_AIX)
I like the fact that all the BSDs can now be nicely joined into one
common block - but please keep TARGET_AIX separate.
While it uses the same "ifconfig" call as the other 5, joining the
block adds two new #ifdef clauses...
> +#if defined(TARGET_AIX)
> + /* AIX ifconfig will complain if it can't find ODM path in env */
> + es = env_set_create(NULL);
> + env_set_add(es, "ODMDIR=/etc/objrepos");
> #endif
... which, I think, warrant a dedicated #elif defined(TARGET_AIX) block
("contain the ugliness").
> +#if !defined(TARGET_FREEBSD) && !defined(TARGET_DRAGONFLY) \
> + && !defined(TARGET_AIX)
> + /* and, hooray, we explicitely need to add a route... */
> + add_route_connected_v6_net(tt, es);
> +#endif
And here, please use positive matching - way easier to see which platform
this applies to than going upwards for the encompassing #if TARGET_*
block and negating parts of them.
If counting right, this would become
> +#if defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) || \
> + defined(TARGET_DARWIN)
... easier to understand.
> +#elif defined (_WIN32)
[..]
> + {
> + /* example: netsh interface ipv6 set address interface=42
> + * 2001:608:8003::d store=active
> + */
I can see why you want to wrap the netsh example command, but I find this
not nice to read. What about:
+ /* example: netsh interface ipv6 set address interface=42
+ * 2001:608:8003::d store=active
+ */
so it's clear that this is a continuation line?
[..]
> +#else /* if defined(TARGET_LINUX) */
> + msg(M_FATAL, "Sorry, but I don't know how to do IPv6 'ifconfig' commands
> on this operating system. You should ifconfig your TUN/TAP device manually
> or use an --up script.");
> +#endif /* if defined(TARGET_LINUX) */
These two "defined(TARGET_LINUX)" are not your doing, but I think they
should be removed. Neither is the "#else" being "the other option to
TARGET_LINUX" nor is the #endif a simple "this is only TARGET_LINUX".
I'd either completely remove the comment or make it something like
#else /* platforms we have no IPv6 code for */
#endif /* outer #if defined(TARGET_xxx) conditional */
I think that's all I have to say about v3 so far :-)
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
