[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/26/2013 07:22 PM, Gene Czarcinski wrote:
> network: static route support for <network>
>
> This patch adds the <route> subelement of <network> to define a static
> route.  the address and prefix (or netmask) attribute identify the
> destination network, and the gateway attribute specifies the next hop
> address (which must be directly reachable from the containing
> <network>) which is to receive the packets destined for
> "address/(prefix|netmask)".
>
> Tests are done to validate that the input definitions are
> correct.  For example, for a static route ip definition,
> the address must be a network address and not a host address.
> Additional checks are added to ensure that the specified gateway
> is directly reachable via a network defined on this bridge.
>
> Whan a static route is added to a bridge, there is a slight
> possibility that the gateway address will be incorrect.  If
> this is handled as an error, that bridge becomes unusable and
> can only be recovered by rebooting.  If the error is
> ignored, then that network can be destroyed and the network
> definition file edited to correct the problem.  Unfortunately,
> the error message only appears in syslog.  However, with
> the checks performed when the network definition file is parsed,
> it is unlikely that this condition will ever occur.
>
> Handling of errors in this manner is consistent with other uses
> of the CommandRun interface.
>
> The command used is of the following form:
>
> ip route add <address>/<prefix> via <gateway> dev <virbr-bridge> \
> proto static metric 1
>
> While family='ipv4' address='0.0.0.0' netmask='0.0.0.0' or prefix='0' is
> supported and does work, the same cannot be said for ipv6.   Therefore,
> if address='::' prefix='0' is specified for family='ipv6' an error
> message is issued stating that this is not currently supported.
> .
> Signed-off-by: Gene Czarcinski <gene czarc net>
> ---
>  docs/formatnetwork.html.in                         |  71 +++++
>  docs/schemas/network.rng                           |  19 ++
>  src/conf/network_conf.c                            | 318 ++++++++++++++++++++-
>  src/conf/network_conf.h                            |  22 ++
>  src/libvirt_private.syms                           |   1 +
>  src/network/bridge_driver.c                        |  78 +++++
>  src/util/virnetdev.c                               |  44 +++
>  src/util/virnetdev.h                               |   5 +
>  .../networkxml2xmlin/dhcp6host-routed-network.xml  |   2 +
>  .../networkxml2xmlout/dhcp6host-routed-network.xml |   2 +
>  10 files changed, 561 insertions(+), 1 deletion(-)
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 4dd0415..15dce3a 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -529,6 +529,50 @@
>        starting.
>      </p>
>  
> +    <h5><a name="elementsStaticroute">Static Routes</a></h5>
> +    <p>
> +      Static route definitions are used to provide routing
> +      information to the virtualization host for networks which are not
> +      defined as a network on one of the virtual bridges so that such
> +      networks can be reachable from the virtualization
> +      host <span class="since">Since 1.0.5</span>.
> +    </p>
> +
> +    <p>
> +      As shown in <a href="formatnetwork.html#examplesNoGateway">this example</a>,
> +      it is possible to define a virtual bridge interface with no
> +      IPv4 or IPv6 networks.  Such interfaces are useful in supporting
> +      networks which have no visibility or direct connectivity with the
> +      virtualization host, but can be used to support
> +      virtual networks which only have guest connectivity.  A guest
> +      with connectivity to the guest-only network and another network
> +      that is directly reachable from the host can act as a gateway between the
> +      networks.  A static route added to the "visible" network definition
> +      provides the routing information so that IP packets can be sent
> +      from the virtualization host to guests on the hidden network.
> +    </p>
> +
> +    <p>
> +      Here is a fragment of a definition which shows the static
> +      route specification as well as the  IPv4 and IPv6 definitions
> +      for network addresses which are referred to in the
> +      <code>gateway</code> gateway address specifications.
> +    </p>
> +
> +    <pre>
> +      ...
> +        &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt;
> +          &lt;dhcp&gt;
> +            &lt;range start="192.168.122.128" end="192.168.122.254" /&gt;
> +          &lt;/dhcp&gt;
> +        &lt;/ip&gt;
> +        &lt;route address="192.168.222.0" prefix="24" gateway="192.168.122.2" /&gt;
> +        &lt;ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /&gt;
> +        &lt;route family="ipv6" address="2001:db8:ca2:3::" prefix="64" gateway="2001:db8:ca2:2::2"&gt;
> +        &lt;/route&gt;
> +      ...
> +    </pre>
> +
>      <h3><a name="elementsAddress">Addressing</a></h3>
>  
>      <p>
> @@ -560,6 +604,7 @@
>            &lt;/dhcp&gt;
>          &lt;/ip&gt;
>          &lt;ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /&gt;
> +        &lt;route family="ipv6" address="2001:db9:ca1:1::" prefix="64" gateway="2001:db8:ca2:2::2" /&gt;
>        &lt;/network&gt;</pre>
>  
>      <dl>
> @@ -809,6 +854,32 @@
>          &lt;/ip&gt;
>        &lt;/network&gt;</pre>
>  
> +    <p>
> +      Below is yet another IPv6 variation.  This variation has only IPv6
> +      defined with DHCPv6 on the primary IPv6 network.  A static link
> +      if defined for a second IPv6 network which will not be visable on
> +      the bridge interface but will have a static route defined for this
> +      network via the specified gateway. Note that the gateway address
> +      must be directly reachable via (on the same subnet as) one of the
> +      &lt;ip&gt; addresses defined for this &lt;network&gt;.
> +      <span class="since">Since 1.0.5</span>
> +    </p>
> +
> +    <pre>
> +      &lt;network&gt;
> +        &lt;name&gt;net7&lt;/name&gt;
> +        &lt;bridge name="virbr7" /&gt;
> +        &lt;forward mode="route"/&gt;
> +        &lt;ip family="ipv6" address="2001:db8:ca2:7::1" prefix="64" &gt;
> +          &lt;dhcp&gt;
> +            &lt;range start="2001:db8:ca2:7::100" end="2001:db8:ca2::1ff" /&gt;
> +            &lt;host id="0:4:7e:7d:f0:7d:a8:bc:c5:d2:13:32:11:ed:16:ea:84:63" name="lucas" ip="2001:db8:ca2:2:3::4" /&gt;
> +          &lt;/dhcp&gt;
> +        &lt;/ip&gt;
> +        &lt;route family="ipv6" address="2001:db8:ca2:8::" prefix="64" gateway="2001:db8:ca2:7::4" &gt;
> +        &lt;/route&gt;
> +      &lt;/network&gt;</pre>
> +
>      <h3><a name="examplesPrivate">Isolated network config</a></h3>
>  
>      <p>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 6c3fae2..1106b76 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -305,6 +305,25 @@
>              </optional>
>            </element>
>          </zeroOrMore>
> +        <!-- <route> element -->
> +        <zeroOrMore>
> +          <!-- The (static) route element specifies a network address and gateway
> +               address to access that network. Bother the network address and


s/Bother/Both/


> +               the gateway address must be specified. -->
> +          <element name="route">
> +            <optional>
> +              <attribute name="family"><ref name="addr-family"/></attribute>
> +            </optional>
> +            <attribute name="address"><ref name="ipAddr"/></attribute>
> +            <optional>
> +              <choice>
> +                <attribute name="netmask"><ref name="ipv4Addr"/></attribute>
> +                <attribute name="prefix"><ref name="ipPrefix"/></attribute>
> +              </choice>
> +            </optional>
> +            <attribute name="gateway"><ref name="ipAddr"/></attribute>
> +          </element>
> +        </zeroOrMore>
>        </interleave>
>      </element>
>    </define>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 1c88547..93ac535 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -137,6 +137,12 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
>  }
>  
>  static void
> +virNetworkRouteDefClear(virNetworkRouteDefPtr def)
> +{
> +    VIR_FREE(def->family);
> +}
> +
> +static void
>  virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def)
>  {
>      VIR_FREE(def->name);
> @@ -215,6 +221,11 @@ virNetworkDefFree(virNetworkDefPtr def)
>      }
>      VIR_FREE(def->ips);
>  
> +    for (ii = 0 ; ii < def->nroutes && def->routes ; ii++) {
> +        virNetworkRouteDefClear(&def->routes[ii]);
> +    }
> +    VIR_FREE(def->routes);
> +
>      for (ii = 0; ii < def->nPortGroups && def->portGroups; ii++) {
>          virPortGroupDefClear(&def->portGroups[ii]);
>      }
> @@ -1264,6 +1275,198 @@ cleanup:
>  }
>  
>  static int
> +virNetworkRouteDefParseXML(const char *networkName,
> +                           xmlNodePtr node,
> +                           xmlXPathContextPtr ctxt,
> +                           virNetworkRouteDefPtr def)
> +{
> +    /*
> +     * virNetworkRouteDef object is already allocated as part
> +     * of an array.  On failure clear: it out, but don't free it.
> +     */
> +
> +    xmlNodePtr save;
> +    char *address = NULL, *netmask = NULL;
> +    char *gateway = NULL;
> +    unsigned long prefix = 0;
> +    int result = -1;
> +    int prefixRc;
> +    virSocketAddr testAddr;
> +
> +    save = ctxt->node;
> +    ctxt->node = node;
> +
> +    /* grab raw data from XML */
> +    def->family = virXPathString("string(./@family)", ctxt);
> +
> +    address = virXPathString("string(./@address)", ctxt);
> +
> +    netmask = virXPathString("string(./@netmask)", ctxt);
> +
> +    gateway = virXPathString("string(./@gateway)", ctxt);

No need for all the extra blank lines.


> +
> +    prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix);
> +    if (prefixRc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Invalid prefix specified in route definition '%s'"),
> +                       networkName);
> +        goto cleanup;
> +    }
> +    if (!(prefixRc < 0))
> +        def->has_prefix = true;
> +    else
> +        def->has_prefix = false;


