[libvirt] [PATCHv3 02/16] Domain conf: allow more than one IP address for net devices
Cedric Bosdonnat
cbosdonnat at suse.com
Mon Oct 27 16:33:47 UTC 2014
Hi Daniel,
On Wed, 2014-10-22 at 11:03 +0100, Daniel P. Berrange wrote:
> On Fri, Oct 10, 2014 at 02:03:54PM +0200, Cédric Bosdonnat wrote:
> > Add the possibility to have more than one IP address configured for a
> > domain network interface. IP addresses can also have a prefix to define
> > the corresponding netmask.
> > ---
> > docs/formatdomain.html.in | 22 +++++++
> > docs/schemas/domaincommon.rng | 11 +++-
> > src/conf/domain_conf.c | 123 +++++++++++++++++++++++++++++++------
> > src/conf/domain_conf.h | 16 ++++-
> > src/libvirt_private.syms | 2 +
> > src/openvz/openvz_conf.c | 2 +-
> > src/openvz/openvz_driver.c | 6 +-
> > src/qemu/qemu_driver.c | 25 ++++++--
> > src/qemu/qemu_hotplug.c | 6 +-
> > src/uml/uml_conf.c | 2 +-
> > src/vbox/vbox_common.c | 3 +-
> > src/xenconfig/xen_common.c | 15 ++---
> > src/xenconfig/xen_sxpr.c | 12 ++--
> > tests/lxcxml2xmldata/lxc-idmap.xml | 2 +
> > 14 files changed, 195 insertions(+), 52 deletions(-)
>
> I think it is probably worth a followup patch to make drivers
> report VIR_ERR_CONFIG_UNSUPPORTED in the case where nips > 1
> and the driver only supports nips==1.
I'll add those errors indeed.
> Ideally we'd also have drivers report an error for the network
> types they don't support IPs with, but that's much more work
> and we don't even have good reporting of errors for the network
> types themselves, let alone IP addrs. So we can ignore that
> for now.
Ok, but good to keep that in mind, indeed.
> > +int
> > +virDomainNetAppendIpAddress(virDomainNetDefPtr def,
> > + const char *address,
> > + unsigned int prefix)
> > +{
> > + virDomainNetIpDefPtr ipDef = NULL;
> > + if (VIR_ALLOC(ipDef) < 0)
> > + return -1;
> > +
> > + if (VIR_STRDUP(ipDef->address, address) < 0)
> > + return -1;
>
> Oh, you leak ipDef on OOM here.
Nice catch!
> > +
> > + ipDef->prefix = prefix;
> > +
> > + if (VIR_APPEND_ELEMENT(def->ips, def->nips, ipDef) < 0) {
> > + virDomainNetIpDefFree(ipDef);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
>
> > @@ -7062,11 +7099,44 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > xmlStrEqual(cur->name, BAD_CAST "source")) {
> > address = virXMLPropString(cur, "address");
> > port = virXMLPropString(cur, "port");
> > - } else if (!address &&
> > - (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> > - def->type == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
> > - xmlStrEqual(cur->name, BAD_CAST "ip")) {
> > - address = virXMLPropString(cur, "address");
> > + } else if (xmlStrEqual(cur->name, BAD_CAST "ip")) {
> > + /* Parse the prefix in every case */
> > + char *prefixStr = NULL;
> > + unsigned int prefixValue = 0;
> > +
> > + if ((prefixStr = virXMLPropString(cur, "prefix")) &&
> > + (virStrToLong_ui(prefixStr, NULL, 10, &prefixValue) < 0)) {
> > +
> > + virReportError(VIR_ERR_INVALID_ARG,
> > + _("Invalid network prefix: '%s'"),
> > + prefixStr);
> > + VIR_FREE(prefixStr);
> > + goto error;
> > + }
> > + VIR_FREE(prefixStr);
> > +
> > + /* Previous behavior: make sure this address it the first one
> > + in the resulting list */
> > + if (!address &&
> > + (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> > + def->type == VIR_DOMAIN_NET_TYPE_BRIDGE)) {
> > +
> > + address = virXMLPropString(cur, "address");
> > + prefix = prefixValue;
> > + } else {
> > + /* All other <ip/> elements will be added after */
> > + virDomainNetIpDefPtr ip = NULL;
> > +
> > + if (VIR_ALLOC(ip) < 0)
> > + goto error;
> > +
> > + ip->address = virXMLPropString(cur, "address");
> > + ip->prefix = prefixValue;
> > +
> > + if (ip->address != NULL &&
> > + VIR_APPEND_ELEMENT(ips, nips, ip) < 0)
> > + goto error;
> > + }
>
> I'm not sure I understand why you have this if/else here. You are
> parsing the addresses in order in the XML, and appending to the list
> of IP address, so sure the first address will always be first in the
> list and so there's no need for the if/else.
The content of address is written before the content of ips in the list.
And the problem comes with the fact that some network types are using an
'address' attribute to store the IP.
> > } else if (!ifname &&
> > xmlStrEqual(cur->name, BAD_CAST "target")) {
> > ifname = virXMLPropString(cur, "dev");
> > @@ -7267,8 +7337,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > dev = NULL;
> > }
> > if (address != NULL) {
> > - def->data.ethernet.ipaddr = address;
> > - address = NULL;
> > + virDomainNetAppendIpAddress(def, address, prefix);
> > + VIR_FREE(address);
> > }
>
> This actually causes the address to be put at the end of the list surely ?
As the list is still empty at that time, this address will be in the
first position
> > break;
> >
> > @@ -7282,8 +7352,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > def->data.bridge.brname = bridge;
> > bridge = NULL;
> > if (address != NULL) {
> > - def->data.bridge.ipaddr = address;
> > - address = NULL;
> > + virDomainNetAppendIpAddress(def, address, prefix);
> > + VIR_FREE(address);
> > }
>
> And this.
same here
> > break;
> >
> > @@ -7381,6 +7451,11 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > break;
> > }
> >
> > + for (i = 0; i < nips; i++) {
> > + if (VIR_APPEND_ELEMENT(def->ips, def->nips, ips[i]) < 0)
> > + goto error;
> > + }
>
> Why not just assign 'def->ips = ips' and def->nips = nips, instead
> of doing more memory allocation in a loop here ?
Because we may already have an address in def->ips due to the other
things mentioned before.
> > +
> > if (script != NULL) {
> > def->script = script;
> > script = NULL;
> > @@ -7643,6 +7718,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> > VIR_FREE(linkstate);
> > VIR_FREE(addrtype);
> > VIR_FREE(trustGuestRxFilters);
> > + VIR_FREE(ips);
> > virNWFilterHashTableFree(filterparams);
> >
> > return def;
> > @@ -16631,6 +16707,21 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
> > return 0;
> > }
>
>
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index afa3da6..6ecf639 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -471,6 +471,15 @@ typedef enum {
> > VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST
> > } virDomainHostdevCapsType;
> >
> > +typedef struct _virDomainNetIpDef virDomainNetIpDef;
> > +typedef virDomainNetIpDef *virDomainNetIpDefPtr;
> > +struct _virDomainNetIpDef {
> > + char *address; /* ipv4 or ipv6 address */
> > + unsigned int prefix; /* number of 1 bits in the net mask */
> > +};
>
> This ought to really use virSocketAddr, but perhaps you do that
> later in this patch series anyway.
No, but I'll switch to it, indeed.
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 1e504ec..3b31b77 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -2090,8 +2090,10 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> > case VIR_DOMAIN_NET_TYPE_ETHERNET:
> > if (STRNEQ_NULLABLE(olddev->data.ethernet.dev,
> > newdev->data.ethernet.dev) ||
> > - STRNEQ_NULLABLE(olddev->data.ethernet.ipaddr,
> > - newdev->data.ethernet.ipaddr)) {
> > + STRNEQ_NULLABLE(olddev->nips > 0 ? olddev->ips[0]->address
> > + : "",
> > + newdev->nips > 0 ? newdev->ips[0]->address
> > + : "")) {
>
> I think could just use NULL instead of "" here.
Indeed, I can't even remember why I put "".
Regards,
--
Cedric
More information about the libvir-list
mailing list