libxl uses some xenstore entries for hints in memory management (especially when starting new domain). This includes dom0 memory limit and Xen free memory margin, based on current system state. Entries are created at first function usage, so force such call at daemon startup, which most likely will be before any domain startup. Also prevent automatic memory management if dom0_mem= option passed to xen hypervisor - it is known to be incompatible with autoballoon. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - disable autoballoon when dom0_mem option detected - rebase on 1.0.6+ Changes in v3: - improve comments and error messages - rename auto_autoballoon src/libxl/libxl_conf.h | 4 ++++ src/libxl/libxl_driver.c | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 44ecd41..42bcdbc 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -62,6 +62,10 @@ struct _libxlDriverPrivate { virPortAllocatorPtr reservedVNCPorts; + /* Controls automatic ballooning of domain0. If true, get memory for new + * domains from domain0. */ + bool autoballoon; + size_t nactive; virStateInhibitCallback inhibitCallback; void *inhibitOpaque; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 3990354..89f4f12 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -29,6 +29,7 @@ #include <math.h> #include <libxl.h> #include <fcntl.h> +#include <regex.h> #include "internal.h" #include "virlog.h" @@ -963,7 +964,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlBuildDomainConfig(driver, vm->def, &d_config) < 0) goto error; - if (libxlFreeMem(priv, &d_config) < 0) { + if (driver->autoballoon && libxlFreeMem(priv, &d_config) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to get free memory for domain '%s'"), d_config.c_info.name); @@ -1146,6 +1147,29 @@ libxlStateCleanup(void) } static int +libxlGetAutoballoon(libxlDriverPrivatePtr driver) +{ + const libxl_version_info *info; + regex_t regex; + int ret; + + info = libxl_get_version_info(driver->ctx); + if (!info) + return 1; /* default to on */ + + ret = regcomp(®ex, + "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )", + REG_NOSUB | REG_EXTENDED); + if (ret) + return 1; + + ret = regexec(®ex, info->commandline, 0, NULL, 0); + regfree(®ex); + return ret == REG_NOMATCH; +} + + +static int libxlStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) @@ -1154,6 +1178,7 @@ libxlStateInitialize(bool privileged, char *log_file = NULL; virCommandPtr cmd; int status, ret = 0; + unsigned int free_mem; char ebuf[1024]; /* Disable libxl driver if non-root */ @@ -1278,6 +1303,16 @@ libxlStateInitialize(bool privileged, NULL))) goto error; + /* This will fill xenstore info about free and dom0 memory if missing, + * should be called before starting first domain */ + if (libxl_get_free_memory(libxl_driver->ctx, &free_mem)) { + VIR_ERROR(_("Unable to configure libxl's memory management parameters")); + goto error; + } + + /* setup autoballoon */ + libxl_driver->autoballoon = libxlGetAutoballoon(libxl_driver); + /* Load running domains first. */ if (virDomainObjListLoadAllConfigs(libxl_driver->domains, libxl_driver->stateDir, -- 1.8.1.4

At least Xen supports backend drivers in another domain (aka "driver domain"). This patch introduces XML config option for such setting as 'domain' element with 'name' attribute. Verification its content is left for the driver. In the future some option will be needed for USB devices (hostdev objects), but for now libxl doesn't have support for PVUSB. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - describe in docs/formatdomain.html.in - enforce empty domain tag (only 'name' attribute allowed) Changes in v3: - fix grammar of the documentation docs/formatdomain.html.in | 29 +++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 16 ++++++++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 4 files changed, 75 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 755d084..9c67b5b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1751,6 +1751,13 @@ </li> </ul> </dd> + <dt><code>domain</code></dt> + <dd>The optional <code>domain</code> element allows to specify a + backend domain (aka driver domain) for the device. If actual device/file + resides in some other domain on the same host, use the <code>name</code> + attribute to specify its name. + <span class="since">Since 1.0.7 (Xen only)</span> + </dd> <dt><code>boot</code></dt> <dd>Specifies that the disk is bootable. The <code>order</code> attribute determines the order in which devices will be tried during @@ -3554,6 +3561,28 @@ qemu-kvm -net nic,model=? /dev/null element is unspecified is to have the link state <code>up</code>. <span class="since">Since 0.9.5</span> </p> + <h5><a name="elementDomain">Setting up a network backend in a driver domain</a></h5> +<pre> + ... + <devices> + ... + <interface type='bridge'> + <source bridge='br0'/> + <b><domain name='netvm'/></b> + </interface> + ... + </devices> + ...</pre> + + <p> + The optional <code>domain</code> element allows to specify a backend + domain (aka driver domain) for the device. Use the <code>name</code> + attribute to specify its name. You can use it to create a direct network + link between domains (so data will not go through the host system). Use + with type 'ethernet' to create a plain network link, or with 'bridge' to + connect to some bridge inside the driver domain. + <span class="since">Since 1.0.7 (Xen only)</span> + </p> <h4><a name="elementsInput">Input devices</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 1eb2f68..6fe4d90 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -908,6 +908,14 @@ </optional> <ref name="target"/> <optional> + <element name="domain"> + <attribute name="name"> + <ref name="domainName"/> + </attribute> + <empty/> + </element> + </optional> + <optional> <ref name="deviceBoot"/> </optional> <optional> @@ -1981,6 +1989,14 @@ </element> </optional> <optional> + <element name="domain"> + <attribute name="name"> + <ref name="domainName"/> + </attribute> + <empty/> + </element> + </optional> + <optional> <element name="model"> <attribute name="type"> <data type="string"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2373397..5350c56 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1183,6 +1183,7 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def) VIR_FREE(def->wwn); VIR_FREE(def->vendor); VIR_FREE(def->product); + VIR_FREE(def->domain_name); if (def->auth.secretType == VIR_DOMAIN_DISK_SECRET_TYPE_USAGE) VIR_FREE(def->auth.secret.usage); virStorageEncryptionFree(def->encryption); @@ -1308,6 +1309,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def) VIR_FREE(def->virtPortProfile); VIR_FREE(def->script); + VIR_FREE(def->domain_name); VIR_FREE(def->ifname); virDomainDeviceInfoClear(&def->info); @@ -4693,6 +4695,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *vendor = NULL; char *product = NULL; char *discard = NULL; + char *domain_name = NULL; int expected_secret_usage = -1; int auth_secret_usage = -1; @@ -4853,6 +4856,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (target && STRPREFIX(target, "ioemu:")) memmove(target, target+6, strlen(target)-5); + } else if (!domain_name && + xmlStrEqual(cur->name, BAD_CAST "domain")) { + domain_name = virXMLPropString(cur, "name"); } else if (xmlStrEqual(cur->name, BAD_CAST "geometry")) { if (virXPathUInt("string(./geometry/@cyls)", ctxt, &def->geometry.cylinders) < 0) { @@ -5148,6 +5154,11 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = saved_node; } + if (domain_name != NULL) { + def->domain_name = domain_name; + domain_name = NULL; + } + if (target == NULL) { if (def->srcpool) { char *tmp; @@ -5504,6 +5515,7 @@ cleanup: VIR_FREE(wwn); VIR_FREE(vendor); VIR_FREE(product); + VIR_FREE(domain_name); ctxt->node = save_ctxt; return def; @@ -6109,6 +6121,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *mode = NULL; char *linkstate = NULL; char *addrtype = NULL; + char *domain_name = NULL; virNWFilterHashTablePtr filterparams = NULL; virDomainActualNetDefPtr actual = NULL; xmlNodePtr oldnode = ctxt->node; @@ -6207,6 +6220,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } else if (!script && xmlStrEqual(cur->name, BAD_CAST "script")) { script = virXMLPropString(cur, "path"); + } else if (!domain_name && + xmlStrEqual(cur->name, BAD_CAST "domain")) { + domain_name = virXMLPropString(cur, "name"); } else if (xmlStrEqual(cur->name, BAD_CAST "model")) { model = virXMLPropString(cur, "type"); } else if (xmlStrEqual(cur->name, BAD_CAST "driver")) { @@ -6437,6 +6453,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->script = script; script = NULL; } + if (domain_name != NULL) { + def->domain_name = domain_name; + domain_name = NULL; + } if (ifname != NULL) { def->ifname = ifname; ifname = NULL; @@ -6574,6 +6594,7 @@ cleanup: VIR_FREE(mode); VIR_FREE(linkstate); VIR_FREE(addrtype); + VIR_FREE(domain_name); virNWFilterHashTableFree(filterparams); return def; @@ -14018,6 +14039,11 @@ virDomainDiskDefFormat(virBufferPtr buf, if (virDomainDiskSourceDefFormat(buf, def) < 0) return -1; + + if (def->domain_name) { + virBufferEscapeString(buf, " <domain name='%s'/>\n", def->domain_name); + } + virDomainDiskGeometryDefFormat(buf, def); virDomainDiskBlockIoDefFormat(buf, def); @@ -14601,6 +14627,8 @@ virDomainNetDefFormat(virBufferPtr buf, return -1; virBufferEscapeString(buf, "<script path='%s'/>\n", def->script); + if (def->domain_name) + virBufferEscapeString(buf, "<domain name='%s'/>\n", def->domain_name); if (def->ifname && !((flags & VIR_DOMAIN_XML_INACTIVE) && (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5b159ac..24d3809 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -718,6 +718,7 @@ struct _virDomainDiskDef { int rawio; /* no = 0, yes = 1 */ int sgio; /* enum virDomainDeviceSGIO */ int discard; /* enum virDomainDiskDiscard */ + char *domain_name; /* backend domain name */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; @@ -981,6 +982,7 @@ struct _virDomainNetDef { unsigned long sndbuf; } tune; char *script; + char *domain_name; /* backend domain name */ char *ifname; virDomainDeviceInfo info; char *filter; -- 1.8.1.4

Marek Marczykowski-Górecki wrote:

This implement handling of <domain name=''/> parameter introduced in previous patch.

Lookup on domain name (to get domain ID) requires libxlDriverPrivate object, so it must be passed down to libxlMakeDisk and libxlMakeNet from top level callers.

Changes in v2: - rebase on 1.0.6+ - fix indentation - make libxl_name_to_domid switch more defensive

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 76 +++++++++++++++++++++++++++++++++++++++++++----- src/libxl/libxl_conf.h | 4 +-- src/libxl/libxl_driver.c | 51 +++++++++++++++++++------------- 3 files changed, 101 insertions(+), 30 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b9cb61e..623956e 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -441,7 +441,9 @@ error: }