How about "def->has_prefix = (prefixRc == 0);" ?


> +
> +    /* Note: both network and gateway addresses must be specified */
> +
> +    if (!address) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("A network address must be specified in route definition '%s'"),


  "Missing required address attribute in route definition of network '%s'"


> +                       networkName);
> +        goto cleanup;
> +    }
> +
> +    if (!gateway) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("A gateway address must be specified in route definition '%s'"),


  "Missing required gateway attribute in route definition of network '%s'"


> +                       networkName);
> +        goto cleanup;
> +    }
> +
> +    if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Bad network address '%s' in route definition '%s'"),

"... of network"

> +                       address, networkName);
> +        goto cleanup;
> +    }
> +
> +    if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Bad gateway address '%s' in route definition '%s'"),



"... of network"


> +                       gateway, networkName);
> +        goto cleanup;
> +    }
> +
> +    /* validate network address, etc. for each family */
> +    if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) {
> +        if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) ||
> +              VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("%s family specified for non-IPv4 address '%s' in route definition '%s'"),
> +                           def->family == NULL? "no" : "ipv4", address, networkName);
> +            goto cleanup;
> +        }
> +        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("%s family specified for non-IPv4 gateway address '%s' in route definition '%s'"),
> +                           def->family == NULL? "no" : "ipv4", address, networkName);
> +            goto cleanup;
> +        }
> +        if (netmask) {
> +            if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) {
> +                virReportError(VIR_ERR_XML_ERROR,
> +                               _("Bad netmask address '%s' in route definition '%s'"),
> +                               netmask, networkName);
> +                goto cleanup;
> +            }
> +
> +            if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("network '%s' has invalid netmask '%s' for address '%s' (both must be IPv4)"),
> +                               networkName, netmask, address);
> +                goto cleanup;
> +            }
> +            if (def->has_prefix) {
> +                /* can't have both netmask and prefix at the same time */
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("route definition '%s' cannot have both a prefix and a netmask"),
> +                               networkName);
> +                goto cleanup;
> +            }
> +        }
> +        else {
> +            if (def->has_prefix)
> +                def->prefix = prefix;

Really you don't need to qualify this assignment - if has_prefix is
false, prefix will always be 0, and that's what def->prefix is
initialized to anyway (and if virXPathULong() returns < 0, prefix will
have been unchanged from it's initial value of 0.


> +        }
> +        if (def->prefix > 32) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Invalid IPv4 prefix '%lu' for '%s' specified in route definition '%s'"),
> +                           def->prefix, def->family == NULL? "no" : "ipv4", networkName);

