On 03/21/2016 02:11 AM, Chunyan Liu wrote:

When AttachNetDevice failed, should call networkReleaseActualDevice to release actual device, and if actual device is hostdev, should remove the hostdev from vm->def->hostdevs.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_driver.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 87ec5a5..05ebe29 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3122,16 +3122,18 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, int ret = -1; char mac[VIR_MAC_STRING_BUFLEN];

+ libxl_device_nic_init(&nic); + /* preallocate new slot for device */ if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) - goto out; + goto cleanup;

/* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. */ if (networkAllocateActualDevice(vm->def, net) < 0) - goto out; + goto cleanup;

actualType = virDomainNetGetActualType(net);

@@ -3139,7 +3141,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, virReportError(VIR_ERR_INVALID_ARG, _("network device with mac %s already exists"), virMacAddrFormat(&net->mac, mac)); - return -1; + goto cleanup; }

if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { @@ -3150,10 +3152,9 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, */ ret = libxlDomainAttachHostDevice(driver, vm, virDomainNetGetActualHostdev(net)); - goto out; + goto cleanup; }

- libxl_device_nic_init(&nic); if (libxlMakeNic(vm->def, net, &nic) < 0) goto cleanup;

@@ -3167,9 +3168,12 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,

cleanup: libxl_device_nic_dispose(&nic); - out: - if (!ret) + if (!ret) { vm->def->nets[vm->def->nnets++] = net; + } else { + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); + }

Nice cleanup. But I think it can go a bit further by assigning the new net to nets right after a successful libxl_device_nic_add(), and only handle failure cleanup here. I've squashed in the change below. Regards, Jim diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 05ebe29..f6417ea 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3164,13 +3164,12 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, goto cleanup; } + vm->def->nets[vm->def->nnets++] = net; ret = 0; cleanup: libxl_device_nic_dispose(&nic); - if (!ret) { - vm->def->nets[vm->def->nnets++] = net; - } else { + if (ret) { virDomainNetRemoveHostdev(vm->def, net); networkReleaseActualDevice(vm->def, net); }

On 03/21/2016 02:11 AM, Chunyan Liu wrote:

Adjust codes to make it cleaner.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 05ebe29..74ebea4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3413,8 +3413,10 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, char mac[VIR_MAC_STRING_BUFLEN]; int ret = -1;

+ libxl_device_nic_init(&nic); + if ((detachidx = virDomainNetFindIdx(vm->def, net)) < 0) - goto out; + goto cleanup;

detach = vm->def->nets[detachidx];

@@ -3424,10 +3426,9 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, */ ret = libxlDomainDetachHostDevice(driver, vm, virDomainNetGetActualHostdev(detach)); - goto out; + goto cleanup; }

- libxl_device_nic_init(&nic); if (libxl_mac_to_device_nic(cfg->ctx, vm->def->id, virMacAddrFormat(&detach->mac, mac), &nic)) goto cleanup; @@ -3442,7 +3443,6 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,

cleanup: libxl_device_nic_dispose(&nic); - out: if (!ret) virDomainNetRemove(vm->def, detachidx); virObjectUnref(cfg);

Same comment here as in patch 1. The device can be removed after a successful libxl_device_nic_remove(). I've squashed in the below change. Regards, Jim diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index ae900d8..b2e23c0 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3438,12 +3438,11 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, goto cleanup; } + virDomainNetRemove(vm->def, detachidx); ret = 0; cleanup: libxl_device_nic_dispose(&nic); - if (!ret) - virDomainNetRemove(vm->def, detachidx); virObjectUnref(cfg); return ret; }

On 03/21/2016 02:11 AM, Chunyan Liu wrote:

When hostdev parent is network device, should call libxlDomainDetachNetDevice to detach the device from a higher level.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_driver.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 74ebea4..328dac8 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3443,8 +3443,10 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,

cleanup: libxl_device_nic_dispose(&nic); - if (!ret) + if (!ret) { + networkReleaseActualDevice(vm->def, detach); virDomainNetRemove(vm->def, detachidx); + }

There was a merge conflict here due to my change in patch 2.

virObjectUnref(cfg); return ret; } @@ -3467,8 +3469,12 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, break;