int -libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) +libxlMakeDisk(libxlDriverPrivatePtr driver,

After looking at this again, I wonder if it is better to just pass the libxl_ctx to libxlMakeDisk and libxlMakeNic. All the callers already have a libxl_ctx, making for a much smaller patch. What do you think? Regards, Jim

+ virDomainDiskDefPtr l_disk, + libxl_device_disk *x_disk) { if (VIR_STRDUP(x_disk->pdev_path, l_disk->src) < 0) return -1; @@ -509,11 +511,39 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) return -1; }

+ if (l_disk->domain_name) { + uint32_t domid; + /* Do not use virDomainObjListFindByName as it causes deadlock here - + * we already have lock on this domain object, but + * virDomainObjListFindByName will try to take it again. + */ + switch (libxl_name_to_domid(driver->ctx, l_disk->domain_name, &domid)) { + case 0: + x_disk->backend_domid = domid; + break; + case ERROR_INVAL: + virReportError(VIR_ERR_XML_DETAIL, + _("Backend domain '%s' does not exists for disk '%s'"), + l_disk->domain_name, l_disk->dst); + return -1; + case ERROR_NOMEM: + virReportOOMError(); + return -1; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get ID of domain '%s'"), + l_disk->domain_name); + return -1; + } + } + return 0; }

static int -libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeDiskList(libxlDriverPrivatePtr driver, + virDomainDefPtr def, + libxl_domain_config *d_config) { virDomainDiskDefPtr *l_disks = def->disks; int ndisks = def->ndisks; @@ -526,7 +556,7 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config) }

for (i = 0; i < ndisks; i++) { - if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0) + if (libxlMakeDisk(driver, l_disks[i], &x_disks[i]) < 0) goto error; }