def->prefix is unsigned int, so the format should be %u, not %lu

> +            goto cleanup;
> +        }
> +    } else if (STREQ(def->family, "ipv6")) {
> +        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("ipv6 family specified for non-IPv6 address '%s' in route definition '%s'"),
> +                           address, networkName);
> +            goto cleanup;
> +        }
> +        if (netmask) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("specifying netmask invalid for IPv6 address '%s' in route definition '%s'"),
> +                           address, networkName);
> +            goto cleanup;
> +        }
> +        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("ipv6 specified for non-IPv6 gateway address '%s' in route definition '%s'"),
> +                           gateway, networkName);
> +            goto cleanup;
> +        }
> +        if (def->has_prefix)
> +            def->prefix = prefix;


Again - this doesn't need to be qualified. As a matter of fact, you can
just unconditionally set it up where you set def->has_prefix - if it
hasn't been set, you'll just be assigning 0 to def->prefix.


> +
> +        if (def->prefix > 128) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Invalid IPv6 prefix '%lu' for '%s' specified in route definition '%s'"),
> +                           def->prefix, def->family, networkName);


again - format for def->prefix should be %u, not %lu


> +            goto cleanup;
> +        }
> +    } else {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Unrecognized family '%s' in definition of route definition '%s'"),
> +                       def->family, networkName);
> +        goto cleanup;
> +    }
> +
> +    /* make sure the address is a network address */
> +    if (netmask) {
> +        if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("error converting address '%s' with netmask '%s' to network-address in route definition '%s'"),
> +                           address, netmask, networkName);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (virSocketAddrMaskByPrefix(&def->address, def->prefix, &testAddr) < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("error converting address '%s' with prefix=%u to network-address in route definition '%s'"),
> +                           address, def->prefix, networkName);
> +            goto cleanup;
> +        }
> +    }
> +    if (!virSocketAddrEqual(&def->address, &testAddr)) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("address '%s' in <route> definition of network '%s' is not a network address"),
> +                       address, networkName);
> +        goto cleanup;
> +    }
> +
> +    result = 0;
> +
> +cleanup:
> +    if (result < 0) {
> +        virNetworkRouteDefClear(def);
> +    }
> +    VIR_FREE(address);
> +    VIR_FREE(netmask);
> +    VIR_FREE(gateway);
> +
> +    ctxt->node = save;
> +    return result;
> +}
> +
> +static int
>  virNetworkPortGroupParseXML(virPortGroupDefPtr def,
>                              xmlNodePtr node,
>                              xmlXPathContextPtr ctxt)
> @@ -1661,8 +1864,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>      char *tmp;
>      char *stp = NULL;
>      xmlNodePtr *ipNodes = NULL;
> +    xmlNodePtr *routeNodes = NULL;
>      xmlNodePtr *portGroupNodes = NULL;
> -    int nIps, nPortGroups;
> +    int nIps, nPortGroups, nRoutes;
>      xmlNodePtr dnsNode = NULL;
>      xmlNodePtr virtPortNode = NULL;
>      xmlNodePtr forwardNode = NULL;
> @@ -1816,6 +2020,70 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>      }
>      VIR_FREE(ipNodes);
>  
> +    nRoutes = virXPathNodeSet("./route", ctxt, &routeNodes);
> +    if (nRoutes < 0)
> +        goto error;
> +
> +    if (nRoutes > 0) {
> +        int ii;
> +
> +        /* allocate array to hold all the route definitions */
> +        if (VIR_ALLOC_N(def->routes, nRoutes) < 0) {
> +            virReportOOMError();
> +            goto error;
> +        }
> +        /* parse each definition */
> +        for (ii = 0; ii < nRoutes; ii++) {
> +            int ret = virNetworkRouteDefParseXML(def->name, routeNodes[ii],
> +                                              ctxt, &def->routes[ii]);
> +            if (ret < 0)
> +                goto error;
> +            def->nroutes++;
> +        }
> +
> +        /* now validate the correctness of any static route gateways specified
> +         *
> +         * note: the parameters within each definition are varified/assumed valid;

s/varified/verified/

> +         * the question being asked and answered here is if the specified gateway
> +         * is directly reachable from this bridge.
> +         */
> +        nRoutes = def->nroutes;
> +        nIps = def->nips;
> +        for (ii = 0; ii < nRoutes; ii++) {
> +            int jj;
> +            virSocketAddr testAddr, testGw;
> +            bool addrMatch;
> +            virNetworkRouteDefPtr gwdef = &def->routes[ii];
> +            if (VIR_SOCKET_ADDR_VALID(&gwdef->gateway)) {


The route entry parser has already guaranteed that gwdef->gateway is
valid (otherwise it would have returned an error).


> +                addrMatch = false;
> +                for (jj = 0; jj < nIps; jj++) {
> +                    virNetworkIpDefPtr def2 = &def->ips[jj];
> +                    if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway)
> +                        != VIR_SOCKET_ADDR_FAMILY(&def2->address)) {
> +                        continue;
> +                    }
> +                    int prefix = virNetworkIpDefPrefix(def2);
> +                    virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr);
> +                    virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw);
> +                    if (VIR_SOCKET_ADDR_VALID(&testAddr) &&
> +                        VIR_SOCKET_ADDR_VALID(&testGw) &&
> +                        virSocketAddrEqual(&testAddr, &testGw)) {
> +                        addrMatch = true;
> +                        break;
> +                    }
> +                }
> +                if (!addrMatch) {
> +                    char *gw = virSocketAddrFormat(&gwdef->gateway);
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                                   _("unreachable static route gateway '%s' specified for network '%s'"),
> +                                   gw, def->name);
> +                    VIR_FREE(gw);
> +                    goto error;
> +                }
> +            }


