[libvirt] [PATCHv3 02/16] Domain conf: allow more than one IP address for net devices
Daniel P. Berrange
berrange at redhat.com
Wed Oct 22 10:03:06 UTC 2014
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.
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.
> +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.
> +
> + 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.
> } 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 ?
> 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.
> 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 ?
> +
> 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.
> 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.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list