[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]