case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = libxlDomainDetachHostDevice(driver, vm, - dev->data.hostdev); + if (dev->data.hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) + ret = libxlDomainDetachNetDevice(driver, vm, + dev->data.hostdev->parent.data.net);

Indentation is odd for libvirt coding standards.

+ else + ret = libxlDomainDetachHostDevice(driver, vm, + dev->data.hostdev); break;

I think it would be good to have a local virDomainHostdevDefPtr variable to avoid the indirection. Also, the comment in the corresponding qemu code describing why a network hostdev is treated differently is worth repeating here. I've squashed in the below change. ACK to 1-3 with the minor adjustments, I'll push them shortly. Regards, Jim diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b2e23c0..cc7f224 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3438,6 +3438,7 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, goto cleanup; } + networkReleaseActualDevice(vm->def, detach); virDomainNetRemove(vm->def, detachidx); ret = 0; @@ -3452,6 +3453,7 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { + virDomainHostdevDefPtr hostdev; int ret = -1; switch (dev->type) { @@ -3465,8 +3467,16 @@ libxlDomainDetachDeviceLive(libxlDriverPrivatePtr driver, break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = libxlDomainDetachHostDevice(driver, vm, - dev->data.hostdev); + hostdev = dev->data.hostdev; + + /* If this is a network hostdev, we need to use the higher-level + * detach function so that mac address / virtualport are reset + */ + if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) + ret = libxlDomainDetachNetDevice(driver, vm, + hostdev->parent.data.net); + else + ret = libxlDomainDetachHostDevice(driver, vm, hostdev); break; default:

On 03/21/2016 02:11 AM, Chunyan Liu wrote:

Add codes to support creating domain with network defition of assigning SRIOV VF from a pool.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_domain.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 3 +++ 2 files changed, 53 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c8d09b1..d11bf3a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -36,6 +36,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "xen_common.h" +#include "network/bridge_driver.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, if (net->ifname && STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); + + /* cleanup actual device */ + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); } }

@@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) } }

+static int +libxlNetworkPrepareDevices(virDomainDefPtr def) +{ + int ret = -1; + size_t i; + + for (i = 0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + int actualType; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (networkAllocateActualDevice(def, net) < 0) + goto cleanup; + + actualType = virDomainNetGetActualType(net); + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + /* Each type='hostdev' network device must also have a + * corresponding entry in the hostdevs array. For netdevs + * that are hardcoded as type='hostdev', this is already + * done by the parser, but for those allocated from a + * network / determined at runtime, we need to do it + * separately. + */ + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); + virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; + + if (virDomainHostdevInsert(def, hostdev) < 0) + goto cleanup; + } + } + ret = 0; + cleanup: + return ret;

Nothing to cleanup here. I think we should just return -1 on failure paths and 0 on success.

+}

/* * Start a domain through libxenlight. @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm, true) < 0) goto cleanup;

+ if (libxlNetworkPrepareDevices(vm->def) < 0) + goto cleanup;

I accessed a machine to test this series and have found a few problems. 1. When attaching an SR-IOV vf from a pool, the attach appears successful. I see that xen's pciback driver is bound to the device in the host. I didn't see the device in the guest (could be a guest problem), nor in dumpxml or /var/run/libvirt/libxl/<dom-name>.xml. Not surprisingly, this causes problems with device-detach # virsh detach-device dom sriov-pool-vif.xml error: Failed to detach device from sriov-pool-vif.xml error: operation failed: no device matching mac address 00:16:3e:7a:35:df found 2. After starting a domain containing an interface from sr-iov vf pool, the interface xml is changed from type='network' to type='hostdev'. E.g. before creating domain <interface type='network'> <source network='passthrough'/> <mac address='00:16:3e:7a:35:df'/> </interface> after creating domain <interface type='hostdev' managed='yes'> <mac address='00:16:3e:7a:35:df'/> <driver name='xen'/> <source> <address type='pci' domain='0x0000' bus='0x0a' slot='0x11' function='0x1'/> </source> </interface> Maybe this is intended behavior, but it seems odd. 3. I started a domain containing an interface from sr-iov vf pool. Looks good. I then tried 'virsh detach-device ...', which never returned until keepalive timeout. The device was removed from the guest, but libvirtd got hung up in the process. Oddly, I wasn't able to interrupt libvirtd with gdb to see what its threads were doing. Do you have access to an appropriate machine to test these scenarios, to see if the issues are reproducible or just a problem with my configuration? Regards, Jim

...

...

On 3/29/2016 at 08:05 AM, in message <56F9C6D0.5000702@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: On 03/21/2016 02:11 AM, Chunyan Liu wrote: Add codes to support creating domain with network defition of assigning SRIOV VF from a pool.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_domain.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 3 +++ 2 files changed, 53 insertions(+)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c8d09b1..d11bf3a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -36,6 +36,7 @@ #include "virtime.h" #include "locking/domain_lock.h" #include "xen_common.h" +#include "network/bridge_driver.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -764,6 +765,10 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, if (net->ifname && STRPREFIX(net->ifname, LIBXL_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); + + /* cleanup actual device */ + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); } }

