[libvirt] [PATCH-v4.2] Support for static routes on a virtual bridge
Gene Czarcinski
gene at czarc.net
Sat May 4 18:56:30 UTC 2013
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
More information about the libvir-list
mailing list