On 04/11/2013 08:11 AM, Gene Czarcinski wrote:
> This patch adds support for adding a static route for
> a network. The "gateway" sub-element specifies
> the gateway's IP address. Both IPv4 and IPv6
> static routes are supported although it is
> expected that this functionality will have
> more use with IPv6.
>
> This updates add the <route> element to define a
> static route. The gateway= subelement species the
> gateway address which is to receive the packets
> forwarded to the network specified on the
> <route> definition.
>
> 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
> has a network defined on this bridge.
s/has a network/is directly reachable via a network/
> 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.
Why is that? What types of "incorrect" gateway address lead to this
condition, and exactly what is "the situation"? I don't think that
should stop putting this patch in, but maybe if we understand the
problem better we can do more to eliminate it in a followup patch.
> 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.
>
> The command used is of the following form:
>
> ip route add <address>/<prefix> via <gateway> dev <virbr-bridge> \
> proto static metric 1
As we've discussed before, we should try to eliminate use of /sbin/ip in
favor of netlink messages, but we're already using /sbin/ip for other
things, so that can be done as a separate followup patch too.
> .
> Signed-off-by: Gene Czarcinski <gene@xxxxxxxxx>
> ---
> docs/formatnetwork.html.in | 90 ++++++
> docs/schemas/network.rng | 22 ++
> src/conf/network_conf.c | 329 ++++++++++++++++++++-
> src/conf/network_conf.h | 8 +
> src/libvirt_private.syms | 1 +
> src/network/bridge_driver.c | 40 +++
> src/util/virnetdev.c | 46 +++
> src/util/virnetdev.h | 5 +
> .../networkxml2xmlin/dhcp6host-routed-network.xml | 4 +
> .../networkxml2xmlout/dhcp6host-routed-network.xml | 4 +
> 10 files changed, 548 insertions(+), 1 deletion(-)
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 4dd0415..9f9304a 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -529,6 +529,63 @@
> starting.
> </p>
>
> + <h5><a name="elementsStaticroute">Static Routes</a></h5>
> + <p>
> + It has been possible to define static routes on a virtual bridge
> + device <span class="since">Since 1.0.5</span>. 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 on the virtualization host.
> + </p>
> +
> + <p>
> + As shown in this example <a href="formatnetwork.html#examplesNoGateway">here</a>,
Put "this example" inside the <a>, and remove "here".
> + 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. However, such interfaces can be used to support
s/host. However, such interfaces/host, but/
> + virtual networks which only have guest connectivity. A guest
> + with connectivity to the internal-only network and another network
s/internal-only/guest-only/
> + with with regular connectivity can act as a gateway between the
s/with with regulat connectivity/that is directly reachable from the host/
> + networks. A static route is used provide the routing information
s/is used provide/added to the "visible" network definition provides/
> + so that IP packets (and especially response packets) can be sent from
s/(and .*)//
> + the virtualization host to guests on the hidden network. While
> + static routes can be useful with IPv4 networks, they are essential
> + in supporting IPv6 on seconday/hidden networks.
I think the last sentence is unnecessary, and not really true - it's
just as essential when dealing with a hidden IPv4 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">
> + </route>
> + <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>
Is there any reason you're putting in a separate </route> if there's no
subelements?
> + ...
> + </pre>
> +
> + <p>
> + Notice that the <code>route</code> elements look very similar to
> + the <code>ip</code> address specification. Although parts are the same,
> + the meaning and effects of the two definitions are very different. The
s/of the two definitions/of the two elements/
> + <code>ip</code> address specification results in an address being defined
> + on the virtual bridge whereas the <code>route</code> definition is used
> + to create the static route for the specified network via the gateway address
> + on the virtual bridge device.
> + </p>
Really. though, I don't think that paragraph is needed.
> +
> <h3><a name="elementsAddress">Addressing</a></h3>
>
> <p>
> @@ -540,6 +597,13 @@
> a forward mode of 'route' or 'nat'.
> </p>
>
> + <p>
> + Another optional element can add a static route definition to
> + the bridge device. The static route is specified by the
> + <code>route</code> element with <code>gateway</code> used to
> + specify the gateway. <span class="since">Since 1.0.5</span>
> + </p>
> +
This was already covered in the Static Routes section above. I don't
think you need to talk about it here.
> <pre>
> ...
> <mac address='00:16:3E:5D:C7:9E'/>
> @@ -560,6 +624,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 +874,31 @@
> </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 rill have a static route defined for this
> + network via the specified gateway. Note that the gateway address
> + must be valid for the networks supported on the bridge interface.
> + <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>
danpb recently pointed out that these examples need to be moved out into
a separate document, but I've misplaced where that is. I guess we can
leave this here until somebody reminds me (and hopefully someone else
has the time to move it :-)
>
> <p>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 6c3fae2..5bcba32 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -305,6 +305,28 @@
> </optional>
> </element>
> </zeroOrMore>
> + <!-- <route> element -->
> + <zeroOrMore>
> + <!-- The (static) route element specifies a network address and gateway
> + address to access that network. -->
> + <element name="route">
> + <optional>
> + <attribute name="family"><ref name="addr-family"/></attribute>
> + </optional>
> + <optional>
> + <attribute name="address"><ref name="ipAddr"/></attribute>
> + </optional>
> + <optional>
> + <choice>
> + <attribute name="netmask"><ref name="ipv4Addr"/></attribute>
> + <attribute name="prefix"><ref name="ipPrefix"/></attribute>
> + </choice>
Should we take this opportunity to retire "netmask"? We'll still have to
keep it in <ip>, but at least we won't have to deal with it in <route>.
Or should we leave it in in the name of keeping the two elements as
similar as possible?
(I guess even if we remove it, we'll still need to check for it so that
we can generate an error, rather than just silently ignoring it...)
> + </optional>
> + <optional>
> + <attribute name="gateway"><ref name="ipAddr"/></attribute>
> + </optional>
> + </element>
> + </zeroOrMore>
> </interleave>
> </element>
> </define>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index c5535e6..15e0ed0 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1280,6 +1280,208 @@ cleanup:
> }
>
> static int
> +virNetworkRouteDefParseXML(const char *networkName,
> + xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virNetworkIpDefPtr def)
Indentation is off here.
Also, you shouldn't be re-using virNetworkIpDefPtr - create a new type
called virNetworkRouteDefPtr (and associated helper functions).
> +{
> + /*
> + * virNetworkRouteDef (reall a virNetworkIpDef) object is already
s/reall/really/
> + * 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;
> + int result = -1;
> + virSocketAddr testAddr;
> +
> + save = ctxt->node;
> + ctxt->node = node;
> +
> + /* grab raw data from XML */
> + def->family = virXPathString("string(./@family)", ctxt);
> + address = virXPathString("string(./@address)", ctxt);
> + if (virXPathULong("string(./@prefix)", ctxt, &prefix) < 0)
> + def->prefix = 0;
You need to save the return value and check for -2 - if the return is -2
that means there was a prefix, but it didn't parse correctly as a ULong.
If it's not -2 but still < 0, then it just wasn't present, so you can
set the default. (I know that's a copy-paste from
virNetworkIpDefParseXML - that's a bug that should be fixed).
> + else
> + def->prefix = prefix;
> +
> + netmask = virXPathString("string(./@netmask)", ctxt);
Hmmm, yeah, I'm thinking it will be safest to accept netmask and just
deal with it. If we ignore it then people will get invalid results, and
if we specifically prohibit it, then we can't change our minds later and
decide to support it. :-/
> +
> + gateway = virXPathString("string(./@gateway)", ctxt);
> +
> + if (address) {
> + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad network address '%s' in route definition of network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> +
> + }
Address is required (unless prefix and netmask are both 0/missing, which
indicates this is a default route). You need to return an error in that
case.
> +
> + if (gateway) {
> + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad gateway address '%s' in route definition of network '%s'"),
> + gateway, networkName);
> + goto cleanup;
> + }
> +
> + }
gateway is *always* required. If it's missing, you need to return an error.
> +
> + /* validate family vs. address and gateway address too*/
> + if (def->family == NULL) {
Really, the validation is the same when def->family is NULL and when
it's "ipv4". Only the error messages will slightly change. Can you
combine this clause with the next?
> + if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) ||
> + VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no family specified for non-IPv4 address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (gateway &&
Since gateway is required, you don't need to see if it's present.
> + (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET) ||
> + VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_UNSPEC)))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no family specified for non-IPv4 gateway address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (def->prefix > 0 && def->prefix > 32) {
The first part of that is redundant - def->prefix > 32 is enough.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid prefix=%u specified for IPv4 address '%s' in network '%s'"),
> + def->prefix, address, networkName);
> + goto cleanup;
> + }
> + } else if (STREQ(def->family, "ipv4")) {
Yeah, this clause - it should be the same as the one above, except that
what's printed in the case of an error is slightly different. (I won't
point out the similar nits in the clause, as I assume it will be
combined with the above)
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("family 'ipv4' specified for non-IPv4 address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (gateway && (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("family 'ipv4' specified for non-IPv4 gateway address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (def->prefix > 0 && def->prefix > 32) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid prefix=%u specified for IPv4 address '%s' in network '%s'"),
> + def->prefix, address, networkName);
> + goto cleanup;
> + }
> + } else if (STREQ(def->family, "ipv6")) {
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("family 'ipv6' specified for non-IPv6 address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (def->prefix == 0 || def->prefix > 64) {
Doesn't prefix='0' imply that this is a default route? And why do you
not allow it to be > 64? You should allow anything up to 128 (for a
single-host route)
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid prefix=%u specified for IPv6 address '%s' in network '%s'"),
> + def->prefix, address, networkName);
> + goto cleanup;
> + }
> + if (netmask) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("specifying netmask invalid for IPv6 address '%s' in network '%s'"),
> + address, networkName);
> + goto cleanup;
> + }
> + if (gateway && (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6))) {
You shouldn't need to check for presence of gateway - it's required and
should have been checked for already.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("family 'ipv6' specified for non-IPv6 gateway address '%s' in network '%s'"),
> + gateway, networkName);
> + goto cleanup;
> + }
> + } else {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Unrecognized family '%s' in definition of network '%s'"),
> + def->family, networkName);
> + goto cleanup;
> + }
> +
> + /* parse/validate netmask */
> + if (netmask) {
> + if (address == NULL) {
> + /* netmask is meaningless without an address */
Not exactly true - It's okay to have a netmask of 0.0.0.0 and no address
(since 0.0.0.0 means "ignore *all* bits of the address). Usually someone
who wanted that effect would simply not specify netmask or prefix, but
they *could*.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask specified without address in network '%s'"),
> + networkName);
> + goto cleanup;
> + }
> +
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
Not true - address could also be unspecified (if netmask was 0.0.0.0).
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask not supported for address '%s' in network '%s' (IPv4 only)"),
> + address, networkName);
> + goto cleanup;
> + }
> +
> + if (def->prefix > 0) {
> + /* can't have both netmask and prefix at the same time */
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("network '%s' cannot have both prefix='%u' and a netmask"),
> + networkName, def->prefix);
> + goto cleanup;
> + }
> +
> + if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0)
> + goto cleanup;
You may actually want to do this parse further up; there is a helper
function to convert from netmask to prefix -
virSocketAddrGetNumNetmaskBits() - which would make checking for
(netmask == "0.0.0.0") simpler.
> +
> + 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 static route gateway specified, make sure the address is a network address */
> + if (def->prefix > 0) {
> + if (virSocketAddrMaskByPrefix(&def->address, def->prefix, &testAddr) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("error converting address '%s' with prefix=%u to network-address in network '%s'"),
> + address, def->prefix, networkName);
> + goto cleanup;
> + }
> + }
> + 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 network '%s'"),
> + address, netmask, networkName);
> + goto cleanup;
> + }
> + }
> + if (!virSocketAddrEqual(&def->address, &testAddr)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("address '%s' in network '%s' is not a network-address"),
> + address, networkName);
> + goto cleanup;
> + }
> +
> + result = 0;
> +
> +cleanup:
> + if (result < 0) {
> + virNetworkIpDefClear(def);
> + }
> + VIR_FREE(address);
> + VIR_FREE(netmask);
> + VIR_FREE(gateway);
> +
> + ctxt->node = save;
> + return result;
> +}
> +
> +static int
> virNetworkPortGroupParseXML(virPortGroupDefPtr def,
> xmlNodePtr node,
> xmlXPathContextPtr ctxt)
> @@ -1676,8 +1878,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;
> @@ -1831,6 +2034,82 @@ 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
> + * address has a network definition on this bridge
> + */
> + nRoutes = def->nroutes;
> + nIps = def->nips;
> + for (ii = 0; ii < nRoutes; ii++) {
> + int jj;
> + virSocketAddr testAddr, testGw;
> + bool addrMatch;
> + virNetworkIpDefPtr 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_IS_FAMILY(&gwdef->gateway, AF_INET6)) &&
> + (!VIR_SOCKET_ADDR_IS_FAMILY(&def2->address, AF_INET6)))
> + continue;
> + if ((VIR_SOCKET_ADDR_IS_FAMILY(&def2->address, AF_INET6)) &&
> + (!VIR_SOCKET_ADDR_IS_FAMILY(&gwdef->gateway, AF_INET6)))
> + continue;
> + if ((VIR_SOCKET_ADDR_IS_FAMILY(&gwdef->gateway, AF_INET) ||
> + VIR_SOCKET_ADDR_IS_FAMILY(&gwdef->gateway, AF_UNSPEC)) &&
> + VIR_SOCKET_ADDR_IS_FAMILY(&def2->address, AF_INET6))
> + continue;
I think you can simplify this. You know that the gateway of every route
is always required, so it will never be AF_UNSPEC, and you know that the
address of every <ip> is also required, so it will never be AF_UNSPEC.
So, you can just say
if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway)
!= VIR_SOCKET_ADDR_FAMILY(&def2->address)) {
continue;
}
> + if (def2->prefix > 0) {
> + virSocketAddrMaskByPrefix(&def2->address, def2->prefix, &testAddr);
> + virSocketAddrMaskByPrefix(&gwdef->gateway, def2->prefix, &testGw);
> + }
> + if (VIR_SOCKET_ADDR_VALID(&def2->netmask)) {
> + virSocketAddrMask(&def2->address, &def2->netmask, &testAddr);
> + virSocketAddrMask(&gwdef->gateway, &def2->netmask, &testGw);
> + }
If no explicit prefix was given, you haven't set testAddr or testGw,
i.e. you havne't accounted for someone relying on the "natural" netmask.
Instead of the two clauses above, you should use this:
int prefix = virNetworkIpDefPrefix(def2);
virSocketAddrMask(&def2->address, prefix, &testAddr);
virSocketAddrMask(&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,
> + _("invalid static route gateway specified: '%s'"),
> + gw);
"unreachable static route gateway '%s' specified for network '%s'"
> + VIR_FREE(gw);
> + goto error;
> + }
> + }
> + }
> + }
> + VIR_FREE(routeNodes);
> +
> forwardNode = virXPathNode("./forward", ctxt);
> if (forwardNode &&
> virNetworkForwardDefParseXML(def->name, forwardNode, ctxt, &def->forward) < 0) {
> @@ -2203,6 +2482,49 @@ error:
> }
>
> static int
> +virNetworkRouteDefFormat(virBufferPtr buf,
> + const virNetworkIpDefPtr 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->prefix > 0) {
> + 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");
> + virBufferAddLit(buf, "</route>\n");
You should replace the above two lines with:
virBufferAddLit(buf, "/>\n");
Since there are never any subelements, there is no reason for a separate
closing token.
> +
> + result = 0;
> +error:
> + return result;
> +}
> +
> +static int
> virPortGroupDefFormat(virBufferPtr buf,
> const virPortGroupDefPtr def)
> {
> @@ -2400,6 +2722,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 6ce9a00..39aec7e 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -109,6 +109,8 @@ struct _virNetworkDNSDef {
> virNetworkDNSSrvDefPtr srvs;
> };
>
> +/* NOTE: The folowing structure is used for both IP address
> + * definitions and for static route definitions */
Don't do that - it always ends up leading to headaches/confusion later.
Just create a new struct called virNetworkRoutDef that has only the
fields needed for a <route> (family, address, prefix, netmask, and gateway).
> typedef struct _virNetworkIpDef virNetworkIpDef;
> typedef virNetworkIpDef *virNetworkIpDefPtr;
> struct _virNetworkIpDef {
> @@ -124,6 +126,9 @@ struct _virNetworkIpDef {
> unsigned int prefix; /* ipv6 - only prefix allowed */
> virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */
>
> + virSocketAddr gateway; /* gateway IP address for ip-route */
> +
> + /* the following has no meaning if this is a static route definition */
> size_t nranges; /* Zero or more dhcp ranges */
> virSocketAddrRangePtr ranges;
>
> @@ -209,6 +214,9 @@ struct _virNetworkDef {
> size_t nips;
> virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */
>
> + size_t nroutes;
> + virNetworkIpDefPtr 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 96eea0a..c51ba36 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1466,6 +1466,7 @@ virNetDevReplaceMacAddress;
> virNetDevReplaceNetConfig;
> virNetDevRestoreMacAddress;
> virNetDevRestoreNetConfig;
> +virNetDevSetGateway;
> virNetDevSetIPv4Address;
> virNetDevSetMAC;
> virNetDevSetMTU;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index e8b314a..2d4102e 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2360,6 +2360,7 @@ out:
> return ret;
> }
>
> +/* add an IP address to a bridge */
> static int
> networkAddAddrToBridge(virNetworkObjPtr network,
> virNetworkIpDefPtr ipdef)
> @@ -2380,6 +2381,28 @@ networkAddAddrToBridge(virNetworkObjPtr network,
> return 0;
> }
>
> +/* add an IP (static) route to a bridge */
> +static int
> +networkAddRouteToBridge(virNetworkObjPtr network,
> + virNetworkIpDefPtr ipdef)
> +{
> + int prefix = virNetworkIpDefPrefix(ipdef);
Since you will be changing virNetworkIpDefPtr to virNetworkRouteDefPtr,
you will no longer be able to call virNetworkIpDefPrefix() as it's
currently defined. To fix this without duplicating code, I think you
need to change virNetworkIpDefPrefix into something like this, and put
it in virsocketaddr.c:
int virSocketAddrGetIpPrefix(const virSocketAddr *addr, const
virSocketAddr *netmask, int prefix)
{
if (prefix > 0) {
return prefix;
} else if (VIR_SOCKET_ADDR_VALID(netmask)) {
return virSocketAddrGetNumNetmaskBits(netmask);
} else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)) {
/* Return the natural prefix for the network's ip address.
* On Linux we could use the IN_CLASSx() macros, but those
* aren't guaranteed on all platforms, so we just deal with
* the bits ourselves.
*/
unsigned char octet
= ntohl(address->data.inet4.sin_addr.s_addr) >> 24;
if ((octet & 0x80) == 0) {
/* Class A network */
return 8;
} else if ((octet & 0xC0) == 0x80) {
/* Class B network */
return 16;
} else if ((octet & 0xE0) == 0xC0) {
/* Class C network */
return 24;
}
return -1;
} else if (VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET6)) {
return 64;
}
return -1;
}
(pretty much all I did was remove "&def->" from the original function)
Then change all the original function's callers appropriately (or if you
want, just have virNetworkIpDefPrefix() call virSocketAddrGetIpPrefix()).
> +
> + if (prefix < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("bridge '%s' has an invalid netmask or IP address"),
> + network->def->bridge);
> + return -1;
> + }
> +
> + if (virNetDevSetGateway(network->def->bridge,
> + &ipdef->address,
> + prefix,
> + &ipdef->gateway) < 0)
> + return -1;
> + return 0;
> +}
> +
> static int
> networkStartNetworkVirtual(struct network_driver *driver,
> virNetworkObjPtr network)
> @@ -2446,6 +2469,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
> if (networkAddIptablesRules(driver, network) < 0)
> goto err1;
>
> + /* first, do all of the real addresses */
> for (ii = 0;
> (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
> ii++) {
> @@ -2464,6 +2488,22 @@ networkStartNetworkVirtual(struct network_driver *driver,
> if (virNetDevSetOnline(network->def->bridge, 1) < 0)
> goto err2;
>
> + /* now, do all of the static routes */
> + if (network->def->nroutes > 0) {
You don't need this outer if - the for loop takes care of it for you.
> + for (ii = 0; ii < network->def->nroutes; ii++) {
> + ipdef = &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(&ipdef->gateway)) {
> + if (networkAddRouteToBridge(network, ipdef) < 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 00e0f94..6ac9d9d 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -769,6 +769,52 @@ 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,
> + virSocketAddr *addr,
> + unsigned int prefix,
> + virSocketAddr *gateway)
In both cases use virSocketAddrPtr instead of virSocketAddr *.
> +{
> + 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", NULL);
> + virCommandAddArgFormat(cmd, "%s", gatewaystr);
> + virCommandAddArgList(cmd, "dev", ifname, NULL);
> + virCommandAddArgList(cmd, "proto", "static", "metric", NULL);
The previous 4 lines could be done with:
virCommandAddArgList(cmd, "via", gatewaystr, "dev", "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 06d0650..8b94ea8 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,
> + virSocketAddr *addr,
> + unsigned int prefix,
> + virSocketAddr *gateway)
Use virSocketAddrPtr instead of virSocketAddr * here too.
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
Why are you guaranteeing that arg1 and arg2 are NONNULL, but not arg4?
> 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..29e9561 100644
> --- a/tests/networkxml2xmlin/dhcp6host-routed-network.xml
> +++ b/tests/networkxml2xmlin/dhcp6host-routed-network.xml
> @@ -19,4 +19,8 @@
> <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>
You'll want to change all of these once you remove the extra </route>
from the formatting function.
> + <route family="ipv6" address="2001:db8:ac10:fc00::" prefix="64" gateway="2001:db8:ac10:fd01::1:24">
> + </route>
> </network>
> diff --git a/tests/networkxml2xmlout/dhcp6host-routed-network.xml b/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> index 7305043..39af583 100644
> --- a/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> +++ b/tests/networkxml2xmlout/dhcp6host-routed-network.xml
> @@ -21,4 +21,8 @@
> <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>
> + <route family='ipv6' address='2001:db8:ac10:fc00::' prefix='64' gateway='2001:db8:ac10:fd01::1:24'>
> + </route>
> </network>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list