Re: [libvirt] [PATCH-v4.2] Support for static routes on a virtual bridge

(I wanted a separate message to comment on this part...) On 04/26/2013 07:22 PM, Gene Czarcinski wrote: > +/* add an IP (static) route to a bridge */ > +static int > +networkAddRouteToBridge(virNetworkObjPtr network, > + virNetworkRouteDefPtr routedef) > +{ > + bool done = false; > + int prefix = 0; > + virSocketAddrPtr addr = &routedef->address; > + virSocketAddrPtr mask = &routedef->netmask; > + > + if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) { > + long val4 = ntohl(addr->data.inet4.sin_addr.s_addr); > + long msk4 = -1; > + if (VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET)) { > + msk4 = ntohl(mask->data.inet4.sin_addr.s_addr); > + } > + if (msk4 == -1) { > + if (val4 == 0 && routedef->prefix == 0) > + done = true; > + } else { > + if (val4 == 0 && msk4 == 0) > + done = true; > + } > + } I'll try and decode this... if ((address == 0.0.0.0) and ((((netmask is unspecified) and (prefix is (0 or unspecified))) or (netmask is 0.0.0.0))) then use 0 for prefix when adding the route Is that correct? First - I would like to avoid references to the internal data structures of a virSocketAddr, and calling ntohnl at this level. virSocketAddr should be able to handle any bit twiddling we need. Now, let's see how much of that we can get rid of: 1) If netmask is 0.0.0.0, virSocketAddrGetIpPrefix will anyway return virSocketAddrGetNumNetmaskBits(0.0.0.0), which is conveniently 0. 2) if neither netmask nor prefix is specified, virSocketAddrGetIpPrefix will return 0 anyway (regardless of address), *but only if address wasn't specified*. If an address *was* specified and it was 0.0.0.0, it returns 8 (treating it as a Class A network) I had actually intended that my modification to virSocketAddrGetIpPrefix() to return 0 would eliminate the need for such code in bridge_driver.c, but didn't do it quite right, and it's just as well, because I just checked and RFCs say that there *is* some valid use for 0.0.0.0/8. > + else if (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET6)) { > + int i, val6 = 0; > + for (i = 0;i < 4;i++) { > + val6 += ((addr->data.inet6.sin6_addr.s6_addr[2 * i] << 8) | > + addr->data.inet6.sin6_addr.s6_addr[2 * i + 1]); > + } > + if (val6 == 0 && routedef->prefix == 0) { > + char *addr = virSocketAddrFormat(&routedef->address); > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("bridge '%s' has prefix=0 for address='%s' which is not supported"), > + network->def->bridge, addr); > + VIR_FREE(addr); > + return -1; > + } > + } and here - if the address is 0 and the prefix is 0/unspecified, then log an error. But if this is really something that's always illegal according to the IPv6 RFCs, then we can/should do that validation in the parser, not here. > + > + if (done) { > + prefix = 0; > + } else { > + prefix = virSocketAddrGetIpPrefix(&routedef->address, > + &routedef->netmask, > + routedef->prefix); > + > + if (prefix < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("bridge '%s' has an invalid netmask or IP address for route definition"), > + network->def->bridge); > + return -1; > + } > + } > + > + if (virNetDevSetGateway(network->def->bridge, > + &routedef->address, > + prefix, > + &routedef->gateway) < 0) > + return -1; > + return 0; > +} > So here's my opinion: 1) remove all that code above (I did that in my interdiff to your patch) 2) Make a new patch that adds something like this: virSocketAddr zero; /* this creates an all-0 address of the appropriate family */ ignore_value(virSocketAddrParse(&zero, (VIR_SOCKET_ADDR_IS_FAMILY(addr,AF_INET) ? "0.0.0.0" : "::"), VIR_SOCKET_ADDR_FAMILY(addr)); if (routedef->prefix || VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET) || virSocketAddrEqual(addr, zero)) { prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); } else { /* neither specified. check for a match with an address of all 0's */ if (virSocketAddrEqual(addr, zero)) prefix = 0; else prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); } virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge '%s' has prefix=0 for address='%s' which is not supported"), network->def->bridge, addr); } } else { /* no prefix given, but address was non-zero, so get default prefix */ prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); } } if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge '%s' has an invalid netmask or IP address for route definition"), network->def->bridge); return -1; } if (virNetDevSetGateway(network->def->bridge, addr, prefix, &routedef->gateway) < 0) return -1; return 0; } 3) If an ipv6 route for "::/0" really is illegal, then check for that in the parser and disallow it there.