I agree. This must have slipped in during the countless iterations. Thanks for catching it. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; On 3/21/19 11:28 PM, Laine Stump wrote: -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Matthias Hartmann Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

When support for hotplug/unplug of SCSI controllers was added way back in December 2009 (commit da9d937b), unplug was handled by calling the now-extinct function qemuMonitorRemovePCIDevice(), which required a PCI address as an argument. At the same time, the idea of every device in the config having a PCI address apparentlty was not yet fully implemented, because the author of the patch including a check for a valid PCI address in the device object. These days, all PCI devices are guaranteed to have a valid PCI address. But more important than that, we no longer detach devices by PCI address, but instead use qemuDomainDeleteDevice(), which identifies the device by its alias. So checking for a valid PCI address is just pointless extra code that obscures the high level of similarity between all the individual qemuDomainDetach*Device() functions. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 65ec5187e6..f2aa3cc048 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5467,22 +5467,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, detach = vm->def->controllers[idx]; - if (detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && - detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - detach->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("device with '%s' address cannot be detached"), - virDomainDeviceAddressTypeToString(detach->info.type)); - goto cleanup; - } - - if (!virDomainDeviceAddressIsValid(&detach->info, detach->info.type)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("device with invalid '%s' address cannot be detached"), - virDomainDeviceAddressTypeToString(detach->info.type)); - goto cleanup; - } - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), -- 2.20.1

On Thu, Mar 21, 2019 at 06:28:45PM -0400, Laine Stump wrote: Wrong. As of commit e3a52afcfcc3478b553dca38140394fd93f90a2c qemu-attach: Assign device aliases that is not true. Although other than not crashing later, I'm not sure what the point of making up aliases that don't match reality is. Or what the point of qemu-attach is anymore - it does not seem to parse -device arguments, nor is it able to get us a JSON monitor, so we would not be able to detach the device anyway. Here you leave in the bogus alias assignment introduced by 9de26f27cfa6a32ce9a23e30a58991432bdcbee5 hotplug: Check for alias in hostdev detach which makes this check pointless. Unrelated to this patch, but it seems this could benefit from a typecast and an EnumError. With the qemu-attach aliaslessness allegations retracted and the double check for alias removed: Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano

qemuDomainDetachHostDevice() has a check at the end that calls qemuDomainDetachNetDevice() in the case that the hostdev is actually a Net device of type='hostdev'. A long time ago when device removal was (supposedly but not actually) synchronous, this would cause some extra code to be run prior to removing the device (e.g. restoring the original MAC address of the device, undoing some sort of virtual port profile, etc). For quite awhile now the device removal has been asynchronous, so that "extra teardown" isn't handled by the detach function, but instead is handled by the Remove function called at a later time. The result is that when we call qemuDomainDetachNetDevice() from qemuDomainDetachHostDevice(), it ends up just calling qemuDomainDetachThisHostDevice() and returning, which is exactly what we do for all other hostdevs anyway. Based on that, remove the behavioral difference when parent.type == VIR_DOMAIN_DEVICE_NET, and just call qemuDomainDetachThisHostDevice() for all hostdevs. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 389d16b090..af99f3bf4c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5640,13 +5640,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return -1; } - /* If this is a network hostdev, we need to use the higher-level detach - * function so that mac address / virtualport are reset - */ - if (detach->parent.type == VIR_DOMAIN_DEVICE_NET) - return qemuDomainDetachNetDevice(driver, vm, &detach->parent, async); - else - return qemuDomainDetachThisHostDevice(driver, vm, detach, async); + return qemuDomainDetachThisHostDevice(driver, vm, detach, async); } -- 2.20.1

qemuDomainDetachDiskDevice() is only called from one place. Moving the contents of the function to that place makes qemuDomainDetachDiskLive() more similar to the other Detach functions called by the toplevel qemuDomainDetachDevice(). The goal is to make each of the device-type-specific functions do this: 1) find the exact device 2) do any device-specific validation 3) do general validation 4) do device-specific shutdown (only needed for net devices) 5) do the common block of code to send device_del to qemu, then optionally wait for a corresponding DEVICE_DELETED event from qemu. with the final aim being that only items 1 & 2 will remain in each device-type-specific function, while 3 & 5 (which are the same for almost every type) will be de-duplicated and moved to the toplevel function that calls all of these (qemuDomainDetachDeviceLive(), which will also contain a callout to the one instance of (4) (netdev). Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 96 ++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index af99f3bf4c..820929b109 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5273,47 +5273,6 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, } -static int -qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr detach, - bool async) -{ - int ret = -1; - - if (qemuDomainDiskBlockJobIsActive(detach)) - return -1; - - if (detach->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - detach->dst); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); - - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; -} - static int qemuFindDisk(virDomainDefPtr def, const char *dst) { @@ -5333,25 +5292,26 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDeviceDefPtr dev, bool async) { - virDomainDiskDefPtr disk; + virDomainDiskDefPtr detach; int idx; + int ret = -1; if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("disk %s not found"), dev->data.disk->dst); return -1; } - disk = vm->def->disks[idx]; + detach = vm->def->disks[idx]; - switch ((virDomainDiskDevice) disk->device) { + switch ((virDomainDiskDevice) detach->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: - switch ((virDomainDiskBus) disk->bus) { + switch ((virDomainDiskBus) detach->bus) { case VIR_DOMAIN_DISK_BUS_VIRTIO: case VIR_DOMAIN_DISK_BUS_USB: case VIR_DOMAIN_DISK_BUS_SCSI: - return qemuDomainDetachDiskDevice(driver, vm, disk, async); + break; case VIR_DOMAIN_DISK_BUS_IDE: case VIR_DOMAIN_DISK_BUS_FDC: @@ -5361,12 +5321,12 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_BUS_SD: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); - break; + return -1; case VIR_DOMAIN_DISK_BUS_LAST: default: - virReportEnumRangeError(virDomainDiskBus, disk->bus); - break; + virReportEnumRangeError(virDomainDiskBus, detach->bus); + return -1; } break; @@ -5374,16 +5334,46 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_DEVICE_FLOPPY: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk device type '%s' cannot be detached"), - virDomainDiskDeviceTypeToString(disk->device)); - break; + virDomainDiskDeviceTypeToString(detach->device)); + return -1; case VIR_DOMAIN_DISK_DEVICE_LAST: default: - virReportEnumRangeError(virDomainDiskDevice, disk->device); + virReportEnumRangeError(virDomainDiskDevice, detach->device); break; } - return -1; + if (qemuDomainDiskBlockJobIsActive(detach)) + return -1; + + if (detach->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug multifunction PCI device: %s"), + detach->dst); + return -1; + } + + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &detach->info); + + if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { + if (virDomainObjIsActive(vm)) + virDomainAuditDisk(vm, detach->src, NULL, "detach", false); + goto cleanup; + } + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); + return ret; } -- 2.20.1

