On 01/09/2015 11:47 AM, Cédric Bosdonnat wrote:
> Made the network configuration schemas and codes for the route element reusable.
> Created networkcommon_conf.[ch] files containing pieces to be used in both domain
> and network configurations.
>
> Replaced the brand new domain route configuration by the network one. Note that it
> now forces the destination address to be defined, even if the user wants to define
> the default gateway.
> ---
> docs/formatdomain.html.in | 14 +-
> docs/schemas/basictypes.rng | 2 +-
> docs/schemas/domaincommon.rng | 29 +-
> docs/schemas/network.rng | 20 +-
> docs/schemas/networkcommon.rng | 22 ++
> po/POTFILES.in | 1 +
> src/Makefile.am | 3 +-
> src/conf/domain_conf.c | 117 ++------
> src/conf/domain_conf.h | 14 +-
> src/conf/network_conf.c | 284 +-----------------
> src/conf/network_conf.h | 20 +-
> src/conf/networkcommon_conf.c | 337 ++++++++++++++++++++++
> src/conf/networkcommon_conf.h | 76 +++++
> src/libvirt_private.syms | 7 +
> src/lxc/lxc_container.c | 22 +-
> src/lxc/lxc_native.c | 26 +-
> tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml | 4 +-
> tests/lxcconf2xmldata/lxcconf2xml-simple.xml | 4 +-
> tests/lxcxml2xmldata/lxc-hostdev.xml | 4 +-
> tests/lxcxml2xmldata/lxc-idmap.xml | 4 +-
> 20 files changed, 542 insertions(+), 468 deletions(-)
> create mode 100644 src/conf/networkcommon_conf.c
> create mode 100644 src/conf/networkcommon_conf.h
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 499879e..292c8c2 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4329,8 +4329,8 @@ qemu-kvm -net nic,model=? /dev/null
> <source network='default'/>
> <target dev='vnet0'/>
> <b><ip address='192.168.122.5' prefix='24'/></b>
> - <b><route family='ipv4' address='192.168.122.0' prefix='24' via='192.168.122.1'/></b>
> - <b><route family='ipv4' via='192.168.122.1'/></b>
> + <b><route family='ipv4' address='192.168.122.0' prefix='24' gateway='192.168.122.1'/></b>
> + <b><route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/></b>
> </interface>
> ...
> <hostdev mode='capabilities' type='net'>
> @@ -4338,8 +4338,8 @@ qemu-kvm -net nic,model=? /dev/null
> <interface>eth0</interface>
> </source>
> <b><ip address='192.168.122.6' prefix='24'/></b>
> - <b><route family='ipv4' address='192.168.122.0' prefix='24' via='192.168.122.1'/></b>
> - <b><route family='ipv4' via='192.168.122.1'/></b>
> + <b><route family='ipv4' address='192.168.122.0' prefix='24' gateway='192.168.122.1'/></b>
> + <b><route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/></b>
> </hostdev>
>
> </devices>
> @@ -4359,11 +4359,7 @@ qemu-kvm -net nic,model=? /dev/null
> <p>
> <span class="since">Since 1.2.12</span> route elements can also be added
> to define the network routes to use for the network device. This element
> - has a <code>family</code> attribute set either to <code>ipv4</code> or
> - <code>ipv6</code>, a mandatory <code>via</code> attribute defining the
> - IP address to route throught and optional <code>address</code> and <code>prefix</code>
> - attributes defining the target network range. If those aren't given, then
> - a default route will be set.
> + has the same attributes than its equivalent for network definitions.
This attributes of this element are described in the documentation for
the <route> element in network definitions.
(with an appropriate link pointing to formatnetwork.html)
> This is only used by the LXC driver.
> </p>
>
> diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
> index 9ddd92b..efc9da4 100644
> --- a/docs/schemas/basictypes.rng
> +++ b/docs/schemas/basictypes.rng
> @@ -174,7 +174,7 @@
> <define name="ipv6Addr">
> <data type="string">
> <!-- To understand this better, take apart the toplevel "|"s -->
> -<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>
> +<param name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)|(::)</param>
Hee hee. ACK because I know the discussion that led to the change. This
really should be in a separate patch though, since it's fixing a bug and
this regexp is used in other places than just route definitions.
> </data>
> </define>
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 879e064..548cd15 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2330,9 +2330,7 @@
> </element>
> </zeroOrMore>
> <zeroOrMore>
> - <element name="route">
> - <ref name="route"/>
> - </element>
> + <ref name="route"/>
> </zeroOrMore>
> <optional>
> <element name="script">
> @@ -3602,27 +3600,6 @@
> </element>
> </define>
>
> - <define name="route">
> - <interleave>
> - <attribute name="family">
> - <ref name="addr-family"/>
> - </attribute>
> - <attribute name="via">
> - <ref name="ipAddr"/>
> - </attribute>
> - <optional>
> - <attribute name="address">
> - <ref name="ipAddr"/>
> - </attribute>
> - </optional>
> - <optional>
> - <attribute name="prefix">
> - <ref name="ipPrefix"/>
> - </attribute>
> - </optional>
> - </interleave>
> - </define>
> -
> <define name="hostdev">
> <element name="hostdev">
> <interleave>
> @@ -3859,9 +3836,7 @@
> </element>
> </zeroOrMore>
> <zeroOrMore>
> - <element name="route">
> - <ref name="route"/>
> - </element>
> + <ref name="route"/>
> </zeroOrMore>
> </interleave>
> </define>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 9a7d156..a8814e7 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -371,25 +371,7 @@
> </zeroOrMore>
> <!-- <route> element -->
> <zeroOrMore>
> - <!-- The (static) route element specifies a network address and gateway
> - address to access that network. Both 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>
> - <optional>
> - <attribute name="metric"><ref name="unsignedInt"/></attribute>
> - </optional>
> - </element>
> + <ref name='route'/>
> </zeroOrMore>
> </interleave>
> </element>
> diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng
> index e26b7f3..162ea3d 100644
> --- a/docs/schemas/networkcommon.rng
> +++ b/docs/schemas/networkcommon.rng
> @@ -224,4 +224,26 @@
> <param name='maxInclusive'>65535</param>
> </data>
> </define>
> +
> + <!-- The (static) route element specifies a network address and gateway
> + address to access that network. Both the network address and
> + the gateway address must be specified. -->
> + <define name='route'>
> + <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>
> + <optional>
> + <attribute name="metric"><ref name="unsignedInt"/></attribute>
> + </optional>
> + </element>
> + </define>
I would have preferred the changes to move this into a separate ref be a
separate patch, but understand that this would have required temporarily
naming it something different (because you already defined a 'route' in
domaincommon.rng that you need to get rid of). (the reason for wanting a
separate patch? It would potentially make it easier to resolve conflicts
when backporting other stuff that touches the same lines, by just
including the code movement patch in the backport.)
I suppose this could be done by having one patch that renamed the
'route' in domaincommon.rng to "routex", then a 2nd patch that does the
code movement and defines "route" in networkcommon.rng, and then the 3rd
patch that changes domain_conf (and domaincommon.rng) to use the new
"route". Unless someone else also thinks this separation is important,
though, don't spend time on it.
> </grammar>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 094c8e3..3064037 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -25,6 +25,7 @@ src/conf/netdev_bandwidth_conf.c
> src/conf/netdev_vlan_conf.c
> src/conf/netdev_vport_profile_conf.c
> src/conf/network_conf.c
> +src/conf/networkcommon_conf.c
> src/conf/node_device_conf.c
> src/conf/numatune_conf.c
> src/conf/nwfilter_conf.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2eaaf11..ba4c6ef 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -287,7 +287,8 @@ NETWORK_EVENT_SOURCES = \
>
> # Network driver generic impl APIs
> NETWORK_CONF_SOURCES = \
> - conf/network_conf.c conf/network_conf.h
> + conf/network_conf.c conf/network_conf.h \
> + conf/networkcommon_conf.c conf/networkcommon_conf.h
>
> # Network filter driver generic impl APIs
> NWFILTER_PARAM_CONF_SOURCES = \
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 57e99e6..8e34fd6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2006-2008 Daniel P. Berrange
> + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -1475,11 +1476,13 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> VIR_FREE(def->ips[i]);
> VIR_FREE(def->ips);
>
> - for (i = 0; i < def->nroutes; i++)
> + for (i = 0; i < def->nroutes; i++) {
> + virNetworkRouteDefClear(def->routes[i]);
> VIR_FREE(def->routes[i]);
> + }
> VIR_FREE(def->routes);
>
> - virDomainDeviceInfoClear(&def->info);
> + virDomainDeviceInfoClear(&def->info);
>
> VIR_FREE(def->filter);
> virNWFilterHashTableFree(def->filterparams);
> @@ -4838,64 +4841,6 @@ virDomainNetIpParseXML(xmlNodePtr node)
> return NULL;
> }
>
> -static virDomainNetRouteDefPtr
> -virDomainNetRouteParse(xmlNodePtr node)
> -{
> - virDomainNetRouteDefPtr route = NULL;
> - char *familyStr = NULL;
> - int family = AF_UNSPEC;
> - char *via = NULL;
> - char *to = NULL;
> - char *prefixStr = NULL;
> -
> - to = virXMLPropString(node, "address");
> - if (!(via = virXMLPropString(node, "via"))) {
> - virReportError(VIR_ERR_INVALID_ARG, "%s",
> - _("Missing route address"));
> - goto error;
> - }
> -
> - familyStr = virXMLPropString(node, "family");
> - if (familyStr && STREQ(familyStr, "ipv4"))
> - family = AF_INET;
> - else if (familyStr && STREQ(familyStr, "ipv6"))
> - family = AF_INET6;
> - else
> - family = virSocketAddrNumericFamily(via);
> -
> - if (VIR_ALLOC(route) < 0)
> - goto error;
> -
> - if (virSocketAddrParse(&route->via, via, family) < 0) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("Failed to parse IP address: '%s'"),
> - via);
> - goto error;
> - }
> -
> - if (to && virSocketAddrParse(&route->to, to, family) < 0) {
> - virReportError(VIR_ERR_INVALID_ARG,
> - _("Failed to parse IP address: '%s'"),
> - to);
> - goto error;
> - }
> -
> - if (!(prefixStr = virXMLPropString(node, "prefix")) ||
> - (virStrToLong_ui(prefixStr, NULL, 10, &route->prefix) < 0)) {
> - }
> -
> - return route;
> -
> - error:
> - VIR_FREE(familyStr);
> - VIR_FREE(via);
> - VIR_FREE(to);
> - VIR_FREE(prefixStr);
> - VIR_FREE(route);
> -
> - return NULL;
> -}
> -
> static int
> virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED,
> xmlXPathContextPtr ctxt,
> @@ -4989,13 +4934,20 @@ virDomainHostdevDefParseXMLCaps(xmlNodePtr node ATTRIBUTE_UNUSED,
> if (nroutenodes) {
> size_t i;
> for (i = 0; i < nroutenodes; i++) {
> - virDomainNetRouteDefPtr route = virDomainNetRouteParse(routenodes[i]);
> + virNetworkRouteDefPtr route = NULL;
>
> - if (!route)
> + if (VIR_ALLOC(route) < 0)
> goto error;
>
> + if (virNetworkRouteDefParseXML(_("Domain hostdev device"), routenodes[i],
> + ctxt, route) < 0) {
> + VIR_FREE(route);
> + goto error;
> + }
> +
> if (VIR_APPEND_ELEMENT(def->source.caps.u.net.routes,
> def->source.caps.u.net.nroutes, route) < 0) {
> + virNetworkRouteDefClear(route);
> VIR_FREE(route);
> goto error;
> }
> @@ -7456,7 +7408,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> size_t nips = 0;
> virDomainNetIpDefPtr *ips = NULL;
> size_t nroutes = 0;
> - virDomainNetRouteDefPtr *routes = NULL;
> + virNetworkRouteDefPtr *routes = NULL;
>
> if (VIR_ALLOC(def) < 0)
> return NULL;
> @@ -7554,12 +7506,21 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> if (VIR_APPEND_ELEMENT(ips, nips, ip) < 0)
> goto error;
> } else if (xmlStrEqual(cur->name, BAD_CAST "route")) {
> - virDomainNetRouteDefPtr route = NULL;
> - if (!(route = virDomainNetRouteParse(cur)))
> + virNetworkRouteDefPtr route = NULL;
> +
> + if (VIR_ALLOC(route) < 0)
> goto error;
>
> - if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0)
> + if (virNetworkRouteDefParseXML(_("Domain interface"), cur, ctxt, route) < 0) {
> + VIR_FREE(route);
> + goto error;
> + }
> +
> + if (VIR_APPEND_ELEMENT(routes, nroutes, route) < 0) {
> + virNetworkRouteDefClear(route);
> + VIR_FREE(route);
> goto error;
> + }
> } else if (!ifname &&
> xmlStrEqual(cur->name, BAD_CAST "target")) {
> ifname = virXMLPropString(cur, "dev");
> @@ -17285,32 +17246,14 @@ virDomainNetIpsFormat(virBufferPtr buf, virDomainNetIpDefPtr *ips, size_t nips)
>
> static void
> virDomainNetRoutesFormat(virBufferPtr buf,
> - virDomainNetRouteDefPtr *routes,
> + virNetworkRouteDefPtr *routes,
> size_t nroutes)
> {
> size_t i;
>
> for (i = 0; i < nroutes; i++) {
> - virDomainNetRouteDefPtr route = routes[i];
> - const char *familyStr = NULL;
> - char *via = virSocketAddrFormat(&route->via);
> - char *to = NULL;
> -
> - if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET6))
> - familyStr = "ipv6";
> - else if (VIR_SOCKET_ADDR_IS_FAMILY(&route->via, AF_INET))
> - familyStr = "ipv4";
> - virBufferAsprintf(buf, "<route family='%s' via='%s'", familyStr, via);
> -
> - if (VIR_SOCKET_ADDR_VALID(&route->to)) {
> - to = virSocketAddrFormat(&route->to);
> - virBufferAsprintf(buf, " address='%s'", to);
> - }
> -
> - if (route->prefix > 0)
> - virBufferAsprintf(buf, " prefix='%d'", route->prefix);
> -
> - virBufferAddLit(buf, "/>\n");
> + virNetworkRouteDefPtr route = routes[i];
> + virNetworkRouteDefFormat(buf, route);
> }
> }
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ac1f4f8..0828d07 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2006-2014 Red Hat, Inc.
> * Copyright (C) 2006-2008 Daniel P. Berrange
> + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -35,6 +36,7 @@
> # include "virthread.h"
> # include "virhash.h"
> # include "virsocketaddr.h"
> +# include "networkcommon_conf.h"
> # include "nwfilter_params.h"
> # include "numatune_conf.h"
> # include "virnetdevmacvlan.h"
> @@ -485,14 +487,6 @@ struct _virDomainNetIpDef {
> unsigned int prefix; /* number of 1 bits in the net mask */
> };
>
> -typedef struct _virDomainNetRouteDef virDomainNetRouteDef;
> -typedef virDomainNetRouteDef *virDomainNetRouteDefPtr;
> -struct _virDomainNetRouteDef {
> - virSocketAddr via;
> - virSocketAddr to;
> - unsigned int prefix;
> -};
> -
> typedef struct _virDomainHostdevCaps virDomainHostdevCaps;
> typedef virDomainHostdevCaps *virDomainHostdevCapsPtr;
> struct _virDomainHostdevCaps {
> @@ -509,7 +503,7 @@ struct _virDomainHostdevCaps {
> size_t nips;
> virDomainNetIpDefPtr *ips;
> size_t nroutes;
> - virDomainNetRouteDefPtr *routes;
> + virNetworkRouteDefPtr *routes;
> } net;
> } u;
> };
> @@ -1013,7 +1007,7 @@ struct _virDomainNetDef {
> size_t nips;
> virDomainNetIpDefPtr *ips;
> size_t nroutes;
> - virDomainNetRouteDefPtr *routes;
> + virNetworkRouteDefPtr *routes;
> };
>
> /* Used for prefix of ifname of any network name generated dynamically
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index ddb5c07..33392a7 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -163,12 +163,6 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
> }
>
> static void
> -virNetworkRouteDefClear(virNetworkRouteDefPtr def)
> -{
> - VIR_FREE(def->family);
> -}
> -
> -static void
> virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def)
> {
> VIR_FREE(def->name);
> @@ -1371,232 +1365,6 @@ virNetworkIPDefParseXML(const char *networkName,
> }
>
> 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, metric = 0;
> - int result = -1;
> - int prefixRc, metricRc;
> - 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 of network '%s'"),
> - networkName);
> - goto cleanup;
> - }
> - def->has_prefix = (prefixRc == 0);
> - def->prefix = prefix;
> - metricRc = virXPathULong("string(./@metric)", ctxt, &metric);
> - if (metricRc == -2) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid metric specified "
> - "in route definition of network '%s'"),
> - networkName);
> - goto cleanup;
> - }
> - if (metricRc == 0) {
> - def->has_metric = true;
> - if (metric == 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid metric value, must be > 0 "
> - "in route definition of network '%s'"),
> - networkName);
> - goto cleanup;
> - }
> - }
> - def->metric = metric;
> -
> - /* Note: both network and gateway addresses must be specified */
> -
> - if (!address) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Missing required address attribute "
> - "in route definition of network '%s'"),
> - networkName);
> - goto cleanup;
> - }
> -
> - if (!gateway) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("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 of network '%s'"),
> - address, networkName);
> - goto cleanup;
> - }
> -
> - if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Bad gateway address '%s' "
> - "in route definition of network '%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_XML_ERROR,
> - def->family == NULL ?
> - _("No family specified for non-IPv4 address '%s' "
> - "in route definition of network '%s'") :
> - _("IPv4 family specified for non-IPv4 address '%s' "
> - "in route definition of network '%s'"),
> - address, networkName);
> - goto cleanup;
> - }
> - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - def->family == NULL ?
> - _("No family specified for non-IPv4 gateway '%s' "
> - "in route definition of network '%s'") :
> - _("IPv4 family specified for non-IPv4 gateway '%s' "
> - "in route definition of network '%s'"),
> - 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 of network '%s'"),
> - netmask, networkName);
> - goto cleanup;
> - }
> - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("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_XML_ERROR,
> - _("Route definition '%s' cannot have both "
> - "a prefix and a netmask"),
> - networkName);
> - goto cleanup;
> - }
> - }
> - if (def->prefix > 32) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid prefix %u specified "
> - "in route definition of network '%s', "
> - "must be 0 - 32"),
> - def->prefix, networkName);
> - goto cleanup;
> - }
> - } else if (STREQ(def->family, "ipv6")) {
> - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("ipv6 family specified for non-IPv6 address '%s' "
> - "in route definition of network '%s'"),
> - address, networkName);
> - goto cleanup;
> - }
> - if (netmask) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Specifying netmask invalid for IPv6 address '%s' "
> - "in route definition of network '%s'"),
> - address, networkName);
> - goto cleanup;
> - }
> - if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("ipv6 specified for non-IPv6 gateway address '%s' "
> - "in route definition of network '%s'"),
> - gateway, networkName);
> - goto cleanup;
> - }
> - if (def->prefix > 128) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Invalid prefix %u specified "
> - "in route definition of network '%s', "
> - "must be 0 - 128"),
> - def->prefix, networkName);
> - goto cleanup;
> - }
> - } else {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("Unrecognized family '%s' "
> - "in route definition of network'%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_INTERNAL_ERROR,
> - _("error converting address '%s' with netmask '%s' "
> - "to network-address "
> - "in route definition of network '%s'"),
> - address, netmask, networkName);
> - goto cleanup;
> - }
> - } else {
> - if (virSocketAddrMaskByPrefix(&def->address,
> - def->prefix, &testAddr) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("error converting address '%s' with prefix %u "
> - "to network-address "
> - "in route definition of network '%s'"),
> - address, def->prefix, networkName);
> - goto cleanup;
> - }
> - }
> - if (!virSocketAddrEqual(&def->address, &testAddr)) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("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;
> -}
> -
Having all this code moved to another file has potential for creating a
conflict if a future bugfix is backported to a stable branch prior to
this patch - that's why I suggest putting the code movement into a
separate patch; just backport the code movement to the branch as a
prerequisite to the bugfix and the merge conflict is gone (which means
nobody has to handwrite "backported" bug fixes).
> -static int
> virNetworkPortGroupParseXML(virPortGroupDefPtr def,
> xmlNodePtr node,
> xmlXPathContextPtr ctxt)
> @@ -2026,6 +1794,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> xmlNodePtr save = ctxt->node;
> xmlNodePtr bandwidthNode = NULL;
> xmlNodePtr vlanNode;
> + char *errorDetail = NULL;
>
> if (VIR_ALLOC(def) < 0)
> return NULL;
> @@ -2037,6 +1806,9 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> goto error;
> }
>
> + if (virAsprintf(&errorDetail, _("Network '%s'"), def->name) < 0)
> + goto error;
> +
> /* Extract network uuid */
> tmp = virXPathString("string(./uuid[1])", ctxt);
> if (!tmp) {
> @@ -2210,7 +1982,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> goto error;
> /* parse each definition */
> for (i = 0; i < nRoutes; i++) {
> - if (virNetworkRouteDefParseXML(def->name,
> + if (virNetworkRouteDefParseXML(errorDetail,
> routeNodes[i],
> ctxt,
> &def->routes[i]) < 0)
> @@ -2336,6 +2108,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> }
>
> VIR_FREE(stp);
> + VIR_FREE(errorDetail);
> ctxt->node = save;
> return def;
>
> @@ -2349,6 +2122,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> VIR_FREE(ipv6nogwStr);
> VIR_FREE(trustGuestRxFilters);
> ctxt->node = save;
> + VIR_FREE(errorDetail);
> return NULL;
> }
>
> @@ -2586,50 +2360,6 @@ virNetworkIpDefFormat(virBufferPtr buf,
> }
>
> static int
> -virNetworkRouteDefFormat(virBufferPtr buf,
> - const virNetworkRouteDef *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);
> - }
> - if (def->has_metric && def->metric > 0)
> - virBufferAsprintf(buf, " metric='%u'", def->metric);
> - virBufferAddLit(buf, "/>\n");
> -
> - result = 0;
> - error:
> - return result;
> -}
> -
> -static int
> virPortGroupDefFormat(virBufferPtr buf,
> const virPortGroupDef *def)
> {
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 8110028..8cce28a 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -38,6 +38,7 @@
> # include "virnetdevvlan.h"
> # include "virmacaddr.h"
> # include "device_conf.h"
> +# include "networkcommon_conf.h"
> # include "virbitmap.h"
>
> typedef enum {
> @@ -162,25 +163,6 @@ 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.
> - */
> - virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */
> - unsigned int prefix; /* ipv6 - only prefix allowed */
> - bool has_prefix; /* prefix= was specified */
> - unsigned int metric; /* value for metric (defaults to 1) */
> - bool has_metric; /* metric= was specified */
> - virSocketAddr gateway; /* gateway IP address for ip-route */
> - };
> -
> typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef;
> typedef virNetworkForwardIfDef *virNetworkForwardIfDefPtr;
> struct _virNetworkForwardIfDef {
> diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c
> new file mode 100644
> index 0000000..da80c0f
> --- /dev/null
> +++ b/src/conf/networkcommon_conf.c
> @@ -0,0 +1,337 @@
> +/*
> + * networkcommon_conf.c: network XML handling
> + *
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange@xxxxxxxxxx>
> + */
> +
> +#include <config.h>
> +
> +#include "virerror.h"
> +#include "datatypes.h"
> +#include "networkcommon_conf.h"
> +#include "viralloc.h"
> +#include "virxml.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NETWORK
> +
> +void
> +virNetworkRouteDefClear(virNetworkRouteDefPtr def)
> +{
> + VIR_FREE(def->family);
> +}
> +
> +int
> +virNetworkRouteDefCreate(const char *errorDetail,
> + char *family,
> + char *address,
> + char *netmask,
> + char *gateway,
> + unsigned int prefix,
> + bool hasPrefix,
> + unsigned int metric,
> + bool hasMetric,
> + virNetworkRouteDefPtr def)
> +{
> + virSocketAddr testAddr;
> + int result = -1;
> +
> + def->family = family;
> + def->prefix = prefix;
> + def->has_prefix = hasPrefix;
> + def->metric = metric;
> + def->has_metric = hasMetric;
> +
> + /* Note: both network and gateway addresses must be specified */
> +
> + if (!address) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Missing required address attribute "
> + "in route definition"),
> + errorDetail);
> + goto cleanup;
> + }
> +
> + if (!gateway) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Missing required gateway attribute "
> + "in route definition"),
> + errorDetail);
> + goto cleanup;
> + }
> +
> + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Bad network address '%s' "
> + "in route definition"),
> + errorDetail, address);
> + goto cleanup;
> + }
> +
> + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Bad gateway address '%s' "
> + "in route definition"),
> + errorDetail, gateway);
> + 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_XML_ERROR,
> + def->family == NULL ?
> + _("%s: No family specified for non-IPv4 address '%s' "
> + "in route definition") :
> + _("%s: IPv4 family specified for non-IPv4 address '%s' "
> + "in route definition"),
> + errorDetail, address);
> + goto cleanup;
> + }
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + def->family == NULL ?
> + _("%s: No family specified for non-IPv4 gateway '%s' "
> + "in route definition") :
> + _("%s: IPv4 family specified for non-IPv4 gateway '%s' "
> + "in route definition"),
> + errorDetail, address);
> + goto cleanup;
> + }
> + if (netmask) {
> + if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Bad netmask address '%s' "
> + "in route definition"),
> + errorDetail, netmask);
> + goto cleanup;
> + }
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Invalid netmask '%s' "
> + "for address '%s' (both must be IPv4)"),
> + errorDetail, netmask, address);
> + goto cleanup;
> + }
> + if (def->has_prefix) {
> + /* can't have both netmask and prefix at the same time */
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Route definition cannot have both "
> + "a prefix and a netmask"),
> + errorDetail);
> + goto cleanup;
> + }
> + }
> + if (def->prefix > 32) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Invalid prefix %u specified "
> + "in route definition, "
> + "must be 0 - 32"),
> + errorDetail, def->prefix);
> + goto cleanup;
> + }
> + } else if (STREQ(def->family, "ipv6")) {
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: ipv6 family specified for non-IPv6 address '%s' "
> + "in route definition"),
> + errorDetail, address);
> + goto cleanup;
> + }
> + if (netmask) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Specifying netmask invalid for IPv6 address '%s' "
> + "in route definition"),
> + errorDetail, address);
> + goto cleanup;
> + }
> + if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->gateway, AF_INET6)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: ipv6 specified for non-IPv6 gateway address '%s' "
> + "in route definition"),
> + errorDetail, gateway);
> + goto cleanup;
> + }
> + if (def->prefix > 128) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Invalid prefix %u specified "
> + "in route definition, "
> + "must be 0 - 128"),
> + errorDetail, def->prefix);
> + goto cleanup;
> + }
> + } else {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Unrecognized family '%s' "
> + "in route definition"),
> + errorDetail, def->family);
> + goto cleanup;
> + }
> +
> + /* make sure the address is a network address */
> + if (netmask) {
> + if (virSocketAddrMask(&def->address, &def->netmask, &testAddr) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: Error converting address '%s' with netmask '%s' "
> + "to network-address "
> + "in route definition"),
> + errorDetail, address, netmask);
> + goto cleanup;
> + }
> + } else {
> + if (virSocketAddrMaskByPrefix(&def->address,
> + def->prefix, &testAddr) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("%s: Error converting address '%s' with prefix %u "
> + "to network-address "
> + "in route definition"),
> + errorDetail, address, def->prefix);
> + goto cleanup;
> + }
> + }
> + if (!virSocketAddrEqual(&def->address, &testAddr)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Address '%s' in route definition "
> + "is not a network address"),
> + errorDetail, address);
> + goto cleanup;
> + }
> +
> + result = 0;
> +
> + cleanup:
> + if (result < 0)
> + virNetworkRouteDefClear(def);
> + VIR_FREE(address);
> + VIR_FREE(netmask);
> + VIR_FREE(gateway);
> +
> + return result;
> +}
> +
> +int
> +virNetworkRouteDefParseXML(const char *errorDetail,
> + 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 *family = NULL;
> + char *address = NULL, *netmask = NULL;
> + char *gateway = NULL;
> + unsigned long prefix = 0, metric = 0;
> + int result = -1;
> + int prefixRc, metricRc;
> + bool hasPrefix = false;
> + bool hasMetric = false;
> +
> + save = ctxt->node;
> + ctxt->node = node;
> +
> + /* grab raw data from XML */
> + 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,
> + _("%s: Invalid prefix specified "
> + "in route definition"),
> + errorDetail);
> + goto cleanup;
> + }
> + hasPrefix = (prefixRc == 0);
> + metricRc = virXPathULong("string(./@metric)", ctxt, &metric);
> + if (metricRc == -2) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Invalid metric specified "
> + "in route definition"),
> + errorDetail);
> + goto cleanup;
> + }
> + if (metricRc == 0) {
> + hasMetric = true;
> + if (metric == 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("%s: Invalid metric value, must be > 0 "
> + "in route definition"),
> + errorDetail);
> + goto cleanup;
> + }
> + }
> +
> + result = virNetworkRouteDefCreate(errorDetail, family, address, netmask,
> + gateway, prefix, hasPrefix, metric,
> + hasMetric, def);
> +
> + cleanup:
> + ctxt->node = save;
> + return result;
> +}
> +
> +int
> +virNetworkRouteDefFormat(virBufferPtr buf,
> + const virNetworkRouteDef *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);
> + }
> + if (def->has_metric && def->metric > 0)
> + virBufferAsprintf(buf, " metric='%u'", def->metric);
> + virBufferAddLit(buf, "/>\n");
> +
> + result = 0;
> + error:
> + return result;
> +}
> diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h
> new file mode 100644
> index 0000000..a42bb21
> --- /dev/null
> +++ b/src/conf/networkcommon_conf.h
> @@ -0,0 +1,76 @@
> +/*
> + * networkcommon_conf.h: network XML handling
> + *
> + * Copyright (C) 2006-2014 Red Hat, Inc.
> + * Copyright (C) 2006-2008 Daniel P. Berrange
> + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange@xxxxxxxxxx>
> + */
> +
> +#ifndef __NETWORKCOMMON_CONF_H__
> +# define __NETWORKCOMMON_CONF_H__
> +
> +# include <libxml/tree.h>
> +# include <libxml/xpath.h>
> +
> +# include "internal.h"
> +# include "virbuffer.h"
> +# include "virsocketaddr.h"
> +
> +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.
> + */
> + virSocketAddr netmask; /* ipv4 - either netmask or prefix specified */
> + unsigned int prefix; /* ipv6 - only prefix allowed */
> + bool has_prefix; /* prefix= was specified */
> + unsigned int metric; /* value for metric (defaults to 1) */
> + bool has_metric; /* metric= was specified */
> + virSocketAddr gateway; /* gateway IP address for ip-route */
> + };
> +
> +void
> +virNetworkRouteDefClear(virNetworkRouteDefPtr def);
> +int
> +virNetworkRouteDefCreate(const char *networkName,
> + char *family,
> + char *address,
> + char *netmask,
> + char *gateway,
> + unsigned int prefix,
> + bool hasPrefix,
> + unsigned int metric,
> + bool hasMetric,
> + virNetworkRouteDefPtr def);
> +int
> +virNetworkRouteDefParseXML(const char *networkName,
> + xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virNetworkRouteDefPtr def);
> +int
> +virNetworkRouteDefFormat(virBufferPtr buf,
> + const virNetworkRouteDef *def);
> +
> +#endif /* __NETWORKCOMMON_CONF_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fb5d003..d0c2a2d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -581,6 +581,13 @@ virNetworkEventLifecycleNew;
> virNetworkEventStateRegisterID;
>
>
> +# conf/networkcommon_conf.h
> +virNetworkRouteDefClear;
> +virNetworkRouteDefCreate;
> +virNetworkRouteDefFormat;
> +virNetworkRouteDefParseXML;
> +
> +
> # conf/node_device_conf.h
> virNodeDevCapsDefFree;
> virNodeDevCapTypeFromString;
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 380d136..c792f17 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -544,17 +544,27 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef,
>
> /* Set the routes */
> for (j = 0; j < netDef->nroutes; j++) {
> - virDomainNetRouteDefPtr route = netDef->routes[j];
> - if (VIR_SOCKET_ADDR_VALID(&route->to))
> - toStr = virSocketAddrFormat(&route->to);
> + virNetworkRouteDefPtr route = netDef->routes[j];
> + int prefix = 0;
> +
> + if (VIR_SOCKET_ADDR_VALID(&route->address))
> + toStr = virSocketAddrFormat(&route->address);
> else
> if (VIR_STRDUP(toStr, "default") < 0)
> goto error_out;
> - viaStr = virSocketAddrFormat(&route->via);
> + viaStr = virSocketAddrFormat(&route->gateway);
> VIR_DEBUG("Adding route %s/%d via %s", toStr, route->prefix, viaStr);
>
> - if (virNetDevAddRoute(newname, &route->to, route->prefix,
> - &route->via, 0) < 0) {
> + if ((prefix = virSocketAddrGetIpPrefix(&route->address,
> + &route->netmask,
> + prefix)) < 0) {
This is incorrect usage of virSocketAddrGetIpPrefix() - it should be
called with route->prefix. If route->prefix is > 0, that is returned,
otherwise it tries to divine the correct prefix from the netmask (or
address if no netmask is given).
Beyond that, since all the uses of virNetDevAddRoute() are preceded by
such a call to virSocketAddrGetIpPrefix(), maybe you should move that
bit of code into virNetDevAddRoute() to avoid duplication.
Also, I just noticed that virSocketAddrGetIpPrefix() doesn't do the
correct thing in the case that address == 0.0.0.0 and prefix == 0 (it
should return 0, but since VIR_SOCKET_ADDR_IS_FAMILY(address, AF_INET)
will be true, it will instead end up returning 8, which isn't what you
want.) I think that should be fixed with a separate small patch to
virSocketAddrGetIpPrefix to special-case
"address->data.inet4.sin_addr.s_addr == 0" so that it returns 0 in that
case.
Are you starting to regret that you listened to my idea of combining net
and domain route code? :-)
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Invalid netmask or IP address in route definition"));
> + goto error_out;
> + }
> +
> + if (virNetDevAddRoute(newname, &route->address, prefix,
> + &route->gateway, route->metric) < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> _("Failed to add route %s/%d via %s"),
It looks to me like virNetDevAddRoute is already logging errors (in at
least some cases). I think at this level you should just silently
cleanup (since the error should already be logged.)
> toStr, route->prefix, viaStr);
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index d7cd1d5..dd99bbb 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
> @@ -2,7 +2,7 @@
> * lxc_native.c: LXC native configuration import
> *
> * Copyright (c) 2014 Red Hat, Inc.
> - * Copyright (c) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + * Copyright (c) 2013-2015 SUSE LINUX Products GmbH, Nuernberg, Germany.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -433,15 +433,32 @@ typedef struct {
> static int
> lxcAddNetworkRouteDefinition(const char *address,
> int family,
> - virDomainNetRouteDefPtr **routes,
> + virNetworkRouteDefPtr **routes,
> size_t *nroutes)
> {
> - virDomainNetRouteDefPtr route = NULL;
> + virNetworkRouteDefPtr route = NULL;
> + char *familyStr = NULL;
> + char *zero = NULL;
> + char *gateway = NULL;
> +
> + if (VIR_STRDUP(zero, family == AF_INET ? VIR_SOCKET_ADDR_IPV4_ALL
> + : VIR_SOCKET_ADDR_IPV6_ALL) < 0)
> + goto error;
>
> if (VIR_ALLOC(route) < 0)
> goto error;
>
> - if (virSocketAddrParse(&route->via, address, family) < 0)
> + if (virSocketAddrParse(&route->gateway, address, family) < 0)
> + goto error;
> +
> + if (VIR_STRDUP(familyStr, family == AF_INET ? "ipv4" : "ipv6") < 0)
> + goto error;
> +
> + if (VIR_STRDUP(gateway, address) < 0)
> + goto error;
> +
> + if (virNetworkRouteDefCreate(_("Domain interface"), familyStr, zero, NULL, gateway,
> + 0, false, 0, false, route) < 0)
> goto error;
>
> if (VIR_APPEND_ELEMENT(*routes, *nroutes, route) < 0)
> @@ -450,6 +467,7 @@ lxcAddNetworkRouteDefinition(const char *address,
> return 0;
>
> error:
> + virNetworkRouteDefClear(route);
> VIR_FREE(route);
> return -1;
> }
> diff --git a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml
> index d2cec8f..79bcfa0 100644
> --- a/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml
> +++ b/tests/lxcconf2xmldata/lxcconf2xml-physnetwork.xml
> @@ -27,8 +27,8 @@
> </source>
> <ip address='192.168.122.2' family='ipv4' prefix='24'/>
> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/>
> - <route family='ipv4' via='192.168.122.1'/>
> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/>
> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/>
> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/>
> </hostdev>
> </devices>
> </domain>
> diff --git a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml
> index b1210e5..45a2012 100644
> --- a/tests/lxcconf2xmldata/lxcconf2xml-simple.xml
> +++ b/tests/lxcconf2xmldata/lxcconf2xml-simple.xml
> @@ -39,8 +39,8 @@
> <source bridge='virbr0'/>
> <ip address='192.168.122.2' family='ipv4' prefix='24'/>
> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='64'/>
> - <route family='ipv4' via='192.168.122.1'/>
> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/>
> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/>
> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/>
> <guest dev='eth0'/>
> <link state='up'/>
> </interface>
> diff --git a/tests/lxcxml2xmldata/lxc-hostdev.xml b/tests/lxcxml2xmldata/lxc-hostdev.xml
> index 61e8655..3972594 100644
> --- a/tests/lxcxml2xmldata/lxc-hostdev.xml
> +++ b/tests/lxcxml2xmldata/lxc-hostdev.xml
> @@ -37,8 +37,8 @@
> </source>
> <ip address='192.168.122.2' family='ipv4'/>
> <ip address='2003:db8:1:0:214:1234:fe0b:3596' family='ipv6' prefix='24'/>
> - <route family='ipv4' via='192.168.122.1'/>
> - <route family='ipv6' via='2003:db8:1:0:214:1234:fe0b:3595'/>
> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/>
> + <route family='ipv6' address='::' gateway='2003:db8:1:0:214:1234:fe0b:3595'/>
> </hostdev>
> </devices>
> </domain>
> diff --git a/tests/lxcxml2xmldata/lxc-idmap.xml b/tests/lxcxml2xmldata/lxc-idmap.xml
> index 2b04a65..b477636 100644
> --- a/tests/lxcxml2xmldata/lxc-idmap.xml
> +++ b/tests/lxcxml2xmldata/lxc-idmap.xml
> @@ -30,8 +30,8 @@
> <source bridge='bri0'/>
> <ip address='192.168.122.12' family='ipv4' prefix='24'/>
> <ip address='192.168.122.13' family='ipv4' prefix='24'/>
> - <route family='ipv4' via='192.168.122.1'/>
> - <route family='ipv4' via='192.168.124.1' address='192.168.124.0' prefix='24'/>
> + <route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/>
> + <route family='ipv4' address='192.168.124.0' prefix='24' gateway='192.168.124.1'/>
> <target dev='veth0'/>
> <guest dev='eth2'/>
> </interface>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list