Otherwise these loops look good now.


> +        }
> +    }
> +
>      forwardNode = virXPathNode("./forward", ctxt);
>      if (forwardNode &&
>          virNetworkForwardDefParseXML(def->name, forwardNode, ctxt, &def->forward) < 0) {
> @@ -1888,6 +2156,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>      return def;
>  
>  error:
> +    VIR_FREE(routeNodes);


and you're free'ing the routeNodes :-)


>      VIR_FREE(stp);
>      virNetworkDefFree(def);
>      VIR_FREE(ipNodes);
> @@ -2113,6 +2382,48 @@ error:
>  }
>  
>  static int
> +virNetworkRouteDefFormat(virBufferPtr buf,
> +                      const virNetworkRouteDefPtr def)
> +{
> +    int result = -1;
> +
> +    virBufferAddLit(buf, "<route");
> +
> +    if (def->family) {
> +        virBufferAsprintf(buf, " family='%s'", def->family);
> +    }
> +    if (VIR_SOCKET_ADDR_VALID(&def->address)) {


Well, we decided to require a valid address (and gateway), so this isn't
strictly necessary, but it doesn't hurt anything (and it's possible
we'll change our mind about requiring address in the future).


> +        char *addr = virSocketAddrFormat(&def->address);
> +        if (!addr)
> +            goto error;
> +        virBufferAsprintf(buf, " address='%s'", addr);
> +        VIR_FREE(addr);
> +    }
> +    if (VIR_SOCKET_ADDR_VALID(&def->netmask)) {
> +        char *addr = virSocketAddrFormat(&def->netmask);
> +        if (!addr)
> +            goto error;
> +        virBufferAsprintf(buf, " netmask='%s'", addr);
> +        VIR_FREE(addr);
> +    }
> +    if (def->has_prefix) {
> +        virBufferAsprintf(buf," prefix='%u'", def->prefix);
> +    }
> +    if (VIR_SOCKET_ADDR_VALID(&def->gateway)) {
> +        char *addr = virSocketAddrFormat(&def->gateway);
> +        if (!addr)
> +            goto error;
> +        virBufferAsprintf(buf, " gateway='%s'", addr);
> +        VIR_FREE(addr);
> +    }
> +    virBufferAddLit(buf, "/>\n");
> +
> +    result = 0;
> +error:
> +    return result;
> +}
> +
> +static int
>  virPortGroupDefFormat(virBufferPtr buf,
>                        const virPortGroupDefPtr def)
>  {
> @@ -2310,6 +2621,11 @@ virNetworkDefFormatInternal(virBufferPtr buf,
>              goto error;
>      }
>  
> +    for (ii = 0; ii < def->nroutes; ii++) {
> +        if (virNetworkRouteDefFormat(buf, &def->routes[ii]) < 0)
> +            goto error;
> +    }
> +
>      if (virNetDevVPortProfileFormat(def->virtPortProfile, buf) < 0)
>          goto error;
>  
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 163478c..7be9bd6 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -135,6 +135,25 @@ struct _virNetworkIpDef {
>      virSocketAddr bootserver;
>     };
>  
> +typedef struct _virNetworkRouteDef virNetworkRouteDef;
> +typedef virNetworkRouteDef *virNetworkRouteDefPtr;
> +struct _virNetworkRouteDef {
> +    char *family;               /* ipv4 or ipv6 - default is ipv4 */
> +    virSocketAddr address;      /* Routed Network IP address */
> +
> +    /* One or the other of the following two will be used for a given
> +     * Network address, but never both. The parser guarantees this.
> +     * The virSocketAddrGetIpPrefix() can be used to get a
> +     * valid prefix.
> +     */
> +    unsigned int prefix;        /* ipv6 - only prefix allowed */
> +    virSocketAddr netmask;      /* ipv4 - either netmask or prefix specified */
> +
> +    virSocketAddr gateway;      /* gateway IP address for ip-route */
> +
> +    bool has_prefix;            /* prefix= was specified */


has_prefix should be right next to prefix.


> +   };
> +
>  typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
>  typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;
>  struct _virNetworkForwardIfDef {
> @@ -209,6 +228,9 @@ struct _virNetworkDef {
>      size_t nips;
>      virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */
>  
> +    size_t nroutes;
> +    virNetworkRouteDefPtr routes; /* ptr to array of static routes on this interface */
> +
>      virNetworkDNSDef dns;   /* dns related configuration */
>      virNetDevVPortProfilePtr virtPortProfile;
>  
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 882b77d..09ad053 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1478,6 +1478,7 @@ virNetDevReplaceMacAddress;
>  virNetDevReplaceNetConfig;
>  virNetDevRestoreMacAddress;
>  virNetDevRestoreNetConfig;
> +virNetDevSetGateway;
>  virNetDevSetIPv4Address;
>  virNetDevSetMAC;
>  virNetDevSetMTU;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 53db5d5..37ab386 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2370,6 +2370,7 @@ out:
>      return ret;
>  }
>  
> +/* add an IP address to a bridge */
>  static int
>  networkAddAddrToBridge(virNetworkObjPtr network,
>                         virNetworkIpDefPtr ipdef)
> @@ -2390,6 +2391,69 @@ networkAddAddrToBridge(virNetworkObjPtr network,
>      return 0;
>  }
>  
> +/* 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;
> +        }
> +    }

My comments about this code got too long, so I moved them to a separate
message.

In short - this is too complicated. I attached a patch to this message
(that also addresses all my other critiques) which removes all of this
extra validation. We can address it in a separate patch (which I've
taken a stab at in my other reply).

> +    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;
> +        }
> +    }
> +
> +    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;
> +}
> +
>  static int
>  networkStartNetworkVirtual(struct network_driver *driver,
>                            virNetworkObjPtr network)
> @@ -2398,6 +2462,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
>      bool v4present = false, v6present = false;
>      virErrorPtr save_err = NULL;
>      virNetworkIpDefPtr ipdef;
> +    virNetworkRouteDefPtr routedef;
>      char *macTapIfName = NULL;
>      int tapfd = -1;
>  
> @@ -2474,6 +2539,19 @@ networkStartNetworkVirtual(struct network_driver *driver,
>      if (virNetDevSetOnline(network->def->bridge, 1) < 0)
>          goto err2;
>  
> +    for (ii = 0; ii < network->def->nroutes; ii++) {
> +        routedef = &network->def->routes[ii];
> +        /* Add the IP route to the bridge */
> +        /* ignore errors, error msg will be generated */
> +        /* but libvirt will not know and net-destroy will work. */
> +        if (VIR_SOCKET_ADDR_VALID(&routedef->gateway)) {
> +            if (networkAddRouteToBridge(network, routedef) < 0) {
> +                /* an error occurred adding the static route */
> +                continue; /* for now, do nothing */
> +            }
> +        }
> +    }
> +
>      /* If forward.type != NONE, turn on global IP forwarding */
>      if (network->def->forward.type != VIR_NETWORK_FORWARD_NONE &&
>          networkEnableIpForwarding(v4present, v6present) < 0) {
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 7ffaac1..69a8830 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -769,6 +769,50 @@ cleanup:
>  }
>  
>  /**
> + * virNetDevSetGateway:


I don't know why I didn't think of this before - this should be called
virNetDevAddRoute(), since it's adding an additional route (destination
+ gateway), not changing "the one" gateway (and nothing else).


> + * @ifname: the interface name
> + * @addr: the IP network address (IPv4 or IPv6)
> + * @prefix: number of 1 bits in the netmask
> + * @gateway: via address for route (same as @addr)
> + *
> + * Add a route for a network IP address to an interface. This function
> + * *does not* remove any previously added IP static routes.
> + *
> + * Returns 0 in case of success or -1 in case of error.
> + */
> +
> +int virNetDevSetGateway(const char *ifname,
> +                        virSocketAddrPtr addr,
> +                        unsigned int prefix,
> +                        virSocketAddrPtr gateway)
> +{
> +    virCommandPtr cmd = NULL;
> +    char *addrstr = NULL, *gatewaystr = NULL;
> +    int ret = -1;
> +
> +    if (!(addrstr = virSocketAddrFormat(addr)))
> +        goto cleanup;
> +    if (!(gatewaystr = virSocketAddrFormat(gateway)))
> +        goto cleanup;
> +    cmd = virCommandNew(IP_PATH);
> +    virCommandAddArgList(cmd, "route", "add", NULL);
> +    virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
> +    virCommandAddArgList(cmd, "via", gatewaystr, "dev", ifname,
> +                              "proto", "static", "metric", NULL);
> +    virCommandAddArgFormat(cmd, "%u", 1);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addrstr);
> +    VIR_FREE(gatewaystr);
> +    virCommandFree(cmd);
> +    return ret;
> +}
> +
> +/**
>   * virNetDevClearIPv4Address:
>   * @ifname: the interface name
>   * @addr: the IP address (IPv4 or IPv6)
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 551ea22..c8f20b1 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -42,6 +42,11 @@ int virNetDevSetIPv4Address(const char *ifname,
>                              virSocketAddr *addr,
>                              unsigned int prefix)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virNetDevSetGateway(const char *ifname,
> +                        virSocketAddrPtr addr,
> +                        unsigned int prefix,
> +                        virSocketAddrPtr gateway)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
>  int virNetDevClearIPv4Address(const char *ifname,
>                                virSocketAddr *addr,
>                                unsigned int prefix)
> diff --git a/tests/networkxml2xmlin/dhcp6host-routed-network.xml b/tests/networkxml2xmlin/dhcp6host-routed-network.xml
> index 2693d87..40f6dfe 100644
> --- a/tests/networkxml2xmlin/dhcp6host-routed-network.xml
> +++ b/tests/networkxml2xmlin/dhcp6host-routed-network.xml
> @@ -19,4 +19,6 @@
>        <host id='0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66' name='badbob' ip='2001:db8:ac10:fd01::1:24' />
>      </dhcp>
>    </ip>
> +  <route address="192.168.222.0" netmask="255.255.255.0" gateway="192.168.122.10"/>
> +  <route family="ipv6" address="2001:db8:ac10:fc00::" prefix="64" gateway="2001:db8:ac10:fd01::1:24"/>
>  </network>
> diff --git a/tests/networkxml2xmlout/dhcp6host-routed-network.xml b/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> index 1d3035b..fc8666b 100644
> --- a/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> +++ b/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> @@ -21,4 +21,6 @@
>        <host id='0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66' name='badbob' ip='2001:db8:ac10:fd01::1:24' />
>      </dhcp>
>    </ip>
> +  <route address='192.168.222.0' netmask='255.255.255.0' gateway='192.168.122.10'/>
> +  <route family='ipv6' address='2001:db8:ac10:fc00::' prefix='64' gateway='2001:db8:ac10:fd01::1:24'/>
>  </network>


A patch which addresses everything here is attached to the message. Your
patch has my ACK after applying the tweaks, but then *my* tweak patch
needs an ACK. Can you apply it locally and see if everything works
(aside from 0.0.0.0/0)? (the bulk of that patch is splitting long log
messages into multiple lines, reindenting one block of code after
removing an extra level on if(), and removing the section I mentioned
above).
>From da3063a72fbeb25984f4836a9dfc94a73773162b Mon Sep 17 00:00:00 2001
From: Laine Stump <laine laine org>
Date: Mon, 29 Apr 2013 11:12:47 -0400
Subject: [PATCH] SQUASH THIS COMMIT

This patch contains modifications to address issues I found during
review of Gene Czarcinski's

 "Support for static routes on a virtual bridge"

patch. This should be squashed into his patch before pushing.

*) simpler setting of prefix and use_prefix.
*) added line breaks and reworded many log messages
*) use %u instead of %lu for def->prefix.
*) remove unnecessary extra check for valid gateway address
   in virNetworkDefParse (it is guaranteed to be valid)
*) rename virNetDevSetGateway to virNetDevAddRoute
*) remove extra checks on prefix/address in networkAddRouteToBridge().
   A simpler alternative will be in a separate patch.
---
 docs/schemas/network.rng    |   2 +-
 src/conf/network_conf.c     | 137 +++++++++++++++++++++++---------------------
 src/conf/network_conf.h     |   6 +-
 src/libvirt_private.syms    |   2 +-
 src/network/bridge_driver.c |  62 ++++----------------
 src/util/virnetdev.c        |   9 +--
 src/util/virnetdev.h        |   8 +--
 7 files changed, 98 insertions(+), 128 deletions(-)

diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 3478e7d..a91a24f 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -319,7 +319,7 @@
         <!-- <route> element -->
         <zeroOrMore>
           <!-- The (static) route element specifies a network address and gateway
-               address to access that network. Bother the network address and
+               address to access that network. Both the network address and
                the gateway address must be specified. -->
           <element name="route">
             <optional>
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 05e8e01..dffa0ad 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1304,51 +1304,50 @@ virNetworkRouteDefParseXML(const char *networkName,
 
     /* grab raw data from XML */
     def->family = virXPathString("string(./@family)", ctxt);
