1. Patchew
  2. Libvirt
  3. View series

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

Martin Kletzander posted 1 patch 1 week, 2 days ago
There is a newer version of this series
src/qemu/qemu_hotplug.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
[PATCH v2] qemu_hotplug: Do not report unknown error when hot-unplugging non-existing device
Posted by Martin Kletzander 1 week, 2 days ago 
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@redhat.com>
---
 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 {
+        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"));
 
-        goto cleanup;
+            goto cleanup;
+        }
     }
 
     if (qemuDomainRemoveVcpu(vm, vcpu) < 0)
-- 
2.47.0
Re: [PATCH v2] qemu_hotplug: Do not report unknown error when hot-unplugging non-existing device
Posted by Ján Tomko 1 week, 1 day ago 
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@redhat.com>
>---
> 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@redhat.com>

Jano
Re: [PATCH v2] qemu_hotplug: Do not report unknown error when hot-unplugging non-existing device
Posted by Martin Kletzander 1 week, 1 day ago 
On Tue, Nov 12, 2024 at 02:10:31PM +0100, Ján Tomko wrote:
>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@redhat.com>
>>---
>> 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.
>

No, sorry, we shouldn't have jumped to cleanup for -2, only skip the
qemuDomainWaitForDeviceRemoval() call.  It's missing one pair of
squiggly brackets and maybe one more change.  I'll send another version.
Thanks for noticing that.

>>+        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@redhat.com>
>
>Jano

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.