@@ -960,6 +965,48 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) } }

+static int +libxlNetworkPrepareDevices(virDomainDefPtr def) +{ + int ret = -1; + size_t i; + + for (i = 0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + int actualType; + + /* If appropriate, grab a physical device from the configured + * network's pool of devices, or resolve bridge device name + * to the one defined in the network definition. + */ + if (networkAllocateActualDevice(def, net) < 0) + goto cleanup; + + actualType = virDomainNetGetActualType(net); + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV && + net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + /* Each type='hostdev' network device must also have a + * corresponding entry in the hostdevs array. For netdevs + * that are hardcoded as type='hostdev', this is already + * done by the parser, but for those allocated from a + * network / determined at runtime, we need to do it + * separately. + */ + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); + virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; + + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; + + if (virDomainHostdevInsert(def, hostdev) < 0) + goto cleanup; + } + } + ret = 0; + cleanup: + return ret;

Nothing to cleanup here. I think we should just return -1 on failure paths and 0 on success.

...

+}

/* * Start a domain through libxenlight. @@ -1036,6 +1083,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm, true) < 0) goto cleanup;

+ if (libxlNetworkPrepareDevices(vm->def) < 0) + goto cleanup;

I accessed a machine to test this series and have found a few problems.

1. When attaching an SR-IOV vf from a pool, the attach appears successful. I see that xen's pciback driver is bound to the device in the host. I didn't see the device in the guest (could be a guest problem),

For pv guest, from testing, it is shown in guest. For HVM, seems xl pci-attach has the same result.

nor in dumpxml or /var/run/libvirt/libxl/<dom-name>.xml.

Ah, there is a mistake during a review change to original patch 1/6. Didn't notice that earlier. In libxlDomainAttachNetDevice: The if (!ret) is needed, maybe cleanup is not proper. Since for actual type is hostdev case, after a successful libxlDomainAttachHostDevice, we need to update vm->def->nets too. cleanup: libxl_device_nic_dispose(&nic); - out: - if (!ret) + if (!ret) { vm->def->nets[vm->def->nnets++] = net; + } else { + virDomainNetRemoveHostdev(vm->def, net); + networkReleaseActualDevice(vm->def, net); + } Similar for libxDomainDetachNetDevice cleanup: Original is still needed. cleanup: libxl_device_nic_dispose(&nic); - if (!ret) + if (!ret) { + networkReleaseActualDevice(vm->def, detach); virDomainNetRemove(vm->def, detachidx); + }

Not surprisingly, this causes problems with device-detach

# virsh detach-device dom sriov-pool-vif.xml error: Failed to detach device from sriov-pool-vif.xml error: operation failed: no device matching mac address 00:16:3e:7a:35:df found

2. After starting a domain containing an interface from sr-iov vf pool, the interface xml is changed from type='network' to type='hostdev'. E.g. before creating domain

<interface type='network'> <source network='passthrough'/> <mac address='00:16:3e:7a:35:df'/> </interface>

after creating domain

<interface type='hostdev' managed='yes'> <mac address='00:16:3e:7a:35:df'/> <driver name='xen'/> <source> <address type='pci' domain='0x0000' bus='0x0a' slot='0x11' function='0x1'/> </source> </interface>

Maybe this is intended behavior, but it seems odd.

This is intended.

3. I started a domain containing an interface from sr-iov vf pool. Looks good. I then tried 'virsh detach-device ...', which never returned until keepalive timeout. The device was removed from the guest, but libvirtd got hung up in the process. Oddly, I wasn't able to interrupt libvirtd with gdb to see what its threads were doing.

This is libxl problem. With xl pci-detach, it has the same issue.

Do you have access to an appropriate machine to test these scenarios, to see if the issues are reproducible or just a problem with my configuration?

Regards, Jim

On 03/21/2016 02:11 AM, Chunyan Liu wrote:

For those VF allocated from a network pool, we need to set its backend to be VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN so that later work can be correct.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_driver.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 328dac8..d7004fd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3145,13 +3145,23 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, }

if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virDomainHostdevDefPtr hostdev = virDomainNetGetActualHostdev(net); + virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; + + /* For those just allocated from a network pool whose backend is + * still VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT, we need to set + * backend correctly. + */ + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) + pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; + /* This is really a "smart hostdev", so it should be attached * as a hostdev (the hostdev code will reach over into the * netdev-specific code as appropriate), then also added to * the nets list (see out:) if successful. */ - ret = libxlDomainAttachHostDevice(driver, vm, - virDomainNetGetActualHostdev(net)); + ret = libxlDomainAttachHostDevice(driver, vm, hostdev);