-
     address = virXPathString("string(./@address)", ctxt);
-
     netmask = virXPathString("string(./@netmask)", ctxt);
-
     gateway = virXPathString("string(./@gateway)", ctxt);
-
     prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix);
     if (prefixRc == -2) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Invalid prefix specified in route definition '%s'"),
+                       _("Invalid prefix specified "
+                         "in route definition of network '%s'"),
                        networkName);
         goto cleanup;
     }
-    if (!(prefixRc < 0))
-        def->has_prefix = true;
-    else
-        def->has_prefix = false;
+    def->has_prefix = (prefixRc == 0);
+    def->prefix = prefix;
 
     /* Note: both network and gateway addresses must be specified */
 
     if (!address) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("A network address must be specified in route definition '%s'"),
+                       _("Missing required address attribute "
+                         "in route definition of network '%s'"),
                        networkName);
         goto cleanup;
     }
 
     if (!gateway) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("A gateway address must be specified in route definition '%s'"),
+                       _("Missing required gateway attribute "
+                         "in route definition of network '%s'"),
                        networkName);
         goto cleanup;
     }
 
     if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Bad network address '%s' in route definition '%s'"),
+                       _("Bad network address '%s' "
+                         "in route definition of network '%s'"),
                        address, networkName);
         goto cleanup;
     }
 
     if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Bad gateway address '%s' in route definition '%s'"),
