Re: [libvirt] [PATCHv3] Support for static routes on a virtual bridge

On 04/11/2013 08:11 AM, Gene Czarcinski wrote: s/has a network/is directly reachable via a network/ Why is that? What types of "incorrect" gateway address lead to this condition, and exactly what is "the situation"? I don't think that should stop putting this patch in, but maybe if we understand the problem better we can do more to eliminate it in a followup patch. As we've discussed before, we should try to eliminate use of /sbin/ip in favor of netlink messages, but we're already using /sbin/ip for other things, so that can be done as a separate followup patch too. Put "this example" inside the <a>, and remove "here". s/host. However, such interfaces/host, but/ s/internal-only/guest-only/ s/with with regulat connectivity/that is directly reachable from the host/ s/is used provide/added to the "visible" network definition provides/ s/(and .*)// I think the last sentence is unnecessary, and not really true - it's just as essential when dealing with a hidden IPv4 network. Is there any reason you're putting in a separate </route> if there's no subelements? s/of the two definitions/of the two elements/ Really. though, I don't think that paragraph is needed. This was already covered in the Static Routes section above. I don't think you need to talk about it here. danpb recently pointed out that these examples need to be moved out into a separate document, but I've misplaced where that is. I guess we can leave this here until somebody reminds me (and hopefully someone else has the time to move it :-) Should we take this opportunity to retire "netmask"? We'll still have to keep it in <ip>, but at least we won't have to deal with it in <route>. Or should we leave it in in the name of keeping the two elements as similar as possible? (I guess even if we remove it, we'll still need to check for it so that we can generate an error, rather than just silently ignoring it...) Indentation is off here. Also, you shouldn't be re-using virNetworkIpDefPtr - create a new type called virNetworkRouteDefPtr (and associated helper functions). s/reall/really/ You need to save the return value and check for -2 - if the return is -2 that means there was a prefix, but it didn't parse correctly as a ULong. If it's not -2 but still < 0, then it just wasn't present, so you can set the default. (I know that's a copy-paste from virNetworkIpDefParseXML - that's a bug that should be fixed). Hmmm, yeah, I'm thinking it will be safest to accept netmask and just deal with it. If we ignore it then people will get invalid results, and if we specifically prohibit it, then we can't change our minds later and decide to support it. :-/ Address is required (unless prefix and netmask are both 0/missing, which indicates this is a default route). You need to return an error in that case. gateway is *always* required. If it's missing, you need to return an error. Really, the validation is the same when def->family is NULL and when it's "ipv4". Only the error messages will slightly change. Can you combine this clause with the next? Since gateway is required, you don't need to see if it's present. The first part of that is redundant - def->prefix > 32 is enough. Yeah, this clause - it should be the same as the one above, except that what's printed in the case of an error is slightly different. (I won't point out the similar nits in the clause, as I assume it will be combined with the above) Doesn't prefix='0' imply that this is a default route? And why do you not allow it to be > 64? You should allow anything up to 128 (for a single-host route) You shouldn't need to check for presence of gateway - it's required and should have been checked for already. Not exactly true - It's okay to have a netmask of 0.0.0.0 and no address (since 0.0.0.0 means "ignore *all* bits of the address). Usually someone who wanted that effect would simply not specify netmask or prefix, but they *could*. Not true - address could also be unspecified (if netmask was 0.0.0.0). You may actually want to do this parse further up; there is a helper function to convert from netmask to prefix - virSocketAddrGetNumNetmaskBits() - which would make checking for (netmask == "0.0.0.0") simpler. I think you can simplify this. You know that the gateway of every route is always required, so it will never be AF_UNSPEC, and you know that the address of every <ip> is also required, so it will never be AF_UNSPEC. So, you can just say if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { continue; } If no explicit prefix was given, you haven't set testAddr or testGw, i.e. you havne't accounted for someone relying on the "natural" netmask. Instead of the two clauses above, you should use this: int prefix = virNetworkIpDefPrefix(def2); virSocketAddrMask(&def2->address, prefix, &testAddr); virSocketAddrMask(&gwdef->gateway, prefix, &testGw); "unreachable static route gateway '%s' specified for network '%s'" You should replace the above two lines with: virBufferAddLit(buf, "/>\n"); Since there are never any subelements, there is no reason for a separate closing token. Don't do that - it always ends up leading to headaches/confusion later. Just create a new struct called virNetworkRoutDef that has only the fields needed for a <route> (family, address, prefix, netmask, and gateway). Since you will be changing virNetworkIpDefPtr to virNetworkRouteDefPtr, you will no longer be able to call virNetworkIpDefPrefix() as it's currently defined. To fix this without duplicating code, I think you need to change virNetworkIpDefPrefix into something like this, and put it in virsocketaddr.c: int virSocketAddrGetIpPrefix(const virSocketAddr *addr, const virSocketAddr *netmask, int prefix) { if (prefix > 0) { return prefix; } else if (VIR_SOCKET_ADDR_VALID(netmask)) { return virSocketAddrGetNumNetmaskBits(netmask); } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) { /* Return the natural prefix for the network's ip address. * On Linux we could use the IN_CLASSx() macros, but those * aren't guaranteed on all platforms, so we just deal with * the bits ourselves. */ unsigned char octet = ntohl(address->data.inet4.sin_addr.s_addr) >> 24; if ((octet & 0x80) == 0) { /* Class A network */ return 8; } else if ((octet & 0xC0) == 0x80) { /* Class B network */ return 16; } else if ((octet & 0xE0) == 0xC0) { /* Class C network */ return 24; } return -1; } else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) { return 64; } return -1; } (pretty much all I did was remove "&def->" from the original function) Then change all the original function's callers appropriately (or if you want, just have virNetworkIpDefPrefix() call virSocketAddrGetIpPrefix()). You don't need this outer if - the for loop takes care of it for you. In both cases use virSocketAddrPtr instead of virSocketAddr *. The previous 4 lines could be done with: virCommandAddArgList(cmd, "via", gatewaystr, "dev", "proto", "static", "metric", NULL); Use virSocketAddrPtr instead of virSocketAddr * here too. Why are you guaranteeing that arg1 and arg2 are NONNULL, but not arg4? You'll want to change all of these once you remove the extra </route> from the formatting function.