Re: [PATCH v2] qemu_hotplug: Do not report unknown error when hot-unplugging non-existing device

On a Monday in 2024, Martin Kletzander wrote: >When qemuDomainDeleteDevice() gets "DeviceNotFound" error it is a >special case as we're trying to remove a device which does not exists >any more. Such occasion is indicated by the return value -2. > >Callers of the aforementioned function ought to base their behaviour on >the return value. However not all callers take as much care for the >return value as one could realistically anticipate. > >Follow the usual direction of removing possible backend object (in case >of character devices), remove the device from its XML without waiting >for the device removal from QEMU (since it is already not there) and >basically follow the same algorithm as there is when the device was >removed, skipping over the wait for the device removal. > >The overall return value also needs to be adjusted since >qemuDomainDeleteDevice() does not set an error on the -2 return value >and would otherwise trigger an unknown error being reported to the user >or management application. > >Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; >--- > src/qemu/qemu_hotplug.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > >diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >index 8361d3d9c1b7..1b6ecb1cd1f9 100644 >--- a/src/qemu/qemu_hotplug.c >+++ b/src/qemu/qemu_hotplug.c >@@ -6090,8 +6090,12 @@ qemuDomainDetachDeviceChr(virQEMUDriver *driver, > if (rc < 0) > goto cleanup; > } else { >- if (qemuDomainDeleteDevice(vm, tmpChr->info.alias) < 0) >+ ret = qemuDomainDeleteDevice(vm, tmpChr->info.alias); >+ if (ret < 0) { >+ if (ret == -2) >+ ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, true); > goto cleanup; >+ } > } > > if (guestfwd) { >@@ -6595,18 +6599,19 @@ qemuDomainHotplugDelVcpu(virQEMUDriver *driver, > > qemuDomainMarkDeviceAliasForRemoval(vm, vcpupriv->alias); > >- if (qemuDomainDeleteDevice(vm, vcpupriv->alias) < 0) { >- if (virDomainObjIsActive(vm)) >+ rc = qemuDomainDeleteDevice(vm, vcpupriv->alias); >+ if (rc < 0) { >+ if (rc == -1 && virDomainObjIsActive(vm)) > virDomainAuditVcpu(vm, oldvcpus, oldvcpus - nvcpus, "update", false); > goto cleanup; >- } >- >- if ((rc = qemuDomainWaitForDeviceRemoval(vm)) <= 0) { >- if (rc == 0) >- virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", >- _("vcpu unplug request timed out. Unplug result must be manually inspected in the domain")); >+ } else { The additional nesting because of the 'else' branch is not necessary - if the condition above was true, we already jumped to cleanup.

>+ if ((rc = qemuDomainWaitForDeviceRemoval(vm)) <= 0) { >+ if (rc == 0) >+ virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", >+ _("vcpu unplug request timed out. Unplug result must be manually inspected in the domain")); > Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt; Jano