Hi Laine,
On Thu, 2015-01-15 at 13:34 -0500, Laine Stump wrote:
> (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).
Looks like you forgot to actually add Gene to CC ;)
> 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@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
> > +
> > +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)
That sounded weird to me too... I was hesitating to VIR_STRDUP it, but
then it's one more point towards it. I'll change that.
>
> > + 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.
Ok indeed.
> > + 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.
Ok. I'll rename it.
> > +}
> > +
> > +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@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;
> > +
> > +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 :-)
Ok, I'll push with those changes.
--
Cedric
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list