On 09.01.2015 17:47, 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

I've always felt that what's shared in RNG should share parser/formatter in the code too.

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>

My brain is just too small to go through this on Monday morning.

</data> </define>

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]);

This code snippet repeats itself in a few places too. Looks like virNetworkRouteDefFree() which encapsulates the two comes handy.

+ } VIR_FREE(def->routes);

- virDomainDeviceInfoClear(&def->info); + virDomainDeviceInfoClear(&def->info);

VIR_FREE(def->filter); virNWFilterHashTableFree(def->filterparams);

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);

Or just user routes[i] directly.

} }

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

+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)) {

I know you've copied the preexisting code, but since this kind of check is done in virNetworkRouteDefCreate() - is it needed here too? It doesn't hurt anything (but performance, which we don't care about that much). I'm just curious.

+ 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/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.

Technically speaking this should be 2013,2015 but who cares, right? :)

* * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public

ACK Michal

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@redhat.com> + */ + +#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@redhat.com> + */ + +#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>