11:33 a.m.
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