Hi, On Fri, Dec 09, 2016 at 03:50:48AM +0100, David Sommerseth wrote: > This adds a warning to the log file if --topology is configured to use > subnet or net30 and the 'subnet mask' argument of an --ifconfig-push option > is not an subnet mask. > > v2 - Make use of ifconfig_sanity_check() in tun.c instead of doing the exact > same check and warning in prepare_push_reply(). Also improve > documentation > of ifconfig_sanity_check() while at it.
Not being the one to complain about your code all the time... but...
since trac #755 is about "topology subnet"...
static void
ifconfig_sanity_check (bool tun, in_addr_t addr, int topology)
{
struct gc_arena gc = gc_new ();
const bool looks_like_netmask = ((addr & 0xFF000000) == 0xFF000000);
if (tun)
{
if (looks_like_netmask && (topology == TOP_NET30 || topology == TOP_P2P))
msg (M_WARN, "WARNING: Since you are using --dev tun with a point-to-poi
nt topology, the second argument to --ifconfig must be an IP address. You are u
sing something (%s) that looks more like a netmask. %s",
print_in_addr_t (addr, 0, &gc),
ifconfig_warn_how_to_silence);
}
else /* tap */
... ifconfig_sanity_check() does *nothing* for TOP_SUBNET (and the code
disagrees with your commit message for "net30").
So the v3 version of that patch would need to add the (missing today,
but actually important diagnostic aid - and the point of #755) clause
if (!looks_like_netmask && topology == TOP_SUBNET)
{
msg (M_WARN, "topology subnet needs a netmask as second argument bla
bla");
}
Also we might to re-think the warning message printed - if called from
an ifconfig-push context, the text "the second argument to --ifconfig must
be an IP address" is less than clear ("my --ifconfig settings are just
fine, what is openvpn warning about?").
Plus, it needs to be actually tested on a non-intel architecture - the
code has been like that forever, but I would not trust it to be endian-
safe (though our use of "in_addr_t" is not actually consistent with
what *should* be in one - parts of the code treat it as "network byte
order", other parts as "host byte order", and the point of using
"in_addr_t" as well defined *type* and not "a bag of 32bits, unsigned"
should have been "as defined by the socket API", which wants network
byte order).
But since this is an enhancement and not a bug, it should not be rushed -
it does not have to be in 2.4.0. Let's do it properly, test it, then
see if we want to have it in 2.4.1 (and while at it, we should teach
check_addr_clash() that networks do have a netmask for a reason, and
"local/remote and --ifconfig are in the same /24" could be perfectly fine
if the pushed netmask is *not* a /24).
gert
--
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025 g...@net.informatik.tu-muenchen.de
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