On 3/22/19 5:07 AM, Peter Krempa wrote: Yeah, that should go below the --, but I can never remember to do that when sending the patch, so I just include the comments in the commit message and remove it later. (Is there a way to add "meta comments"?) Ooh!! Good eye!!

It was sitting down in the middle of all the qemuDomainDetach*() functions. Move it up with the rest of the qemuDomain*Graphics*() functions. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 151 ++++++++++++++++++++-------------------- 1 file changed, 77 insertions(+), 74 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1f17e2a05e..ca89cf5d97 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4170,6 +4170,83 @@ qemuDomainFindGraphicsIndex(virDomainDefPtr def, return -1; } + +int +qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver, + virDomainObjPtr vm, + int type, + virDomainGraphicsAuthDefPtr auth, + const char *defaultPasswd, + int asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + time_t now = time(NULL); + const char *expire; + char *validTo = NULL; + const char *connected = NULL; + const char *password; + int ret = -1; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + if (!auth->passwd && !defaultPasswd) { + ret = 0; + goto cleanup; + } + password = auth->passwd ? auth->passwd : defaultPasswd; + + if (auth->connected) + connected = virDomainGraphicsAuthConnectedTypeToString(auth->connected); + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + goto cleanup; + ret = qemuMonitorSetPassword(priv->mon, type, password, connected); + + if (ret == -2) { + if (type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Graphics password only supported for VNC")); + ret = -1; + } else { + ret = qemuMonitorSetVNCPassword(priv->mon, password); + } + } + if (ret != 0) + goto end_job; + + if (password[0] == '\0' || + (auth->expires && auth->validTo <= now)) { + expire = "now"; + } else if (auth->expires) { + if (virAsprintf(&validTo, "%lu", (unsigned long)auth->validTo) < 0) + goto end_job; + expire = validTo; + } else { + expire = "never"; + } + + ret = qemuMonitorExpirePassword(priv->mon, type, expire); + + if (ret == -2) { + /* XXX we could fake this with a timer */ + if (auth->expires) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Expiry of passwords is not supported")); + ret = -1; + } else { + ret = 0; + } + } + + end_job: + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + cleanup: + VIR_FREE(validTo); + virObjectUnref(cfg); + return ret; +} + + int qemuDomainChangeGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5827,80 +5904,6 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, return ret; } -int -qemuDomainChangeGraphicsPasswords(virQEMUDriverPtr driver, - virDomainObjPtr vm, - int type, - virDomainGraphicsAuthDefPtr auth, - const char *defaultPasswd, - int asyncJob) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - time_t now = time(NULL); - const char *expire; - char *validTo = NULL; - const char *connected = NULL; - const char *password; - int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - if (!auth->passwd && !defaultPasswd) { - ret = 0; - goto cleanup; - } - password = auth->passwd ? auth->passwd : defaultPasswd; - - if (auth->connected) - connected = virDomainGraphicsAuthConnectedTypeToString(auth->connected); - - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto cleanup; - ret = qemuMonitorSetPassword(priv->mon, type, password, connected); - - if (ret == -2) { - if (type != VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Graphics password only supported for VNC")); - ret = -1; - } else { - ret = qemuMonitorSetVNCPassword(priv->mon, password); - } - } - if (ret != 0) - goto end_job; - - if (password[0] == '\0' || - (auth->expires && auth->validTo <= now)) { - expire = "now"; - } else if (auth->expires) { - if (virAsprintf(&validTo, "%lu", (unsigned long)auth->validTo) < 0) - goto end_job; - expire = validTo; - } else { - expire = "never"; - } - - ret = qemuMonitorExpirePassword(priv->mon, type, expire); - - if (ret == -2) { - /* XXX we could fake this with a timer */ - if (auth->expires) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Expiry of passwords is not supported")); - ret = -1; - } else { - ret = 0; - } - } - - end_job: - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; - cleanup: - VIR_FREE(validTo); - virObjectUnref(cfg); - return ret; -} int qemuDomainAttachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.20.1