@@ -543,7 +573,9 @@ error: }

int -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic) +libxlMakeNic(libxlDriverPrivatePtr driver, + virDomainNetDefPtr l_nic, + libxl_device_nic *x_nic) { /* TODO: Where is mtu stored? * @@ -579,11 +611,39 @@ libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic) return -1; }

+ if (l_nic->domain_name) { + uint32_t domid; + /* Do not use virDomainObjListFindByName as it causes deadlock here - + * we already have lock on this domain object, but + * virDomainObjListFindByName will try to take it again. + */ + switch (libxl_name_to_domid(driver->ctx, l_nic->domain_name, &domid)) { + case 0: + x_nic->backend_domid = domid; + break; + case ERROR_INVAL: + virReportError(VIR_ERR_XML_DETAIL, + _("Backend domain '%s' does not exists for nic '%s'"), + l_nic->domain_name, l_nic->ifname); + return -1; + case ERROR_NOMEM: + virReportOOMError(); + return -1; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get ID of domain '%s'"), + l_nic->domain_name); + return -1; + } + } + return 0; }

static int -libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeNicList(libxlDriverPrivatePtr driver, + virDomainDefPtr def, + libxl_domain_config *d_config) { virDomainNetDefPtr *l_nics = def->nets; int nnics = def->nnets; @@ -598,7 +658,7 @@ libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) for (i = 0; i < nnics; i++) { x_nics[i].devid = i;

- if (libxlMakeNic(l_nics[i], &x_nics[i])) + if (libxlMakeNic(driver, l_nics[i], &x_nics[i])) goto error; }

@@ -756,11 +816,11 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver, return -1; }

- if (libxlMakeDiskList(def, d_config) < 0) { + if (libxlMakeDiskList(driver, def, d_config) < 0) { return -1; }

- if (libxlMakeNicList(def, d_config) < 0) { + if (libxlMakeNicList(driver, def, d_config) < 0) { return -1; }

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index e8fb9c4..33126f3 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -115,9 +115,9 @@ virCapsPtr libxlMakeCapabilities(libxl_ctx *ctx);

int -libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev); +libxlMakeDisk(libxlDriverPrivatePtr driver, virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev); int -libxlMakeNic(virDomainNetDefPtr l_nic, libxl_device_nic *x_nic); +libxlMakeNic(libxlDriverPrivatePtr driver, virDomainNetDefPtr l_nic, libxl_device_nic *x_nic); int libxlMakeVfb(libxlDriverPrivatePtr driver, virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0a0690d..df31001 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3233,8 +3233,10 @@ libxlDomainUndefine(virDomainPtr dom) }

static int -libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv, - virDomainObjPtr vm, virDomainDiskDefPtr disk) +libxlDomainChangeEjectableMedia(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) { virDomainDiskDefPtr origdisk = NULL; libxl_device_disk x_disk; @@ -3263,7 +3265,7 @@ libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv, return -1; }

- if (libxlMakeDisk(disk, &x_disk) < 0) + if (libxlMakeDisk(driver, disk, &x_disk) < 0) goto cleanup;

if ((ret = libxl_cdrom_insert(priv->ctx, vm->def->id, &x_disk, NULL)) < 0) { @@ -3288,8 +3290,10 @@ cleanup: }

static int -libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +libxlDomainAttachDeviceDiskLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { virDomainDiskDefPtr l_disk = dev->data.disk; libxl_device_disk x_disk; @@ -3297,7 +3301,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv,

switch (l_disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: - ret = libxlDomainChangeEjectableMedia(priv, vm, l_disk); + ret = libxlDomainChangeEjectableMedia(driver, priv, vm, l_disk); break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { @@ -3318,7 +3322,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv, goto cleanup; }

- if (libxlMakeDisk(l_disk, &x_disk) < 0) + if (libxlMakeDisk(driver, l_disk, &x_disk) < 0) goto cleanup;

if ((ret = libxl_device_disk_add(priv->ctx, vm->def->id, @@ -3349,8 +3353,10 @@ cleanup: }

static int -libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +libxlDomainDetachDeviceDiskLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { virDomainDiskDefPtr l_disk = NULL; libxl_device_disk x_disk; @@ -3371,7 +3377,7 @@ libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv,

l_disk = vm->def->disks[i];

- if (libxlMakeDisk(l_disk, &x_disk) < 0) + if (libxlMakeDisk(driver, l_disk, &x_disk) < 0) goto cleanup;

if ((ret = libxl_device_disk_remove(priv->ctx, vm->def->id, @@ -3403,14 +3409,15 @@ cleanup: }

static int -libxlDomainAttachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, +libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int ret = -1;

switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = libxlDomainAttachDeviceDiskLive(priv, vm, dev); + ret = libxlDomainAttachDeviceDiskLive(driver, priv, vm, dev); if (!ret) dev->data.disk = NULL; break; @@ -3455,14 +3462,16 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) }

static int -libxlDomainDetachDeviceLive(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, +libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, virDomainDeviceDefPtr dev) { int ret = -1;

switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = libxlDomainDetachDeviceDiskLive(priv, vm, dev); + ret = libxlDomainDetachDeviceDiskLive(driver, priv, vm, dev); break;

default: @@ -3502,8 +3511,10 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) }

static int -libxlDomainUpdateDeviceLive(libxlDomainObjPrivatePtr priv, - virDomainObjPtr vm, virDomainDeviceDefPtr dev) +libxlDomainUpdateDeviceLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) { virDomainDiskDefPtr disk; int ret = -1; @@ -3513,7 +3524,7 @@ libxlDomainUpdateDeviceLive(libxlDomainObjPrivatePtr priv, disk = dev->data.disk; switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: - ret = libxlDomainChangeEjectableMedia(priv, vm, disk); + ret = libxlDomainChangeEjectableMedia(driver, priv, vm, disk); if (ret == 0) dev->data.disk = NULL; break; @@ -3650,7 +3661,7 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;

- if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0) + if ((ret = libxlDomainAttachDeviceLive(driver, priv, vm, dev)) < 0) goto cleanup;

/* @@ -3755,7 +3766,7 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;

- if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0) + if ((ret = libxlDomainDetachDeviceLive(driver, priv, vm, dev)) < 0) goto cleanup;

/* @@ -3860,7 +3871,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;

- if ((ret = libxlDomainUpdateDeviceLive(priv, vm, dev)) < 0) + if ((ret = libxlDomainUpdateDeviceLive(driver, priv, vm, dev)) < 0) goto cleanup;

/*

Marek Marczykowski-Górecki wrote:

Vfb entries in domain config are used only by PV drivers. Qemu parameters are build based on b_info struct. So fill it with the same data as vfb entries (actually the first one). This will additionally allow graphic-less domain, when no <graphics/> entries are present in domain XML (previously VNC was always enabled).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 114 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 82 insertions(+), 32 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 623956e..e5d8dc5 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -332,7 +332,56 @@ error: }

static int -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeVNCInfo(libxlDriverPrivatePtr driver, + virDomainGraphicsDefPtr l_vfb, + libxl_vnc_info *x_vnc)

For HVM guests, I noticed this function is called twice, first from libxlMakeDomBuildInfo, then again via libxlMakeVfbList.

+{ + unsigned short port; + const char *listenAddr; + + libxl_defbool_set(&x_vnc->enable, 1); + /* driver handles selection of free port */ + libxl_defbool_set(&x_vnc->findunused, 0); + if (l_vfb->data.vnc.autoport) { + + if (virPortAllocatorAcquire(driver->reservedVNCPorts, &port) < 0)

Another port gets allocated on the second invocation

+ return -1; + if (port == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unable to find an unused VNC port")); + return -1; + } + l_vfb->data.vnc.port = port;

which updates libvirt's port info with incorrect info, since qemu gets launched with the port info in the b_info struct.

+ } + x_vnc->display = l_vfb->data.vnc.port - LIBXL_VNC_PORT_MIN; + + listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0); + if (listenAddr) { + /* libxl_device_vfb_init() does strdup("127.0.0.1") */

You'll need to change the strdup to VIR_STRDUP, even in the comment, to satisfy make syntax-check. Or tweak the syntax-check rule :).

+ VIR_FREE(x_vnc->listen); + if (VIR_STRDUP(x_vnc->listen, listenAddr) < 0) { + return -1; + } + } + return 0; +} + +static int +libxlMakeSDLInfo(virDomainGraphicsDefPtr l_vfb, + libxl_sdl_info *x_sdl) +{ + libxl_defbool_set(&x_sdl->enable, 1); + if (VIR_STRDUP(x_sdl->display, l_vfb->data.sdl.display) < 0) + return -1; + if (VIR_STRDUP(x_sdl->xauthority, l_vfb->data.sdl.xauth) < 0) + return -1; + return 0; +} + +static int +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, + virDomainDefPtr def, + libxl_domain_config *d_config) { libxl_domain_build_info *b_info = &d_config->b_info; int hvm = STREQ(def->os.type, "hvm"); @@ -404,6 +453,34 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config) if (VIR_STRDUP(b_info->u.hvm.boot, bootorder) < 0) goto error;

+ /* Disable VNC and SDL until explicitly enabled */ + libxl_defbool_set(&b_info->u.hvm.vnc.enable, 0); + libxl_defbool_set(&b_info->u.hvm.sdl.enable, 0); + + for (i = 0; i < def->ngraphics; i++) { + switch (def->graphics[i]->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + if (libxl_defbool_val(b_info->u.hvm.vnc.enable)) + continue; + if (libxlMakeVNCInfo(driver, def->graphics[i], + &b_info->u.hvm.vnc) < 0) + goto error; + if (def->graphics[i]->data.vnc.keymap && + (b_info->u.hvm.keymap = + strdup(def->graphics[i]->data.vnc.keymap)) == NULL) { + virReportOOMError();

Needs to be converted to VIR_STRDUP. Regards, Jim

+ goto error; + } + break; + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + if (libxl_defbool_val(b_info->u.hvm.sdl.enable)) + continue; + if (libxlMakeSDLInfo(def->graphics[i], &b_info->u.hvm.sdl) < 0) + goto error; + break; + } + } + /* * The following comment and calculation were taken directly from * libxenlight's internal function libxl_get_required_shadow_memory(): @@ -679,41 +756,14 @@ libxlMakeVfb(libxlDriverPrivatePtr driver, virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb) { - unsigned short port; - const char *listenAddr; - switch (l_vfb->type) { case VIR_DOMAIN_GRAPHICS_TYPE_SDL: - libxl_defbool_set(&x_vfb->sdl.enable, 1); - if (VIR_STRDUP(x_vfb->sdl.display, l_vfb->data.sdl.display) < 0) - return -1; - if (VIR_STRDUP(x_vfb->sdl.xauthority, l_vfb->data.sdl.xauth) < 0) + if (libxlMakeSDLInfo(l_vfb, &x_vfb->sdl) < 0) return -1; break; case VIR_DOMAIN_GRAPHICS_TYPE_VNC: - libxl_defbool_set(&x_vfb->vnc.enable, 1); - /* driver handles selection of free port */ - libxl_defbool_set(&x_vfb->vnc.findunused, 0); - if (l_vfb->data.vnc.autoport) { - - if (virPortAllocatorAcquire(driver->reservedVNCPorts, &port) < 0) - return -1; - if (port == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused VNC port")); - return -1; - } - l_vfb->data.vnc.port = port; - } - x_vfb->vnc.display = l_vfb->data.vnc.port - LIBXL_VNC_PORT_MIN; - - listenAddr = virDomainGraphicsListenGetAddress(l_vfb, 0); - if (listenAddr) { - /* libxl_device_vfb_init() does VIR_STRDUP("127.0.0.1") */ - VIR_FREE(x_vfb->vnc.listen); - if (VIR_STRDUP(x_vfb->vnc.listen, listenAddr) < 0) - return -1; - } + if (libxlMakeVNCInfo(driver, l_vfb, &x_vfb->vnc) < 0) + return -1; if (VIR_STRDUP(x_vfb->keymap, l_vfb->data.vnc.keymap) < 0) return -1; break; @@ -812,7 +862,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver, if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0) return -1;

- if (libxlMakeDomBuildInfo(def, d_config) < 0) { + if (libxlMakeDomBuildInfo(driver, def, d_config) < 0) { return -1; }

Marek Marczykowski-Górecki wrote:

While iterating with virDomainObjListForEach it is safe to remove current element. But while iterating, 'doms' lock is already taken, so can't use standard virDomainObjListRemove. So introduce virDomainObjListRemoveLocked for this purpose.

Changes in v2: - fix indentation

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 20 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5350c56..a4010da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2336,6 +2336,23 @@ void virDomainObjListRemove(virDomainObjListPtr doms, virObjectUnlock(doms); }

+/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove' + * requirements + * + * Can be used to remove current element while iterating with + * virDomainObjListForEach + */ +void virDomainObjListRemoveLocked(virDomainObjListPtr doms, + virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(dom->def->uuid, uuidstr); + virObjectUnlock(dom); + + virHashRemoveEntry(doms->objs, uuidstr);

Peter fixed a race in virDomainObjListRemove with commit b7c98329, and although this function expects the domain obj list to be locked on entry, it still seems some aspect of the race might exist here. Is it possible for another thread to lock the dom and execute some code on it after the freeing function has been triggered? Regards, Jim

+} + static int virDomainDeviceCCWAddressIsValid(virDomainDeviceCCWAddressPtr addr) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 24d3809..b8dfe19 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2234,6 +2234,8 @@ virDomainDefPtr virDomainObjCopyPersistentDef(virDomainObjPtr dom,

void virDomainObjListRemove(virDomainObjListPtr doms, virDomainObjPtr dom); +void virDomainObjListRemoveLocked(virDomainObjListPtr doms, + virDomainObjPtr dom);

virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 042081f..e21a14d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -274,6 +274,7 @@ virDomainObjListLoadAllConfigs; virDomainObjListNew; virDomainObjListNumOfDomains; virDomainObjListRemove; +virDomainObjListRemoveLocked; virDomainObjNew; virDomainObjSetDefTransient; virDomainObjSetState;

On 06/20/2013 10:24 AM, Marek Marczykowski-Górecki wrote:

On 20.06.2013 17:34, Jim Fehlig wrote:

...

Marek Marczykowski-Górecki wrote:

...

While iterating with virDomainObjListForEach it is safe to remove current element. But while iterating, 'doms' lock is already taken, so can't use standard virDomainObjListRemove. So introduce virDomainObjListRemoveLocked for this purpose.

Changes in v2: - fix indentation

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/conf/domain_conf.c | 17 +++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 20 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5350c56..a4010da 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2336,6 +2336,23 @@ void virDomainObjListRemove(virDomainObjListPtr doms, virObjectUnlock(doms); }

+/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove' + * requirements + * + * Can be used to remove current element while iterating with + * virDomainObjListForEach + */ +void virDomainObjListRemoveLocked(virDomainObjListPtr doms, + virDomainObjPtr dom) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(dom->def->uuid, uuidstr); + virObjectUnlock(dom); + + virHashRemoveEntry(doms->objs, uuidstr);

Peter fixed a race in virDomainObjListRemove with commit b7c98329, and although this function expects the domain obj list to be locked on entry, it still seems some aspect of the race might exist here. Is it possible for another thread to lock the dom and execute some code on it after the freeing function has been triggered? The next patch uses it only in libxlReconnectDomain (at driver initialization), with driver lock hold whole the time from the vm object creation. So in this particular case no other thread can access this vm object.

In general case I think it is still safe. The other thread need to have lock on 'doms' list to get access to 'dom' object. Because the caller also must have a lock on the list before a) getting lock on a 'dom' object, b) calling virDomainObjListRemoveLocked, it isn't possible for the other thread to access 'dom' object before caller release lock on the list.

Ah, I think I got it...

Hmm, this description isn't clear... So let me try to enumerate calls: Thread A (which removes the entry from the list): 1. virObjectLock(doms) 2. virObjectLock(dom) 3. virDomainObjListRemoveLocked(doms, dom) 4. virObjectUnlock(dom) 5. virHashRemoveEntry 6. virObjectUnlock(doms)

Above sequence can happen during virDomainObjListForEach.

If thread B tries to get lock on dom object, it should call: 1. virObjectLock(doms) (e.g. as part of *Lookup function) 1a. (lookup here) 2. virObjectLock(dom)

At no point thread B can get lock on 'doms' while thread A is in the middle of above code, so it can't reach dom object to get lock on it and use it. If thread B have lock on 'doms' or 'dom' before thread A starts above code, thread A will block until B finish.

and now I do, given the clarification. Thanks. This has been on the list for a while with no additional comments, so ACK and pushed. Regards, Jim

On 06/12/2013 07:54 PM, Marek Marczykowski-Górecki wrote:

Use virDomainObjListRemoveLocked instead of virDomainObjListRemove, as driver->domains is already taken by virDomainObjListForEach.

Above deadlock can be triggered when libvirtd is started after some domain have been started by hand (in which case driver will not find libvirt-xml domain config).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index df31001..b2cff5b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1100,7 +1100,7 @@ libxlReconnectDomain(virDomainObjPtr vm, out: libxlVmCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_UNKNOWN); if (!vm->persistent) - virDomainObjListRemove(driver->domains, vm); + virDomainObjListRemoveLocked(driver->domains, vm); else virObjectUnlock(vm);

Trivial once 6/14 ACK'ed. ACK and pushed. Regards, Jim

Marek Marczykowski-Górecki wrote:

Do not leave uninitialized variables, not all parameters are set in libxlMake*.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index e5d8dc5..d654ace 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -522,6 +522,8 @@ libxlMakeDisk(libxlDriverPrivatePtr driver, virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) { + libxl_device_disk_init(x_disk); + if (VIR_STRDUP(x_disk->pdev_path, l_disk->src) < 0) return -1;

@@ -659,6 +661,8 @@ libxlMakeNic(libxlDriverPrivatePtr driver, * x_nics[i].mtu = 1492; */

+ libxl_device_nic_init(x_nic); + virMacAddrGetRaw(&l_nic->mac, x_nic->mac);

if (l_nic->model && !STREQ(l_nic->model, "netfront")) { @@ -756,6 +760,8 @@ libxlMakeVfb(libxlDriverPrivatePtr driver, virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb) { + libxl_device_vfb_init(x_vfb);

This was already being called in libxlMakeVfbList, but agreed it is better called here. I rebased the patch, removed the extra libxl_device_vfb_init call in libxlMakeVfbList, and pushed the patch. Regards, Jim

+ switch (l_vfb->type) { case VIR_DOMAIN_GRAPHICS_TYPE_SDL: if (libxlMakeSDLInfo(l_vfb, &x_vfb->sdl) < 0) @@ -859,6 +865,8 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver, virDomainDefPtr def, libxl_domain_config *d_config) {

+ libxl_domain_config_init(d_config); + if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0) return -1;

On 20.06.2013 23:25, Jim Fehlig wrote:

Marek Marczykowski-Górecki wrote:

...

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8dec70b..7b50853 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2306,11 +2306,6 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, int ret = -1;

virCheckFlags(VIR_DOMAIN_SAVE_PAUSED, -1); - if (dxml) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - }

libxlDriverLock(driver);

@@ -2318,6 +2313,19 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, if (fd < 0) goto cleanup;

+ if (dxml) {

What kind of changes are supported in dxml? I tried passing xml that contained another vif, which caused libxl to fail the restore

What I use it for is (and is working): - change name (use the same savefile to create many VMs) - change IP - change backend disk image

libxl: error: libxl_device.c:878:device_backend_callback: unable to add device with path /local/domain/0/backend/vif/25/0

...

+ virDomainDefPtr def2 = NULL; + + if (!(def2 = virDomainDefParseString(dxml, driver->caps, driver->xmlopt, + 1 << VIR_DOMAIN_VIRT_XEN, + VIR_DOMAIN_XML_INACTIVE))) { + goto cleanup; + } + virDomainDefFree(def); + def = def2;

But we've already thrown away the original config and replaced it with config that subsequently fails.

The qemu driver calls virDomainDefCheckABIStability to ensure the new config is compatible, and IMO we should do the same here.

I'll take a look at it after my vacation (10.07)...

Regards, Jim

...

+ } + + if (!(vm = virDomainObjListAdd(driver->domains, def, driver->xmlopt, VIR_DOMAIN_OBJ_LIST_ADD_LIVE |

-- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab

On 06/12/2013 07:54 PM, Marek Marczykowski-Górecki wrote:

Both live and config.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_driver.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 161 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7b50853..ae0d4f7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3417,6 +3417,111 @@ cleanup: }

static int +libxlDomainAttachDeviceNetLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainNetDefPtr l_net = dev->data.net; + libxl_device_nic x_nic; + char mac[VIR_MAC_STRING_BUFLEN]; + int ret = -1; + + switch (dev->data.net->type) {

l_net->type ?

+ case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + /* -2 means "multiple matches" so then fail also */ + if (virDomainNetFindIdx(vm->def, dev->data.net) != -1) {

And just pass l_net here. Also, you should check for -2. I think this is better coded as idx = virDomainNetFindIdx(vm->def, l_net); if (idx == -2) { virReportError(VIR_ERR_OPERATION_FAILED, _("multiple devices matching mac address %s found"), virMacAddrFormat(&net->mac, mac)); return -1; } else if (idx < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("no matching network device was found")); return -1; }

+ virReportError(VIR_ERR_OPERATION_FAILED, + _("device matching mac address %s already exists"),

We should use the same error message as the qemu driver, which is included in my snippet above.

+ virMacAddrFormat(&dev->data.net->mac, mac)); + goto cleanup; + } + + if (libxlMakeNic(driver, l_net, &x_nic) < 0) + goto cleanup; + + if ((ret = libxl_device_nic_add(priv->ctx, vm->def->id, + &x_nic, NULL)) < 0) {

That indentation looks a little odd. Perhaps best to split into 2 lines ret = libxl_deivce_nic_add(...); if (ret < 0)

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("libxenlight failed to attach net '%s'"), + virMacAddrFormat(&dev->data.net->mac, mac)); + goto cleanup; + } + + virDomainNetInsert(vm->def, l_net); + + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("net device type '%s' cannot be hotplugged"),

s/net/network/

+ virDomainNetTypeToString(l_net->type)); + break; + } + +cleanup: + return ret;

Nothing to cleanup. Might as well just return errors where they occur or success when done.

+} + +static int +libxlDomainDetachDeviceNetLive(libxlDriverPrivatePtr driver, + libxlDomainObjPrivatePtr priv, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev) +{ + virDomainNetDefPtr l_net = NULL; + libxl_device_nic x_nic; + char mac[VIR_MAC_STRING_BUFLEN]; + int i; + int ret = -1; + + switch (dev->data.net->type) { + case VIR_DOMAIN_NET_TYPE_ETHERNET: + case VIR_DOMAIN_NET_TYPE_BRIDGE: + if ((i = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) { + if (i == -2) { + + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple devices matching mac address %s found"), + virMacAddrFormat(&dev->data.net->mac, mac)); + } + else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("network device %s not found"), + virMacAddrFormat(&dev->data.net->mac, mac)); + } + goto cleanup; + }

For consistency, the error checking logic here should be changed to match the Attach function.

+ + l_net = vm->def->nets[i]; + + if (libxlMakeNic(driver, l_net, &x_nic) < 0) + goto cleanup; + + if ((ret = libxl_device_nic_remove(priv->ctx, vm->def->id, + &x_nic, NULL)) < 0) {

Split into 2 lines?

+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("libxenlight failed to detach nic '%d'"), + i); + goto cleanup; + } + + virDomainNetRemove(vm->def, i); + virDomainNetDefFree(l_net); + + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("net device type '%s' cannot hot unplugged"),

s/net/network/

+ virDomainNetTypeToString(dev->data.net->type)); + break; + } + +cleanup: + return ret;

Nothing to cleanup.

+} + +static int libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver, libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, virDomainDeviceDefPtr dev) @@ -3430,6 +3535,12 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver, dev->data.disk = NULL; break;

+ case VIR_DOMAIN_DEVICE_NET: + ret = libxlDomainAttachDeviceNetLive(driver, priv, vm, dev); + if (!ret) + dev->data.net = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be attached"), @@ -3444,6 +3555,8 @@ static int libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net; + char mac[VIR_MAC_STRING_BUFLEN];

switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -3461,6 +3574,22 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) dev->data.disk = NULL; break;

+ case VIR_DOMAIN_DEVICE_NET: + net = dev->data.net; + if (virDomainNetFindIdx(vmdef, net) >= 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("net device with mac %s already exists."),

Reuse existing error message.

+ virMacAddrFormat(&net->mac, mac)); + return -1; + } + if (virDomainNetInsert(vmdef, net)) { + virReportOOMError(); + return -1; + } + /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ + dev->data.net = NULL; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent attach of device is not supported")); @@ -3482,6 +3611,10 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, ret = libxlDomainDetachDeviceDiskLive(driver, priv, vm, dev); break;

+ case VIR_DOMAIN_DEVICE_NET: + ret = libxlDomainDetachDeviceNetLive(driver, priv, vm, dev); + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("device type '%s' cannot be detached"), @@ -3495,20 +3628,45 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, static int libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) { - virDomainDiskDefPtr disk, detach; + virDomainDiskDefPtr disk, disk_detach; + virDomainNetDefPtr net, net_detach; + char mac[VIR_MAC_STRING_BUFLEN]; + int net_idx; int ret = -1;

switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if (!(detach = virDomainDiskRemoveByName(vmdef, disk->dst))) { + if (!(disk_detach = virDomainDiskRemoveByName(vmdef, disk->dst))) { virReportError(VIR_ERR_INVALID_ARG, _("no target device %s"), disk->dst); break; } - virDomainDiskDefFree(detach); + virDomainDiskDefFree(disk_detach); + ret = 0; + break; + + case VIR_DOMAIN_DEVICE_NET: + net = dev->data.net; + if ((net_idx = virDomainNetFindIdx(vmdef, net)) < 0) { + if (net_idx == -2) { + + virReportError(VIR_ERR_OPERATION_FAILED, + _("multiple devices matching mac address %s found"), + virMacAddrFormat(&dev->data.net->mac, mac)); + } + else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("network device %s not found"), + virMacAddrFormat(&dev->data.net->mac, mac)); + } + return -1; + }

Again for consistency, same error checking logic here. Regards, Jim

+ net_detach = virDomainNetRemove(vmdef, net_idx); + virDomainNetDefFree(net_detach); ret = 0; break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("persistent detach of device is not supported"));

On 06/12/2013 07:54 PM, Marek Marczykowski-Górecki wrote:

Do not silently ignore its value.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- src/libxl/libxl_conf.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d654ace..278c5e5 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -684,6 +684,8 @@ libxlMakeNic(libxlDriverPrivatePtr driver, case VIR_DOMAIN_NET_TYPE_ETHERNET: if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; + if (VIR_STRDUP(x_nic->ip, l_nic->data.bridge.ipaddr) < 0)

If the type is ETHERNET, shouldn't this be l_nic->data.ethernet.ipaddr ? Does this apply to type BRIDGE as well? Regards, Jim

+ return -1; break; default: virReportError(VIR_ERR_INTERNAL_ERROR,