Re: [libvirt] [PATCHv2 3/7] Move code related to network routes to networkcommon_conf.[ch]

(Gene - I Cc'ed you because of one question I have for you down in the bowels of the review. Just search for "Gene" and you'll get to it).

On 01/15/2015 04:25 AM, Cédric Bosdonnat wrote: > Moving code for parsing and formatting network routes to > networkcommon_conf helps reusing those routes for domains. The route > definition has been hidden to help reducing the number of unnecessary > checks in the format function. > --- > po/POTFILES.in | 1 + > src/Makefile.am | 3 +- > src/conf/network_conf.c | 297 ++---------------------------- > src/conf/network_conf.h | 22 +-- > src/conf/networkcommon_conf.c | 414 ++++++++++++++++++++++++++++++++++++++++++ > src/conf/networkcommon_conf.h | 72 ++++++++ > src/libvirt_private.syms | 11 ++ > src/network/bridge_driver.c | 44 ++--- > 8 files changed, 526 insertions(+), 338 deletions(-) > create mode 100644 src/conf/networkcommon_conf.c > create mode 100644 src/conf/networkcommon_conf.h > > 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 216abac..4bba536 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/network_conf.c b/src/conf/network_conf.c > index 26fe18d..66f9b6c 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); > @@ -251,7 +245,7 @@ virNetworkDefFree(virNetworkDefPtr def) > VIR_FREE(def->ips); > > for (i = 0; i < def->nroutes && def->routes; i++) > - virNetworkRouteDefClear(&def->routes[i]); > + virNetworkRouteDefFree(def->routes[i]); > VIR_FREE(def->routes); > > for (i = 0; i < def->nPortGroups && def->portGroups; i++) > @@ -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; > -} > - > -static int > virNetworkPortGroupParseXML(virPortGroupDefPtr def, > xmlNodePtr node, > xmlXPathContextPtr ctxt) > @@ -2209,11 +1977,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) > goto error; > /* parse each definition */ > for (i = 0; i < nRoutes; i++) { > - if (virNetworkRouteDefParseXML(def->name, > + virNetworkRouteDefPtr route = NULL; > + > + if (!(route = virNetworkRouteDefParseXML(def->name, > routeNodes[i], > - ctxt, > - &def->routes[i]) < 0) > + ctxt))) > goto error; > + def->routes[i] = route; > def->nroutes++; > } > > @@ -2229,17 +1999,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) > size_t j; > virSocketAddr testAddr, testGw; > bool addrMatch; > - virNetworkRouteDefPtr gwdef = &def->routes[i]; > + virNetworkRouteDefPtr gwdef = def->routes[i]; > + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(gwdef); > addrMatch = false; > for (j = 0; j < nIps; j++) { > virNetworkIpDefPtr def2 = &def->ips[j]; > - if (VIR_SOCKET_ADDR_FAMILY(&gwdef->gateway) > + if (VIR_SOCKET_ADDR_FAMILY(gateway) > != VIR_SOCKET_ADDR_FAMILY(&def2->address)) { > continue; > } > int prefix = virNetworkIpDefPrefix(def2); > virSocketAddrMaskByPrefix(&def2->address, prefix, &testAddr); > - virSocketAddrMaskByPrefix(&gwdef->gateway, prefix, &testGw); > + virSocketAddrMaskByPrefix(gateway, prefix, &testGw); > if (VIR_SOCKET_ADDR_VALID(&testAddr) && > VIR_SOCKET_ADDR_VALID(&testGw) && > virSocketAddrEqual(&testAddr, &testGw)) { > @@ -2248,7 +2019,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) > } > } > if (!addrMatch) { > - char *gw = virSocketAddrFormat(&gwdef->gateway); > + char *gw = virSocketAddrFormat(gateway); > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("unreachable static route gateway '%s' specified for network '%s'"), > gw, def->name); > @@ -2585,50 +2356,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) > { > @@ -2850,7 +2577,7 @@ virNetworkDefFormatBuf(virBufferPtr buf, > } > > for (i = 0; i < def->nroutes; i++) { > - if (virNetworkRouteDefFormat(buf, &def->routes[i]) < 0) > + if (virNetworkRouteDefFormat(buf, def->routes[i]) < 0) > goto error; > } > > diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h > index 8110028..b113e14 100644 > --- a/src/conf/network_conf.h > +++ b/src/conf/network_conf.h > @@ -39,6 +39,7 @@ > # include "virmacaddr.h" > # include "device_conf.h" > # include "virbitmap.h" > +# include "networkcommon_conf.h" > > typedef enum { > VIR_NETWORK_FORWARD_NONE = 0, > @@ -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 { > @@ -259,7 +241,7 @@ struct _virNetworkDef { > virNetworkIpDefPtr ips; /* ptr to array of IP addresses on this network */ > > size_t nroutes; > - virNetworkRouteDefPtr routes; /* ptr to array of static routes on this interface */ > + virNetworkRouteDefPtr *routes; /* ptr to array of static routes on this interface */ > > virNetworkDNSDef dns; /* dns related configuration */ > virNetDevVPortProfilePtr virtPortProfile; > diff --git a/src/conf/networkcommon_conf.c b/src/conf/networkcommon_conf.c > new file mode 100644 > index 0000000..1545367 > --- /dev/null > +++ b/src/conf/networkcommon_conf.c > @@ -0,0 +1,414 @@ > +/* > + * 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(a)redhat.com&gt; > + */ > + > +#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 > + > +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 > +virNetworkRouteDefFree(virNetworkRouteDefPtr def) > +{ > + VIR_FREE(def->family); > + VIR_FREE(def); > +} > + > +virNetworkRouteDefPtr > +virNetworkRouteDefCreate(const char *errorDetail, > + char *family, > + char *address, > + char *netmask, > + char *gateway, > + unsigned int prefix, > + bool hasPrefix, > + unsigned int metric, > + bool hasMetric) > +{ > + virNetworkRouteDefPtr def = NULL; > + virSocketAddr testAddr; > + > + if (VIR_ALLOC(def) < 0) > + return NULL; > + > + def->family = family; moving family rather than copying it works, but it complicates things down in the caller, which then has to make some intelligent decisions about whether or not the original pointer has been moved to somewhere and is still in use, or if it should be free'd (see below). For that reason, I think family should be VIR_STRDUP'ed here and the original unconditionally VIR_FREE'd in the caller. (this wasn't an issue in the original code, because family was being parsed directly into the object)

> + 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 error; > + } > + > + if (!gateway) { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Missing required gateway attribute " > + "in route definition"), > + errorDetail); > + goto error; > + } > + > + if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Bad network address '%s' " > + "in route definition"), > + errorDetail, address); > + goto error; > + } > + > + if (virSocketAddrParse(&def->gateway, gateway, AF_UNSPEC) < 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Bad gateway address '%s' " > + "in route definition"), > + errorDetail, gateway); > + goto error; > + } > + > + /* 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 error; > + } > + 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 error; > + } > + 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 error; > + } > + 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 error; > + } > + 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 error; > + } > + } > + 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 error; > + } > + } 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 error; > + } > + if (netmask) { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Specifying netmask invalid for IPv6 address '%s' " > + "in route definition"), > + errorDetail, address); > + goto error; > + } > + 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 error; > + } > + 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 error; > + } > + } else { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Unrecognized family '%s' " > + "in route definition"), > + errorDetail, def->family); > + goto error; > + } > + > + /* 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 error; > + } > + } 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 error; > + } > + } > + if (!virSocketAddrEqual(&def->address, &testAddr)) { > + virReportError(VIR_ERR_XML_ERROR, > + _("%s: Address '%s' in route definition " > + "is not a network address"), > + errorDetail, address); > + goto error; > + } > + > + return def; > + > + error: > + virNetworkRouteDefFree(def); > + return NULL; > +} > + > +virNetworkRouteDefPtr > +virNetworkRouteDefParseXML(const char *errorDetail, > + xmlNodePtr node, > + xmlXPathContextPtr ctxt) > +{ > + /* > + * virNetworkRouteDef object is already allocated as part > + * of an array. On failure clear: it out, but don't free it. > + */ > + > + virNetworkRouteDefPtr def = NULL; > + xmlNodePtr save; > + char *family = NULL; > + char *address = NULL, *netmask = NULL; > + char *gateway = NULL; > + unsigned long prefix = 0, metric = 0; > + 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; > + } > + } > + > + def = virNetworkRouteDefCreate(errorDetail, family, address, netmask, > + gateway, prefix, hasPrefix, metric, > + hasMetric); > + > + cleanup: > + ctxt->node = save; > + VIR_FREE(address); > + VIR_FREE(netmask); > + VIR_FREE(gateway); You haven't free'd family, which will cause a leak if an error is found prior to calling virNetworkRouteDefCreate(). Of course since virNetworkRouteDefCreate() *moves* family into the object rather than copying it, if you indiscriminately free the original pointer here, you've just free'd something that's still in use in the case of non-failure. (more explanation of why I think that virNetworkRouteDefCreate() should VIR_STRDUP family instead of moving it) > + return def; > +} > + > +int > +virNetworkRouteDefFormat(virBufferPtr buf, > + const virNetworkRouteDef *def) > +{ > + int result = -1; > + char *addr = NULL; > + > + virBufferAddLit(buf, "<route"); > + > + if (def->family) > + virBufferAsprintf(buf, " family='%s'", def->family); > + > + addr = virSocketAddrFormat(&def->address); > + > + if (!addr) > + goto error; Since you need to change the handling of family anyway, while you're moving this, you could change these usages to: if (!(addr = virSocketAddrFormat(&def->address))) goto error; to save lines.

> + virBufferAsprintf(buf, " address='%s'", addr); > + VIR_FREE(addr); > + > + if (VIR_SOCKET_ADDR_VALID(&def->netmask)) { > + addr = virSocketAddrFormat(&def->netmask); > + > + if (!addr) > + goto error; same here > + virBufferAsprintf(buf, " netmask='%s'", addr); > + VIR_FREE(addr); > + } > + if (def->has_prefix) > + virBufferAsprintf(buf, " prefix='%u'", def->prefix); > + > + addr = virSocketAddrFormat(&def->gateway); > + if (!addr) > + goto error; and here. etc. > + 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; Also, while you're moving it, can you change this error label to cleanup? We didn't use to pay too much attention to this, but for awhile now we've been trying to use "error" only for codepaths that are taken exclusively in case of an error, and "cleanup" for those that are executed in all cases.

> +} > + > +virSocketAddrPtr > +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def) > +{ > + if (def) > + return &def->address; > + > + return NULL; > +} > + > +int > +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def) > +{ > + int prefix = 0; > + virSocketAddr zero; > + > + if (!def) > + return -1; > + > + /* this creates an all-0 address of the appropriate family */ > + ignore_value(virSocketAddrParse(&zero, > + (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) > + ? VIR_SOCKET_ADDR_IPV4_ALL > + : VIR_SOCKET_ADDR_IPV6_ALL), > + VIR_SOCKET_ADDR_FAMILY(&def->address))); > + > + if (virSocketAddrEqual(&def->address, &zero)) { > + if (def->has_prefix && def->prefix == 0) > + prefix = 0; I understand that you created this function by just moving existing code, so I hesitate to even get into the subject of refactoring it here (as a matter of fact, the more I think about it, the more I think it's better to *not* refactor as I will suggest below, in order to avoid unintentional regressions when backporting to branches. But I've already typed it, so I'll leave it in :-) I think the above chunk should be outside all of the conditionals and shouldn't check for == 0 - no matter what else is true, if the user specified a prefix, that is what should be returned. Likewise, if they specified a netmask, then the netmask should be converted to a prefix no matter what else was supplied. Only if there is no valid prefix and no valid netmask, *then* we should create a prefix based on the class/family of address. The following seems like a proper way to implement this (although again - I don't think I want such a substantial change in what is essentially a code movement patch. Instead just leave it as is, and I'll make a patch implementing my idea after you've pushed yours): 1) change virSocketAddrGetIpPrefix() to add the check for address == 0 (separately for ipv6 and ipv4) and set prefix to 0 in those cases. 2) virNetworkRouteDefGetPrefix() can be simplified to do this: * if prefix_specified is true, just return prefix (even if it's 0), otherwise return the result of virSocketAddrGetIpPrefix(). > + else if ((VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET) && > + virSocketAddrEqual(&def->netmask, &zero))) > + prefix = 0; > + else > + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, > + def->prefix); > + } else { > + prefix = virSocketAddrGetIpPrefix(&def->address, &def->netmask, > + def->prefix); > + } > + > + return prefix; > +} > + > +unsigned int > +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def) > +{ > + if (def && def->has_metric && def->metric > 0) > + return def->metric; > + > + return 1; Hmm. Looking at the existing code, it does look like "1" is the default setting for metric. But when I look at "man route" I see that 0 is the default metric for IPv4 and 1 for IPv6. So why don't we allow a metric of 0? Gene? > +} > + > +virSocketAddrPtr > +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def) > +{ > + if (def) > + return &def->gateway; > + return NULL; > +} > diff --git a/src/conf/networkcommon_conf.h b/src/conf/networkcommon_conf.h > new file mode 100644 > index 0000000..4d2b1d2 > --- /dev/null > +++ b/src/conf/networkcommon_conf.h > @@ -0,0 +1,72 @@ > +/* > + * 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(a)redhat.com&gt; > + */ > + > +#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; > + > +void > +virNetworkRouteDefFree(virNetworkRouteDefPtr def); > + > +virNetworkRouteDefPtr > +virNetworkRouteDefCreate(const char *networkName, > + char *family, > + char *address, > + char *netmask, > + char *gateway, > + unsigned int prefix, > + bool hasPrefix, > + unsigned int metric, > + bool hasMetric); > + > +virNetworkRouteDefPtr > +virNetworkRouteDefParseXML(const char *networkName, > + xmlNodePtr node, > + xmlXPathContextPtr ctxt); > +int > +virNetworkRouteDefFormat(virBufferPtr buf, > + const virNetworkRouteDef *def); > + > +virSocketAddrPtr > +virNetworkRouteDefGetAddress(virNetworkRouteDefPtr def); > + > +int > +virNetworkRouteDefGetPrefix(virNetworkRouteDefPtr def); > + > +unsigned int > +virNetworkRouteDefGetMetric(virNetworkRouteDefPtr def); > + > +virSocketAddrPtr > +virNetworkRouteDefGetGateway(virNetworkRouteDefPtr def); > + > +#endif /* __NETWORKCOMMON_CONF_H__ */ > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 7ceb54d..a35f077 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -582,6 +582,17 @@ virNetworkEventLifecycleNew; > virNetworkEventStateRegisterID; > > > +# conf/networkcommon_conf.h > +virNetworkRouteDefCreate; > +virNetworkRouteDefFormat; > +virNetworkRouteDefFree; > +virNetworkRouteDefGetAddress; > +virNetworkRouteDefGetGateway; > +virNetworkRouteDefGetMetric; > +virNetworkRouteDefGetPrefix; > +virNetworkRouteDefParseXML; > + > + > # conf/node_device_conf.h > virNodeDevCapsDefFree; > virNodeDevCapTypeFromString; > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index fca60f1..7b84e27 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -1934,29 +1934,10 @@ static int > networkAddRouteToBridge(virNetworkObjPtr network, > virNetworkRouteDefPtr routedef) > { > - int prefix = 0; > - unsigned int metric; > - virSocketAddrPtr addr = &routedef->address; > - virSocketAddrPtr mask = &routedef->netmask; > - virSocketAddr zero; > - > - /* this creates an all-0 address of the appropriate family */ > - ignore_value(virSocketAddrParse(&zero, > - (VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET) > - ? "0.0.0.0" : "::"), > - VIR_SOCKET_ADDR_FAMILY(addr))); > - > - if (virSocketAddrEqual(addr, &zero)) { > - if (routedef->has_prefix && routedef->prefix == 0) > - prefix = 0; > - else if ((VIR_SOCKET_ADDR_IS_FAMILY(mask, AF_INET) && > - virSocketAddrEqual(mask, &zero))) > - prefix = 0; > - else > - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); > - } else { > - prefix = virSocketAddrGetIpPrefix(addr, mask, routedef->prefix); > - } > + int prefix = virNetworkRouteDefGetPrefix(routedef); > + unsigned int metric = virNetworkRouteDefGetMetric(routedef); > + virSocketAddrPtr addr = virNetworkRouteDefGetAddress(routedef); > + virSocketAddrPtr gateway = virNetworkRouteDefGetGateway(routedef); > > if (prefix < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -1966,13 +1947,8 @@ networkAddRouteToBridge(virNetworkObjPtr network, > return -1; > } > > - if (routedef->has_metric && routedef->metric > 0) > - metric = routedef->metric; > - else > - metric = 1; > - > - if (virNetDevAddRoute(network->def->bridge, &routedef->address, > - prefix, &routedef->gateway, metric) < 0) { > + if (virNetDevAddRoute(network->def->bridge, addr, > + prefix, gateway, metric) < 0) { > return -1; > } > return 0; > @@ -2063,11 +2039,15 @@ networkStartNetworkVirtual(virNetworkObjPtr network) > goto err2; > > for (i = 0; i < network->def->nroutes; i++) { > - routedef = &network->def->routes[i]; > + virSocketAddrPtr gateway = NULL; > + > + routedef = network->def->routes[i]; > + gateway = virNetworkRouteDefGetGateway(routedef); > + > /* Add the IP route to the bridge */ > /* ignore errors, error msg will be generated */ > /* but libvirt will not know and net-destroy will work. */ > - if (VIR_SOCKET_ADDR_VALID(&routedef->gateway)) { > + if (VIR_SOCKET_ADDR_VALID(gateway)) { I wonder why this check was here in the first place - wouldn't we have disallowed the route definition in the first place if there wasn't a valid gateway? (again, this is just idle speculation. The task of this patch was to reorganize the existing code, not change behavior :-) > if (networkAddRouteToBridge(network, routedef) < 0) { > /* an error occurred adding the static route */ > continue; /* for now, do nothing */ In the end, ACK with the change to handling of family during parsing, and combining the few virSocketAddrFormat() calls with the subsequent conditionals (and the indentation problem Michal pointed out). The other issues should be handled in a separate patch which you needn't worry about (you've gone far enough beyond the call of duty already :-)

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list