ACK, will push shortly. I'm still looking at 4/6 and 6/6. Regards, Jim

On 03/21/2016 02:11 AM, Chunyan Liu wrote:

Functions like libxlNetworkPrepareDevices, libxlBuildDomainConfig, virHostdevPrepareDomainDevices will allocate and reserve some resources. In libxlDomainStart, after calling these functions, in case of failure, we need to call libxlDomainCleanup to check and release resoureces. Besides, since libxlDomainCleanup will call virDomainLockProcessPause, so we move virDomainLockProcessStart/Resume to earlier stage.

I think this cleanup should be done before applying 4/6. It is really independent of support for vf from an SR-IOV pool.

Signed-off-by: Chunyan Liu <cyliu@suse.com> --- src/libxl/libxl_domain.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index d11bf3a..6855ce4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1083,20 +1083,13 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm, true) < 0) goto cleanup;

- if (libxlNetworkPrepareDevices(vm->def) < 0) - goto cleanup; - - if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, - cfg->ctx, &d_config) < 0) - goto cleanup; - - if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) - goto cleanup; - - if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, - vm->def, VIR_HOSTDEV_SP_PCI) < 0) - goto cleanup; - + /* libxlNetworkPrepareDevices, libxlBuildDomainConfig, + * virHostdevPrepareDomainDevices will allocate and reserve + * some resources. In case of failure, need to release + * resoureces. This can be done by calling libxlDomainCleanup. + * Since libxlDomainCleanup will call virDomainLockProcessPause, + * so we move virDomainLockProcessStart/Resume to here. + */

I don't think the comment is necessary. Without looking at git history, one wouldn't know where it was moved from :-).

if (virDomainLockProcessStart(driver->lockManager, "xen:///system", vm, @@ -1111,6 +1104,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, goto cleanup; VIR_FREE(priv->lockState);

+ if (libxlNetworkPrepareDevices(vm->def) < 0) + goto release_dom; + + if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, + cfg->ctx, &d_config) < 0) + goto release_dom; + + if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) + goto release_dom; + + if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, + vm->def, VIR_HOSTDEV_SP_PCI) < 0) + goto release_dom; + /* Unlock virDomainObj while creating the domain */ virObjectUnlock(vm);

@@ -1194,16 +1201,12 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,

cleanup_dom: ret = -1; - if (priv->deathW) { - libxl_evdisable_domain_death(cfg->ctx, priv->deathW); - priv->deathW = NULL; - }

Disabling the death event should be done in another patch IMO, after adding the call to libxlDomainCleanup.

libxlDomainDestroyInternal(driver, vm); vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);

release_dom: - virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState); + libxlDomainCleanup(driver, vm);

release_dom is not the best name for the label after this change. I've split this patch into 3 patches and posted the result https://www.redhat.com/archives/libvir-list/2016-March/msg01310.html Can you review and comment on that series? As mentioned above, I'd like to get the resource leaks plugged in libxlDomainStart before adding 4/6. Regards, Jim