Re: [libvirt] [PATCH] Share the code and schemas for domain and network route definitions

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