Hi Michal, On Mon, 2015-01-12 at 11:47 +0100, Michal Privoznik wrote: > 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.
Yes, this is the beginning: there surely is more to move into those new files that isn't related to routes. > > 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. To help you, it just adds |(::) at the end to include the default '::' notation. > > </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. Yes, I feel that something needs to be done for that too. > > + } > > 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. Oh indeed. > > } > > } > > > > > 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. Indeed, that one may not be needed. > > + 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? :) Well, that was previously erroneous, since I hacked on it last year but wasn't mentioned in the header... but yeah, who cares ;) > > * > > * This library is free software; you can redistribute it and/or > > * modify it under the terms of the GNU Lesser General Public > > > ACK I'll do the changes before pushing. -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list