[libvirt] [PATCH-v4.2] Support for static routes on a virtual bridge
Laine Stump
laine at laine.org
Mon Apr 29 15:38:25 UTC 2013
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 at 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>
> + ...
> + <ip address="192.168.122.1" netmask="255.255.255.0">
> + <dhcp>
> + <range start="192.168.122.128" end="192.168.122.254" />
> + </dhcp>
> + </ip>
> + <route address="192.168.222.0" prefix="24" gateway="192.168.122.2" />
> + <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" />
> + <route family="ipv6" address="2001:db8:ca2:3::" prefix="64" gateway="2001:db8:ca2:2::2">
> + </route>
> + ...
> + </pre>
> +
> <h3><a name="elementsAddress">Addressing</a></h3>
>
> <p>
> @@ -560,6 +604,7 @@
> </dhcp>
> </ip>
> <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" />
> + <route family="ipv6" address="2001:db9:ca1:1::" prefix="64" gateway="2001:db8:ca2:2::2" />
> </network></pre>
>
> <dl>
> @@ -809,6 +854,32 @@
> </ip>
> </network></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
> + <ip> addresses defined for this <network>.
> + <span class="since">Since 1.0.5</span>
> + </p>
> +
> + <pre>
> + <network>
> + <name>net7</name>
> + <bridge name="virbr7" />
> + <forward mode="route"/>
> + <ip family="ipv6" address="2001:db8:ca2:7::1" prefix="64" >
> + <dhcp>
> + <range start="2001:db8:ca2:7::100" end="2001:db8:ca2::1ff" />
> + <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" />
> + </dhcp>
> + </ip>
> + <route family="ipv6" address="2001:db8:ca2:8::" prefix="64" gateway="2001:db8:ca2:7::4" >
> + </route>
> + </network></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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SQUASH-THIS-COMMIT.patch
Type: text/x-patch
Size: 21200 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130429/440d648d/attachment-0001.bin>
More information about the libvir-list
mailing list