On 04/29/2013 11:55 AM, Laine Stump wrote:
(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.
First of all, I am back from my new home inspection trip. The submittal of this patch was a little rushed and suffered as a result. As you may have noticed, I sometimes (often?) over-engineer my solutions (belt, suspenders, elastic waistband, glue-on pants and make sure to check them every time you stand up). I am in complete agreement with you suggested changes for network_conf.* and appreciate your patch since you did all of the work. My plan is to roll all of the changes into a single patch which will be resubmitted (for v1.0.6 since I missed 1.0.5). Concerning the patch for bridge_driver.c ... I did not like it when I submitted it. The first thing is that I need to find out why ::/0 is getting an error. The error message is "RTNETLINK answers: File exists" and this is exactly the same error message you get if you try to do a second static route for an existing route (address + prefix). "/sbin/ip -6 route" provides little info but "sbin/route -A inet6" is a little more helpful. However, although there are multiple [::]/0 routes, none of them are defined for the virtual bridge .... maybe I found a bug ... wishful thinking [?] Next, if ::/0 is invalid, then this needs to be addressed in the parser. I will re-work and run this up the flag pole again. Gene