There were two outliers at the end of the file beyond the Vcpu functions. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 174 ++++++++++++++++++++-------------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ca89cf5d97..094d135aac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6102,6 +6102,93 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, } +int +qemuDomainDetachInputDevice(virDomainObjPtr vm, + virDomainInputDefPtr def, + bool async) +{ + virDomainInputDefPtr input; + int ret = -1; + int idx; + + if ((idx = virDomainInputDefFind(vm->def, def)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching input device not found")); + return -1; + } + input = vm->def->inputs[idx]; + + switch ((virDomainInputBus) input->bus) { + case VIR_DOMAIN_INPUT_BUS_PS2: + case VIR_DOMAIN_INPUT_BUS_XEN: + case VIR_DOMAIN_INPUT_BUS_PARALLELS: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("input device on bus '%s' cannot be detached"), + virDomainInputBusTypeToString(input->bus)); + return -1; + + case VIR_DOMAIN_INPUT_BUS_LAST: + case VIR_DOMAIN_INPUT_BUS_USB: + case VIR_DOMAIN_INPUT_BUS_VIRTIO: + break; + } + + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &input->info); + + if (qemuDomainDeleteDevice(vm, input->info.alias) < 0) + goto cleanup; + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveInputDevice(vm, input); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); + return ret; +} + + +int +qemuDomainDetachVsockDevice(virDomainObjPtr vm, + virDomainVsockDefPtr dev, + bool async) +{ + virDomainVsockDefPtr vsock = vm->def->vsock; + int ret = -1; + + + if (!vsock || + !virDomainVsockDefEquals(dev, vsock)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("matching vsock device not found")); + return -1; + } + + if (!async) + qemuDomainMarkDeviceForRemoval(vm, &vsock->info); + + if (qemuDomainDeleteDevice(vm, vsock->info.alias) < 0) + goto cleanup; + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveVsockDevice(vm, vsock); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); + return ret; +} + + static int qemuDomainRemoveVcpu(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -6755,90 +6842,3 @@ qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virObjectUnref(cfg); return ret; } - - -int -qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def, - bool async) -{ - virDomainInputDefPtr input; - int ret = -1; - int idx; - - if ((idx = virDomainInputDefFind(vm->def, def)) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("matching input device not found")); - return -1; - } - input = vm->def->inputs[idx]; - - switch ((virDomainInputBus) input->bus) { - case VIR_DOMAIN_INPUT_BUS_PS2: - case VIR_DOMAIN_INPUT_BUS_XEN: - case VIR_DOMAIN_INPUT_BUS_PARALLELS: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("input device on bus '%s' cannot be detached"), - virDomainInputBusTypeToString(input->bus)); - return -1; - - case VIR_DOMAIN_INPUT_BUS_LAST: - case VIR_DOMAIN_INPUT_BUS_USB: - case VIR_DOMAIN_INPUT_BUS_VIRTIO: - break; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &input->info); - - if (qemuDomainDeleteDevice(vm, input->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveInputDevice(vm, input); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; -} - - -int -qemuDomainDetachVsockDevice(virDomainObjPtr vm, - virDomainVsockDefPtr dev, - bool async) -{ - virDomainVsockDefPtr vsock = vm->def->vsock; - int ret = -1; - - - if (!vsock || - !virDomainVsockDefEquals(dev, vsock)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("matching vsock device not found")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &vsock->info); - - if (qemuDomainDeleteDevice(vm, vsock->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveVsockDevice(vm, vsock); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; -} -- 2.20.1

On Thu, Mar 21, 2019 at 18:28:53 -0400, Laine Stump wrote: This function is not just moved ... [...] ... but replaced. This is a separate semantic change. I'd also prefer if these changes are made separately. [1] These header movements don't belong to this patch.

On Fri, Mar 22, 2019 at 09:36:31 -0400, Laine Stump wrote: Okay then. I'd split out the change to exported functions so that it's indeed just moving the described functions ... [...] I don't think you need to move them if you then delete them later. Anyways, if you want to keep them in sync, then move the headers in the patch which is moving the code. If there's more to move than just the headers for functions which are moved it'll be better to do a separate patch to clean up the headers.

On Thu, Mar 21, 2019 at 18:28:54 -0400, Laine Stump wrote: [...] This change is not mentioned in the commit message. Also it's not really functionally important.

qemuDomainDetachDeviceLive() is called from two places in qemu_driver.c. Move the call to qemuDomainUpdateDeviceList() from the end of that function, which is now in qemu_hotplug.c, to the two places that call qemuDomainDetachDeviceLive(). Removing that one call from qemuDomainDetachDeviceList() will permit us to call it from the test driver, replacing the separate calls to qemuDomainDetachDeviceDiskLive(), qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog(). We want to do this so that part of the common functionality of those three functions (and the rest of the device-specific Detach functions) can be pulled up into qemuDomainDetachDeviceLive() without breaking the test. (This is done in the next patch). Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_driver.c | 7 +++++++ src/qemu/qemu_hotplug.c | 3 --- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0b80004c6e..8e7bb913a2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8932,6 +8932,10 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (qemuDomainDetachDeviceLive(vm, dev_copy, driver, false) < 0) goto cleanup; + + if (qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -9014,6 +9018,9 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, if (qemuDomainDetachDeviceLive(vm, &dev, driver, true) < 0) goto cleanup; + + if (qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; } if (vmdef) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6b713e1c27..ea5b6bf8ac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6271,9 +6271,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, break; } - if (ret == 0) - ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE); - return ret; } -- 2.20.1

On Thu, Mar 21, 2019 at 18:28:57 -0400, Laine Stump wrote: This is again doing something not mentioned in the commit message.

Most of these functions will soon contain only some setup for detaching the device, not the detach code proper (since that code is identical for these devices). Their device specific functions are all being renamed to qemuDomainDetachPrep*(), where * is the name of that device's data member in the virDomainDeviceDef object. Since there will be other code in qemuDomainDetachDeviceLive() after the calls to qemuDomainDetachPrep*() that could still fail, we no longer directly set "ret" with the return code from qemuDomainDetachPrep*() functions, but simply return -1 on failure, and wait until the end of qemuDomainDetachDeviceLive() to set ret = 0. Two of the functions (for Lease and Chr) are atypical, and can't be easily consolidated with the others, so they are just being named qemuDomainDetachDevice*() to make it clearer that they perform the entire operation and not just some setup. Along with the rename, qemuDomainDetachPrep*() functions are also given similar arglists, including an arg called "match" that points to the proto-object of the device we want to delete, and another arg "detach" that is used to return a pointer to the actual object that will be (for now *has been*) detached. To make sure these new args aren't confused with existing local pointers that sometimes had the same name (detach), the local pointer to the device is now named after the device type ("controller", "disk", etc). These point to the same place as (*detach)->data.blah, it's just easier on the eyes to have, e.g., "disk->dst" rather than "(*detach)->data.disk-dst". Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 363 +++++++++++++++++++++++----------------- 1 file changed, 205 insertions(+), 158 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 903a0c46eb..b0e2c738b9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5390,27 +5390,28 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) } static int -qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr match, + virDomainDiskDefPtr *detach, + bool async) { - virDomainDiskDefPtr detach; + virDomainDiskDefPtr disk; int idx; int ret = -1; - if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) { + if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); + _("disk %s not found"), match->dst); return -1; } - detach = vm->def->disks[idx]; + *detach = disk = vm->def->disks[idx]; - switch ((virDomainDiskDevice) detach->device) { + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: - switch ((virDomainDiskBus) detach->bus) { + switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_VIRTIO: case VIR_DOMAIN_DISK_BUS_USB: case VIR_DOMAIN_DISK_BUS_SCSI: @@ -5428,7 +5429,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_BUS_LAST: default: - virReportEnumRangeError(virDomainDiskBus, detach->bus); + virReportEnumRangeError(virDomainDiskBus, disk->bus); return -1; } break; @@ -5437,32 +5438,32 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_DEVICE_FLOPPY: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk device type '%s' cannot be detached"), - virDomainDiskDeviceTypeToString(detach->device)); + virDomainDiskDeviceTypeToString(disk->device)); return -1; case VIR_DOMAIN_DISK_DEVICE_LAST: default: - virReportEnumRangeError(virDomainDiskDevice, detach->device); + virReportEnumRangeError(virDomainDiskDevice, disk->device); break; } - if (qemuDomainDiskBlockJobIsActive(detach)) + if (qemuDomainDiskBlockJobIsActive(disk)) return -1; - if (detach->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + qemuIsMultiFunctionDevice(vm->def, &disk->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), - detach->dst); + disk->dst); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + qemuDomainMarkDeviceForRemoval(vm, &disk->info); - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { + if (qemuDomainDeleteDevice(vm, disk->info.alias) < 0) { if (virDomainObjIsActive(vm)) - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); + virDomainAuditDisk(vm, disk->src, NULL, "detach", false); goto cleanup; } @@ -5470,7 +5471,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); + ret = qemuDomainRemoveDiskDevice(driver, vm, disk); } cleanup: @@ -5541,57 +5542,55 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } static int -qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepController(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainControllerDefPtr match, + virDomainControllerDefPtr *detach, + bool async) { + virDomainControllerDefPtr controller; int idx, ret = -1; - virDomainControllerDefPtr detach = NULL; - if (dev->data.controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("'%s' controller cannot be hot unplugged."), - virDomainControllerTypeToString(dev->data.controller->type)); + virDomainControllerTypeToString(match->type)); return -1; } - if ((idx = virDomainControllerFind(vm->def, - dev->data.controller->type, - dev->data.controller->idx)) < 0) { + if ((idx = virDomainControllerFind(vm->def, match->type, match->idx)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("controller %s:%d not found"), - virDomainControllerTypeToString(dev->data.controller->type), - dev->data.controller->idx); + virDomainControllerTypeToString(match->type), + match->idx); goto cleanup; } - detach = vm->def->controllers[idx]; + *detach = controller = vm->def->controllers[idx]; - if (qemuDomainControllerIsBusy(vm, detach)) { + if (qemuDomainControllerIsBusy(vm, controller)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached: device is busy")); goto cleanup; } - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - dev->data.disk->dst); + if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot hot unplug multifunction PCI device")); goto cleanup; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + qemuDomainMarkDeviceForRemoval(vm, &controller->info); - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveControllerDevice(driver, vm, detach); + ret = qemuDomainRemoveControllerDevice(driver, vm, controller); } cleanup: @@ -5603,29 +5602,30 @@ qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, /* search for a hostdev matching dev and detach it */ static int -qemuDomainDetachHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr match, + virDomainHostdevDefPtr *detach, + bool async) { - virDomainHostdevDefPtr hostdev = dev->data.hostdev; - virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virDomainHostdevSubsysPtr subsys = &match->source.subsys; virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &subsys->u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &subsys->u.scsi; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; - virDomainHostdevDefPtr detach = NULL; + virDomainHostdevDefPtr hostdev; int idx; int ret = -1; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hot unplug is not supported for hostdev mode '%s'"), - virDomainHostdevModeTypeToString(hostdev->mode)); + virDomainHostdevModeTypeToString(match->mode)); return -1; } - idx = virDomainHostdevFind(vm->def, hostdev, &detach); + idx = virDomainHostdevFind(vm->def, match, &hostdev); + *detach = hostdev; if (idx < 0) { switch (subsys->type) { @@ -5678,30 +5678,30 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return -1; } - if (qemuAssignDeviceHostdevAlias(vm->def, &detach->info->alias, -1) < 0) + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) return -1; - if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device with guest address: " "%.4x:%.2x:%.2x.%.1x"), - detach->info->addr.pci.domain, detach->info->addr.pci.bus, - detach->info->addr.pci.slot, detach->info->addr.pci.function); + hostdev->info->addr.pci.domain, hostdev->info->addr.pci.bus, + hostdev->info->addr.pci.slot, hostdev->info->addr.pci.function); return -1; } - if (!detach->info->alias) { + if (!hostdev->info->alias) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached without a device alias")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, detach->info); + qemuDomainMarkDeviceForRemoval(vm, hostdev->info); - if (qemuDomainDeleteDevice(vm, detach->info->alias) < 0) { + if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) { if (virDomainObjIsActive(vm)) - virDomainAuditHostdev(vm, detach, "detach", false); + virDomainAuditHostdev(vm, hostdev, "detach", false); goto cleanup; } @@ -5709,7 +5709,7 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveHostDevice(driver, vm, detach); + ret = qemuDomainRemoveHostDevice(driver, vm, hostdev); } cleanup: @@ -5722,24 +5722,25 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainShmemDefPtr dev, - bool async) +qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr match, + virDomainShmemDefPtr *detach, + bool async) { int ret = -1; ssize_t idx = -1; - virDomainShmemDefPtr shmem = NULL; + virDomainShmemDefPtr shmem; - if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) { + if ((idx = virDomainShmemDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' shmem device not present " "in domain configuration"), - virDomainShmemModelTypeToString(dev->model)); + virDomainShmemModelTypeToString(match->model)); return -1; } - shmem = vm->def->shmems[idx]; + *detach = shmem = vm->def->shmems[idx]; switch ((virDomainShmemModel)shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: @@ -5776,13 +5777,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainWatchdogDefPtr dev, - bool async) +qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr match, + virDomainWatchdogDefPtr *detach, + bool async) { int ret = -1; - virDomainWatchdogDefPtr watchdog = vm->def->watchdog; + virDomainWatchdogDefPtr watchdog; + + + *detach = watchdog = vm->def->watchdog; if (!watchdog) { virReportError(VIR_ERR_DEVICE_MISSING, "%s", @@ -5793,9 +5798,9 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, /* While domains can have up to one watchdog, the one supplied by the user * doesn't necessarily match the one domain has. Refuse to detach in such * case. */ - if (!(watchdog->model == dev->model && - watchdog->action == dev->action && - virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) { + if (!(watchdog->model == match->model && + watchdog->action == match->action && + virDomainDeviceInfoAddressIsEqual(&match->info, &watchdog->info))) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' watchdog device not present " "in domain configuration"), @@ -5831,40 +5836,41 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, static int -qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRedirdevDefPtr dev, - bool async) +qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr match, + virDomainRedirdevDefPtr *detach, + bool async) { int ret = -1; - virDomainRedirdevDefPtr tmpRedirdevDef; + virDomainRedirdevDefPtr redirdev; ssize_t idx; - if ((idx = virDomainRedirdevDefFind(vm->def, dev)) < 0) { + if ((idx = virDomainRedirdevDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("no matching redirdev was not found")); return -1; } - tmpRedirdevDef = vm->def->redirdevs[idx]; + *detach = redirdev = vm->def->redirdevs[idx]; - if (!tmpRedirdevDef->info.alias) { + if (!redirdev->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("alias not set for redirdev device")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info); + qemuDomainMarkDeviceForRemoval(vm, &redirdev->info); - if (qemuDomainDeleteDevice(vm, tmpRedirdevDef->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef); + ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev); } cleanup: @@ -5875,53 +5881,54 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachNetDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepNet(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainNetDefPtr match, + virDomainNetDefPtr *detach, + bool async) { int detachidx, ret = -1; - virDomainNetDefPtr detach = NULL; + virDomainNetDefPtr net; - if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) + if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0) goto cleanup; - detach = vm->def->nets[detachidx]; + *detach = net = vm->def->nets[detachidx]; - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, &net->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), - detach->ifname); + net->ifname); goto cleanup; } - if (!detach->info.alias) { - if (qemuAssignDeviceNetAlias(vm->def, detach, -1) < 0) + if (!net->info.alias) { + if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) goto cleanup; } - if (virDomainNetGetActualBandwidth(detach) && - virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) && - virNetDevBandwidthClear(detach->ifname) < 0) + if (virDomainNetGetActualBandwidth(net) && + virNetDevSupportBandwidth(virDomainNetGetActualType(net)) && + virNetDevBandwidthClear(net->ifname) < 0) VIR_WARN("cannot clear bandwidth setting for device : %s", - detach->ifname); + net->ifname); /* deactivate the tap/macvtap device on the host, which could also * affect the parent device (e.g. macvtap passthrough mode sets * the parent device offline) */ - ignore_value(qemuInterfaceStopDevice(detach)); + ignore_value(qemuInterfaceStopDevice(net)); if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + qemuDomainMarkDeviceForRemoval(vm, &net->info); - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { + if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) { if (virDomainObjIsActive(vm)) { /* the audit message has a different format for hostdev network devices */ - if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) - virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(detach), "detach", false); + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) + virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net), "detach", false); else - virDomainAuditNet(vm, detach, NULL, "detach", false); + virDomainAuditNet(vm, net, NULL, "detach", false); } goto cleanup; } @@ -5930,7 +5937,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, detach); + ret = qemuDomainRemoveNetDevice(driver, vm, net); } cleanup: @@ -5941,7 +5948,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachChrDevice(virQEMUDriverPtr driver, +qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr, bool async) @@ -5999,42 +6006,43 @@ qemuDomainDetachChrDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRNGDefPtr rng, - bool async) +qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr match, + virDomainRNGDefPtr *detach, + bool async) { ssize_t idx; - virDomainRNGDefPtr tmpRNG; + virDomainRNGDefPtr rng; int ret = -1; - if ((idx = virDomainRNGFind(vm->def, rng)) < 0) { + if ((idx = virDomainRNGFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' RNG device not present " "in domain configuration"), - virDomainRNGBackendTypeToString(rng->model)); + virDomainRNGBackendTypeToString(match->model)); return -1; } - tmpRNG = vm->def->rngs[idx]; + *detach = rng = vm->def->rngs[idx]; - if (!tmpRNG->info.alias) { + if (!rng->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("alias not set for RNG device")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + qemuDomainMarkDeviceForRemoval(vm, &rng->info); - if (qemuDomainDeleteDevice(vm, tmpRNG->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + ret = qemuDomainRemoveRNGDevice(driver, vm, rng); } cleanup: @@ -6045,26 +6053,27 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainMemoryDefPtr memdef, - bool async) +qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr match, + virDomainMemoryDefPtr *detach, + bool async) { virDomainMemoryDefPtr mem; int idx; int ret = -1; - qemuDomainMemoryDeviceAlignSize(vm->def, memdef); + qemuDomainMemoryDeviceAlignSize(vm->def, match); - if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { + if ((idx = virDomainMemoryFindByDef(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' memory device not present " "in the domain configuration"), - virDomainMemoryModelTypeToString(memdef->model)); + virDomainMemoryModelTypeToString(match->model)); return -1; } - mem = vm->def->mems[idx]; + *detach = mem = vm->def->mems[idx]; if (!mem->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6093,20 +6102,21 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def, - bool async) +qemuDomainDetachPrepInput(virDomainObjPtr vm, + virDomainInputDefPtr match, + virDomainInputDefPtr *detach, + bool async) { virDomainInputDefPtr input; int ret = -1; int idx; - if ((idx = virDomainInputDefFind(vm->def, def)) < 0) { + if ((idx = virDomainInputDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("matching input device not found")); return -1; } - input = vm->def->inputs[idx]; + *detach = input = vm->def->inputs[idx]; switch ((virDomainInputBus) input->bus) { case VIR_DOMAIN_INPUT_BUS_PS2: @@ -6144,16 +6154,18 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm, static int -qemuDomainDetachVsockDevice(virDomainObjPtr vm, - virDomainVsockDefPtr dev, - bool async) +qemuDomainDetachPrepVsock(virDomainObjPtr vm, + virDomainVsockDefPtr match, + virDomainVsockDefPtr *detach, + bool async) { - virDomainVsockDefPtr vsock = vm->def->vsock; + virDomainVsockDefPtr vsock; int ret = -1; + *detach = vsock = vm->def->vsock; if (!vsock || - !virDomainVsockDefEquals(dev, vsock)) { + !virDomainVsockDefEquals(match, vsock)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("matching vsock device not found")); return -1; @@ -6180,9 +6192,9 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm, static int -qemuDomainDetachLease(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainLeaseDefPtr lease) +qemuDomainDetachDeviceLease(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainLeaseDefPtr lease) { virDomainLeaseDefPtr det_lease; int idx; @@ -6209,6 +6221,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, virQEMUDriverPtr driver, bool async) { + virDomainDeviceDef detach = { .type = match->type }; int ret = -1; switch ((virDomainDeviceType)match->type) { @@ -6218,10 +6231,10 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * Detach functions. */ case VIR_DOMAIN_DEVICE_LEASE: - return qemuDomainDetachLease(driver, vm, match->data.lease); + return qemuDomainDetachDeviceLease(driver, vm, match->data.lease); case VIR_DOMAIN_DEVICE_CHR: - return qemuDomainDetachChrDevice(driver, vm, match->data.chr, async); + return qemuDomainDetachDeviceChr(driver, vm, match->data.chr, async); /* * All the other device types follow a very similar pattern - @@ -6231,38 +6244,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * is okay to detach the device. */ case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async); + if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk, + &detach.data.disk, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachControllerDevice(driver, vm, match, async); + if (qemuDomainDetachPrepController(driver, vm, match->data.controller, + &detach.data.controller, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainDetachNetDevice(driver, vm, match, async); + if (qemuDomainDetachPrepNet(driver, vm, match->data.net, + &detach.data.net, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainDetachHostDevice(driver, vm, match, async); + if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev, + &detach.data.hostdev, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async); + if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng, + &detach.data.rng, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDetachMemoryDevice(driver, vm, match->data.memory, async); + if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory, + &detach.data.memory, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(driver, vm, match->data.shmem, async); + if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem, + &detach.data.shmem, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(driver, vm, match->data.watchdog, async); + if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog, + &detach.data.watchdog, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainDetachInputDevice(vm, match->data.input, async); + if (qemuDomainDetachPrepInput(vm, match->data.input, + &detach.data.input, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainDetachRedirdevDevice(driver, vm, match->data.redirdev, async); + if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev, + &detach.data.redirdev, async) < 0) { + return -1; + } break; - case VIR_DOMAIN_DEVICE_VSOCK: - ret = qemuDomainDetachVsockDevice(vm, match->data.vsock, async); + if (qemuDomainDetachPrepVsock(vm, match->data.vsock, + &detach.data.vsock, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_FS: @@ -6284,6 +6329,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; } + ret = 0; + return ret; } -- 2.20.1

On 3/22/19 8:10 AM, Peter Krempa wrote: Well... I don't see the point in *keeping* it. :-P It's never used without being set, so it's pointless. And not having a NULL initialization makes it more consistent with the functions for other device types. Same comment as above. But if you're really attached to it, then I'll continue initializing it to NULL. When more code is added here in patch 20, ret will once again be used. I figured it would be better to leave it in, rather than remove it just to add it back. I can do that if you want though.

On Thu, Mar 21, 2019 at 18:28:59 -0400, Laine Stump wrote: Don't use inline, it's pointless. @success is always false in the one call place the other patches add. Especially I doubt it will ever be different. All async device deletion callbacks do their own per-device-type audit call on success. This function is unused thus breaks build. This is currently not audited, I think it should be added separately.

Now that all the qemuDomainDetachPrep*() functions look nearly identical at the end, we can put one copy of that identical code in qemuDomainDetachDeviceLive() at the point after the individual prep functions have been called, and remove the duplicated code from all the prep functions. The code to locate the target "detach" device based on the "match" device remains, as do all device-type-specific validations. Unfortunately there are a few things going on at once in this patch, which makes it a bit more difficult to follow than the others; it was just impossible to do the changes in stages and still have a buildable/testable tree at each step. The other changes of note: * The individual prep functions no longer need their driver or async args, so those are removed, as are the local "ret" variables, since in all cases the functions just directly return -1 or 0. * Some of the prep functions were checking for a valid alias and/or for attempts to detach a multifunction PCI device, but not all. In fact, both checks are valid (or at least harmless) for *all* device types, so they are removed from the prep functions, and done a single time in the common function. (any attempts to *create* an alias when there isn't one has been removed, since that is doomed to failure anyway; the only way the device wouldn't have an alias is if 1) the domain was created by calling virsh qemu-attach to attach an existing qemu process to libvirt, and 2) the qemu command that started said process used "old style" arguments for creating devices that didn't have any device ids. Even if we constructed a device id for one of these devices, qemu wouldn't recognize it in the device_del command anyway, so we may as well fail earlier with "device missing alias" rather than failing later with "couldn't delete device net0".) * Only one type of device has shutdown code that must not be called until after *all* validation of the device is done (including checking for multifunction PCI and valid alias, which is done in the toplevel common code). For this reason, the Net function has been split in two, with the 2nd half (qemuDomainDetachShutdownNet()) called from the common function, right before sending the delete command to qemu. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- src/qemu/qemu_hotplug.c | 497 ++++++++++++---------------------------- 1 file changed, 149 insertions(+), 348 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 5e5ffe16d3..de7a7a2c95 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,7 +5208,7 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, } -static inline void +static void qemuDomainRemoveAuditDevice(virDomainObjPtr vm, virDomainDeviceDefPtr detach, bool success) @@ -5462,15 +5462,12 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) } static int -qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepDisk(virDomainObjPtr vm, virDomainDiskDefPtr match, - virDomainDiskDefPtr *detach, - bool async) + virDomainDiskDefPtr *detach) { virDomainDiskDefPtr disk; int idx; - int ret = -1; if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -5522,34 +5519,7 @@ qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, if (qemuDomainDiskBlockJobIsActive(disk)) return -1; - if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - qemuIsMultiFunctionDevice(vm->def, &disk->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - disk->dst); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &disk->info); - - if (qemuDomainDeleteDevice(vm, disk->info.alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditDisk(vm, disk->src, NULL, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, disk); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } @@ -5614,14 +5584,12 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } static int -qemuDomainDetachPrepController(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepController(virDomainObjPtr vm, virDomainControllerDefPtr match, - virDomainControllerDefPtr *detach, - bool async) + virDomainControllerDefPtr *detach) { virDomainControllerDefPtr controller; - int idx, ret = -1; + int idx; if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -5635,7 +5603,7 @@ qemuDomainDetachPrepController(virQEMUDriverPtr driver, _("controller %s:%d not found"), virDomainControllerTypeToString(match->type), match->idx); - goto cleanup; + return -1; } *detach = controller = vm->def->controllers[idx]; @@ -5643,42 +5611,18 @@ qemuDomainDetachPrepController(virQEMUDriverPtr driver, if (qemuDomainControllerIsBusy(vm, controller)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached: device is busy")); - goto cleanup; - } - - if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("cannot hot unplug multifunction PCI device")); - goto cleanup; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &controller->info); - - if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveControllerDevice(driver, vm, controller); + return -1; } - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } /* search for a hostdev matching dev and detach it */ static int -qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr match, - virDomainHostdevDefPtr *detach, - bool async) + virDomainHostdevDefPtr *detach) { virDomainHostdevSubsysPtr subsys = &match->source.subsys; virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; @@ -5687,7 +5631,6 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; virDomainHostdevDefPtr hostdev; int idx; - int ret = -1; if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -5750,58 +5693,16 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, return -1; } - if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) - return -1; - - if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device with guest address: " - "%.4x:%.2x:%.2x.%.1x"), - hostdev->info->addr.pci.domain, hostdev->info->addr.pci.bus, - hostdev->info->addr.pci.slot, hostdev->info->addr.pci.function); - return -1; - } - - if (!hostdev->info->alias) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("device cannot be detached without a device alias")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, hostdev->info); - - if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditHostdev(vm, hostdev, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveHostDevice(driver, vm, hostdev); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - - return ret; - + return 0; } static int -qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepShmem(virDomainObjPtr vm, virDomainShmemDefPtr match, - virDomainShmemDefPtr *detach, - bool async) + virDomainShmemDefPtr *detach) { - int ret = -1; - ssize_t idx = -1; + ssize_t idx; virDomainShmemDefPtr shmem; if ((idx = virDomainShmemDefFind(vm->def, match)) < 0) { @@ -5828,34 +5729,15 @@ qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &shmem->info); - - if (qemuDomainDeleteDevice(vm, shmem->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveShmemDevice(driver, vm, shmem); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepWatchdog(virDomainObjPtr vm, virDomainWatchdogDefPtr match, - virDomainWatchdogDefPtr *detach, - bool async) + virDomainWatchdogDefPtr *detach) { - int ret = -1; virDomainWatchdogDefPtr watchdog; @@ -5887,34 +5769,15 @@ qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); - - if (qemuDomainDeleteDevice(vm, watchdog->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepRedirdev(virDomainObjPtr vm, virDomainRedirdevDefPtr match, - virDomainRedirdevDefPtr *detach, - bool async) + virDomainRedirdevDefPtr *detach) { - int ret = -1; virDomainRedirdevDefPtr redirdev; ssize_t idx; @@ -5926,59 +5789,41 @@ qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, *detach = redirdev = vm->def->redirdevs[idx]; - if (!redirdev->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias not set for redirdev device")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &redirdev->info); - - if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepNet(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepNet(virDomainObjPtr vm, virDomainNetDefPtr match, - virDomainNetDefPtr *detach, - bool async) + virDomainNetDefPtr *detach) { - int detachidx, ret = -1; + int detachidx; virDomainNetDefPtr net; if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0) - goto cleanup; + return -1; *detach = net = vm->def->nets[detachidx]; - if (qemuIsMultiFunctionDevice(vm->def, &net->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - net->ifname); - goto cleanup; - } + return 0; +} - if (!net->info.alias) { - if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) - goto cleanup; - } +static void +qemuDomainDetachShutdownNet(virDomainNetDefPtr net) +{ +/* + * These operations are in a separate function from + * qemuDomainDetachPrepNet() because they can't be done until after + * we've validated that this device really can be removed - in + * particular we need to check for multifunction PCI devices and + * presence of a device alias, which isn't done until *after* the + * return from qemuDomainDetachPrepNet(). Since we've already passed + * the "point of no return", we ignore any errors, and trudge ahead + * with shutting down and detaching the device even if there is an + * error in one of these functions. + */ if (virDomainNetGetActualBandwidth(net) && virNetDevSupportBandwidth(virDomainNetGetActualType(net)) && virNetDevBandwidthClear(net->ifname) < 0) @@ -5990,35 +5835,8 @@ qemuDomainDetachPrepNet(virQEMUDriverPtr driver, * the parent device offline) */ ignore_value(qemuInterfaceStopDevice(net)); - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &net->info); - - if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) { - if (virDomainObjIsActive(vm)) { - /* the audit message has a different format for hostdev network devices */ - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) - virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net), "detach", false); - else - virDomainAuditNet(vm, net, NULL, "detach", false); - } - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, net); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; } - static int qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -6078,15 +5896,12 @@ qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, static int -qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepRNG(virDomainObjPtr vm, virDomainRNGDefPtr match, - virDomainRNGDefPtr *detach, - bool async) + virDomainRNGDefPtr *detach) { ssize_t idx; virDomainRNGDefPtr rng; - int ret = -1; if ((idx = virDomainRNGFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6098,42 +5913,17 @@ qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, *detach = rng = vm->def->rngs[idx]; - if (!rng->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias not set for RNG device")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &rng->info); - - if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRNGDevice(driver, vm, rng); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepMemory(virDomainObjPtr vm, virDomainMemoryDefPtr match, - virDomainMemoryDefPtr *detach, - bool async) + virDomainMemoryDefPtr *detach) { virDomainMemoryDefPtr mem; int idx; - int ret = -1; qemuDomainMemoryDeviceAlignSize(vm->def, match); @@ -6147,40 +5937,16 @@ qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, *detach = mem = vm->def->mems[idx]; - if (!mem->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias for the memory device was not found")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &mem->info); - - if (qemuDomainDeleteDevice(vm, mem->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int qemuDomainDetachPrepInput(virDomainObjPtr vm, virDomainInputDefPtr match, - virDomainInputDefPtr *detach, - bool async) + virDomainInputDefPtr *detach) { virDomainInputDefPtr input; - int ret = -1; int idx; if ((idx = virDomainInputDefFind(vm->def, match)) < 0) { @@ -6205,35 +5971,16 @@ qemuDomainDetachPrepInput(virDomainObjPtr vm, break; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &input->info); - - if (qemuDomainDeleteDevice(vm, input->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveInputDevice(vm, input); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int qemuDomainDetachPrepVsock(virDomainObjPtr vm, virDomainVsockDefPtr match, - virDomainVsockDefPtr *detach, - bool async) + virDomainVsockDefPtr *detach) { virDomainVsockDefPtr vsock; - int ret = -1; - *detach = vsock = vm->def->vsock; if (!vsock || @@ -6243,23 +5990,7 @@ qemuDomainDetachPrepVsock(virDomainObjPtr vm, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &vsock->info); - - if (qemuDomainDeleteDevice(vm, vsock->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveVsockDevice(vm, vsock); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } @@ -6294,6 +6025,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, bool async) { virDomainDeviceDef detach = { .type = match->type }; + virDomainDeviceInfoPtr info = NULL; int ret = -1; switch ((virDomainDeviceType)match->type) { @@ -6309,75 +6041,76 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return qemuDomainDetachDeviceChr(driver, vm, match->data.chr, async); /* - * All the other device types follow a very similar pattern - - * First we call type-specific functions to 1) locate the device - * we want to detach (based on the prototype device in match) - * and 2) do any device-type-specific validation to assure it - * is okay to detach the device. + * The rest of the device types (that support hot unplug) + * follow a very similar pattern - First we call type-specific + * functions to 1) locate the device we want to detach (based + * on the prototype device in match) and 2) do any + * device-type-specific validation to assure it is okay to + * detach the device. */ case VIR_DOMAIN_DEVICE_DISK: - if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk, - &detach.data.disk, async) < 0) { + if (qemuDomainDetachPrepDisk(vm, match->data.disk, + &detach.data.disk) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_CONTROLLER: - if (qemuDomainDetachPrepController(driver, vm, match->data.controller, - &detach.data.controller, async) < 0) { + if (qemuDomainDetachPrepController(vm, match->data.controller, + &detach.data.controller) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_NET: - if (qemuDomainDetachPrepNet(driver, vm, match->data.net, - &detach.data.net, async) < 0) { + if (qemuDomainDetachPrepNet(vm, match->data.net, + &detach.data.net) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_HOSTDEV: - if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev, - &detach.data.hostdev, async) < 0) { + if (qemuDomainDetachPrepHostdev(vm, match->data.hostdev, + &detach.data.hostdev) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_RNG: - if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng, - &detach.data.rng, async) < 0) { + if (qemuDomainDetachPrepRNG(vm, match->data.rng, + &detach.data.rng) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_MEMORY: - if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory, - &detach.data.memory, async) < 0) { + if (qemuDomainDetachPrepMemory(vm, match->data.memory, + &detach.data.memory) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_SHMEM: - if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem, - &detach.data.shmem, async) < 0) { + if (qemuDomainDetachPrepShmem(vm, match->data.shmem, + &detach.data.shmem) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_WATCHDOG: - if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog, - &detach.data.watchdog, async) < 0) { + if (qemuDomainDetachPrepWatchdog(vm, match->data.watchdog, + &detach.data.watchdog) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_INPUT: if (qemuDomainDetachPrepInput(vm, match->data.input, - &detach.data.input, async) < 0) { + &detach.data.input) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_REDIRDEV: - if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev, - &detach.data.redirdev, async) < 0) { + if (qemuDomainDetachPrepRedirdev(vm, match->data.redirdev, + &detach.data.redirdev) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_VSOCK: if (qemuDomainDetachPrepVsock(vm, match->data.vsock, - &detach.data.vsock, async) < 0) { + &detach.data.vsock) < 0) { return -1; } break; @@ -6401,7 +6134,75 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; } - ret = 0; + /* "detach" now points to the actual device we want to detach */ + + if (!(info = virDomainDeviceGetInfo(&detach))) { + /* + * This should never happen, since all of the device types in + * the switch cases that end with a "break" instead of a + * return have a virDeviceInfo in them. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("device of type '%s' has no device info"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + + /* Make generic validation checks common to all device types */ + + if (!info->alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot detach %s device with no alias"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + if (qemuIsMultiFunctionDevice(vm->def, info)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with multifunction PCI guest address: " + "%.4x:%.2x:%.2x.%.1x"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + + + /* + * Do any device-specific shutdown that should be + * done after all validation checks, but before issuing the qemu + * command to delete the device. For now, the only type of device + * that has such shutdown needs is the net device. + */ + if (detach.type == VIR_DOMAIN_DEVICE_NET) + qemuDomainDetachShutdownNet(detach.data.net); + + + /* + * Issue the qemu monitor command to delete the device (based on + * its alias), and optionally wait a short time in case the + * DEVICE_DELETED event arrives from qemu right away. + */ + if (!async) + qemuDomainMarkDeviceForRemoval(vm, info); + + if (qemuDomainDeleteDevice(vm, info->alias) < 0) { + if (virDomainObjIsActive(vm)) + qemuDomainRemoveAuditDevice(vm, &detach, false); + goto cleanup; + } + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDevice(driver, vm, &detach); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } -- 2.20.1

On 3/22/19 8:32 AM, Peter Krempa wrote: That is a good question! I'm not sure why network devices have this part of their shutdown handled early. I don't think we should make that change here though. I can insert an earlier patch to do it, hoping that it won't break anything, or I can make a later patch that moves it after this change, which seems kind of superfluous, but would make it simpler to revert in case it causes a regression. What's your opinion? I couldn't remember where was the proper place to put ATTRIBUTE_UNUSED in the function definition, and no matter where I put it it didn't work, so I gave up and used inline. Obviously that doesn't work for all compilers though, so I'll figure out the right place.

On 3/22/19 10:14 AM, Peter Krempa wrote: iirc, bandwidth rules are removed, and the interface is taken offline. Most like it's just that way because the original code had those happening early by coincidence, and then later when it was split into the separate detach and remove pieces, it got left in detach. Yeah, especially since I already made the patch that splits it out, etc. :-) Hmmph. I could *swear* that was the first one I tried, and I got an error. Maybe I wasn't paying close attention, and it was an error about some other detail that I coincidentally fixed at the same time I switched to using inline.

On Thu, Mar 21, 2019 at 18:29:01 -0400, Laine Stump wrote: So between the lines this reads as if qemuDomainRemoveHostDevice is broken. I agree though that fixing everything systematically is better. Well, but it's called prior to doing a return down below. The deduction from the sentence is that those that use 'return' don't necessarily have it and thus wouldn't ever be reached. I think you can change this to a best-effort scenario. Copy the alias only if info is non-null and emit the event in the same case. Also at this point 'info' should be set to NULL as any call to the Remove function frees the definition thus info would be dangling. The last sentence is not necessary. Or before if it's the same prologue code. Btw, this is probably a regression since we fixed locking of the 'driver' object. Until then the event would stay queued until the end of the API. ACK when the alias is copied best-effort rather than reporting error and info is set to NULL appropriately.