+                       _("Bad gateway address '%s' "
+                         "in route definition of network '%s'"),
                        gateway, networkName);
         goto cleanup;
     }
@@ -1358,79 +1357,85 @@ virNetworkRouteDefParseXML(const char *networkName,
         if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) ||
               VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("%s family specified for non-IPv4 address '%s' in route definition '%s'"),
-                           def->family == NULL? "no" : "ipv4", address, networkName);
+                           _("%s family specified for non-IPv4 address '%s' "
+                             "in route definition of network '%s'"),
+                           def->family == NULL? "no" : "ipv4",
+                           address, networkName);
             goto cleanup;
         }
         if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("%s family specified for non-IPv4 gateway address '%s' in route definition '%s'"),
-                           def->family == NULL? "no" : "ipv4", address, networkName);
+                           _("%s family specified for non-IPv4 gateway '%s' "
+                             "in route definition of network '%s'"),
+                           def->family == NULL? "no" : "ipv4",
+                           address, networkName);
             goto cleanup;
         }
         if (netmask) {
             if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) {
                 virReportError(VIR_ERR_XML_ERROR,
-                               _("Bad netmask address '%s' in route definition '%s'"),
+                               _("Bad netmask address '%s' "
+                                 "in route definition of network '%s'"),
                                netmask, networkName);
                 goto cleanup;
             }
