(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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list