[libvirt] [PATCH-v4.2] Support for static routes on a virtual bridge
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> + ... + <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 + 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); + + 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; + + /* 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'"), + networkName); + goto cleanup; + } + + if (!gateway) { + virReportError(VIR_ERR_XML_ERROR, + _("A gateway address must be specified in route definition '%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'"), + 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'"), + 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; + } + 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); + 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; + + 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); + 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; + * 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)) { + 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; + } + } + } + } + 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); 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)) { + 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 */ + }; + 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; + } + } + 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: + * @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> -- 1.8.1.4
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> + ... + <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).
(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.
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
On 05/04/2013 02:56 PM, Gene Czarcinski wrote:
maybe I found a bug Then again, maybe my old "reverse midas touch" is at it again and I have found yet another bug ... that is, undesirable feature. Everything I have found so far says that ::/0 is the same as specifying "default" for IPv6 and that is valid.
Given what I am seeing in the iproute package, I am going to proceed with the patch for libvirt assuming that prefix=0 is valid for both IPv4 and IPv6, You suggested approach for checking for a zero address was what I wanted to do but was not sure how to do when I was pressed for time. Still need to check for zero address and zero prefix or netmask so that defaults do not kick in. Gene
participants (2)
-
Gene Czarcinski -
Laine Stump