-
             if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("network '%s' has invalid netmask '%s' for address '%s' (both must be IPv4)"),
+                               _("network '%s' has invalid netmask '%s' "
+                                 "for address '%s' (both must be IPv4)"),
                                networkName, netmask, address);
                 goto cleanup;
             }
             if (def->has_prefix) {
                 /* can't have both netmask and prefix at the same time */
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("route definition '%s' cannot have both a prefix and a netmask"),
+                               _("route definition '%s' cannot have both "
+                                 "a prefix and a netmask"),
                                networkName);
                 goto cleanup;
             }
         }
-        else {
-            if (def->has_prefix)
-                def->prefix = prefix;
-        }
         if (def->prefix > 32) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Invalid IPv4 prefix '%lu' for '%s' specified in route definition '%s'"),
-                           def->prefix, def->family == NULL? "no" : "ipv4", networkName);
+                           _("Invalid IPv4 prefix %u for '%s' specified "
+                             "in route definition of network '%s'"),
+                           def->prefix,
+                           def->family == NULL? "no" : "ipv4", networkName);
             goto cleanup;
         }
     } else if (STREQ(def->family, "ipv6")) {
         if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("ipv6 family specified for non-IPv6 address '%s' in route definition '%s'"),
+                           _("ipv6 family specified for non-IPv6 address '%s' "
+                             "in route definition of network '%s'"),
                            address, networkName);
             goto cleanup;
         }
         if (netmask) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("specifying netmask invalid for IPv6 address '%s' in route definition '%s'"),
+                           _("specifying netmask invalid for IPv6 address '%s' "
+                             "in route definition of network '%s'"),
                            address, networkName);
             goto cleanup;
         }
         if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("ipv6 specified for non-IPv6 gateway address '%s' in route definition '%s'"),
+                           _("ipv6 specified for non-IPv6 gateway address '%s' "
+                             "in route definition of network '%s'"),
                            gateway, networkName);
             goto cleanup;
         }
-        if (def->has_prefix)
-            def->prefix = prefix;
-
         if (def->prefix > 128) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Invalid IPv6 prefix '%lu' for '%s' specified in route definition '%s'"),
+                           _("Invalid IPv6 prefix %u for '%s' specified "
+                             "in route definition of network '%s'"),
                            def->prefix, def->family, networkName);
             goto cleanup;
         }
     } else {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Unrecognized family '%s' in definition of route definition '%s'"),
+                       _("Unrecognized family '%s' "
+                         "in route definition of network'%s'"),
                        def->family, networkName);
         goto cleanup;
     }
@@ -1439,21 +1444,27 @@ virNetworkRouteDefParseXML(const char *networkName,
     if (netmask) {
         if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("error converting address '%s' with netmask '%s' to network-address in route definition '%s'"),
+                           _("error converting address '%s' with netmask '%s' "
+                             "to network-address "
+                             "in route definition of network '%s'"),
                            address, netmask, networkName);
             goto cleanup;
         }
     } else {
-        if (virSocketAddrMaskByPrefix(&def->address, def->prefix, &testAddr) < 0) {
+        if (virSocketAddrMaskByPrefix(&def->address,
+                                      def->prefix, &testAddr) < 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("error converting address '%s' with prefix=%u to network-address in route definition '%s'"),
+                           _("error converting address '%s' with prefix %u "
+                             "to network-address "
+                             "in route definition of network '%s'"),
                            address, def->prefix, networkName);
             goto cleanup;
         }
     }
     if (!virSocketAddrEqual(&def->address, &testAddr)) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                       _("address '%s' in <route> definition of network '%s' is not a network address"),
+                       _("address '%s' in route definition of network '%s' "
+                         "is not a network address"),
                        address, networkName);
         goto cleanup;
     }
@@ -2066,7 +2077,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
 
         /* now validate the correctness of any static route gateways specified
          *
-         * note: the parameters within each definition are varified/assumed valid;
+         * note: the parameters within each definition are verified/assumed valid;
          * the question being asked and answered here is if the specified gateway
          * is directly reachable from this bridge.
          */
@@ -2077,33 +2088,31 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
             virSocketAddr testAddr, testGw;
             bool addrMatch;
             virNetworkRouteDefPtr gwdef = &def->routes[ii];
