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