-            if (VIR_SOCKET_ADDR_VALID(&gwdef->gateway)) {
-                addrMatch = false;
-                for (jj = 0; jj < nIps; jj++) {
-                    virNetworkIpDefPtr def2 = &def->ips[jj];
-                    if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway)
-                        != VIR_SOCKET_ADDR_FAMILY(&def2->address)) {
-                        continue;
-                    }
-                    int prefix = virNetworkIpDefPrefix(def2);
-                    virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr);
-                    virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw);
-                    if (VIR_SOCKET_ADDR_VALID(&testAddr) &&
-                        VIR_SOCKET_ADDR_VALID(&testGw) &&
-                        virSocketAddrEqual(&testAddr, &testGw)) {
-                        addrMatch = true;
-                        break;
-                    }
+            addrMatch = false;
+            for (jj = 0; jj < nIps; jj++) {
+                virNetworkIpDefPtr def2 = &def->ips[jj];
+                if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway)
+                    != VIR_SOCKET_ADDR_FAMILY(&def2->address)) {
+                    continue;
                 }
-                if (!addrMatch) {
-                    char *gw = virSocketAddrFormat(&gwdef->gateway);
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("unreachable static route gateway '%s' specified for network '%s'"),
-                                   gw, def->name);
-                    VIR_FREE(gw);
-                    goto error;
+                int prefix = virNetworkIpDefPrefix(def2);
+                virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr);
+                virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw);
+                if (VIR_SOCKET_ADDR_VALID(&testAddr) &&
+                    VIR_SOCKET_ADDR_VALID(&testGw) &&
+                    virSocketAddrEqual(&testAddr, &testGw)) {
+                    addrMatch = true;
+                    break;
                 }
             }
+            if (!addrMatch) {
+                char *gw = virSocketAddrFormat(&gwdef->gateway);
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("unreachable static route gateway '%s' specified for network '%s'"),
+                               gw, def->name);
+                VIR_FREE(gw);
+                goto error;
+            }
         }
     }
 
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
index b14b5a1..71cca6f 100644
--- a/src/conf/network_conf.h
+++ b/src/conf/network_conf.h
@@ -160,12 +160,10 @@ struct _virNetworkRouteDef {
      * The virSocketAddrGetIpPrefix() can be used to get a
      * valid prefix.
      */
-    unsigned int prefix;        /* ipv6 - only prefix allowed */
     virSocketAddr netmask;      /* ipv4 - either netmask or prefix specified */
-
-    virSocketAddr gateway;      /* gateway IP address for ip-route */
-
+    unsigned int prefix;        /* ipv6 - only prefix allowed */
     bool has_prefix;            /* prefix= was specified */
+    virSocketAddr gateway;      /* gateway IP address for ip-route */
    };
 
 typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8e76d4a..9ef93f2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1463,6 +1463,7 @@ virMacAddrSetRaw;
 
 
 # util/virnetdev.h
+virNetDevAddRoute;
 virNetDevClearIPv4Address;
 virNetDevExists;
 virNetDevGetIndex;
@@ -1481,7 +1482,6 @@ virNetDevReplaceMacAddress;
 virNetDevReplaceNetConfig;
 virNetDevRestoreMacAddress;
 virNetDevRestoreNetConfig;
-virNetDevSetGateway;
 virNetDevSetIPv4Address;
 virNetDevSetMAC;
 virNetDevSetMTU;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index f4394ed..5598cd2 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2396,61 +2396,23 @@ 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;
-        }
-    }
-    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;
-        }
-    }
-
-    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;
-        }
+    prefix = virSocketAddrGetIpPrefix(&routedef->address,
+                                      &routedef->netmask,
+                                      routedef->prefix);
+    if (prefix < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("network '%s' has an invalid netmask "
+                         "or IP address in route definition"),
+                       network->def->name);
+        return -1;
     }
 
-    if (virNetDevSetGateway(network->def->bridge,
-                            &routedef->address,
-                            prefix,
-                            &routedef->gateway) < 0)
+    if (virNetDevAddRoute(network->def->bridge, &routedef->address,
+                          prefix, &routedef->gateway) < 0) {
         return -1;
+    }
     return 0;
 }
 
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 69a8830..f199f87 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -781,10 +781,11 @@ cleanup:
  * Returns 0 in case of success or -1 in case of error.
  */
 
-int virNetDevSetGateway(const char *ifname,
-                        virSocketAddrPtr addr,
-                        unsigned int prefix,
-                        virSocketAddrPtr gateway)
+int
+virNetDevAddRoute(const char *ifname,
+                  virSocketAddrPtr addr,
+                  unsigned int prefix,
+                  virSocketAddrPtr gateway)
 {
     virCommandPtr cmd = NULL;
     char *addrstr = NULL, *gatewaystr = NULL;
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index c8f20b1..2a61db6 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -42,10 +42,10 @@ int virNetDevSetIPv4Address(const char *ifname,
                             virSocketAddr *addr,
                             unsigned int prefix)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
-int virNetDevSetGateway(const char *ifname,
-                        virSocketAddrPtr addr,
-                        unsigned int prefix,
-                        virSocketAddrPtr gateway)
+int virNetDevAddRoute(const char *ifname,
+                      virSocketAddrPtr addr,
+                      unsigned int prefix,
+                      virSocketAddrPtr gateway)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
 int virNetDevClearIPv4Address(const char *ifname,
                               virSocketAddr *addr,
-- 
1.7.11.7


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