[libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'

Laine Stump posted 1 patch 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200514181238.2153800-1-laine@redhat.com
src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

[libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'

Posted by Laine Stump 1 week ago
Although the original patches to support controllers with
hotplug='off' were checking during hotplug/attach requests that the
device was being plugged into a PCI controller that didn't have
hotplug disabled, but I forgot to do the same for device detach (the
main impetus for adding the feature was to prevent unplugs originating
from within the guest, so it slipped my mind). So although the guest
OS was ultimately unable to honor the unplug request, libvirt could
still be used to make such a request, and since device attach/detach
are asynchronous operations, the caller to libvirt would receive a
success status back (the device would stubbornly/correctly remain in
the domain status XML however)

This patch remedies that, by looking at the controller for the device
in the detach request, and immediately failing the operation if that
controller has hotplug=off.

r changes. Lines starting

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ab5a7aef84..5fe125b1a7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
         return -1;
     }
 
+    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+        int controllerIdx = virDomainControllerFind(vm->def,
+                                                    VIR_DOMAIN_CONTROLLER_TYPE_PCI,
+                                                    info->addr.pci.bus);
+        if (controllerIdx < 0) {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("cannot hot unplug %s device with PCI guest address: "
+                             VIR_PCI_DEVICE_ADDRESS_FMT
+                             " - controller not found"),
+                           virDomainDeviceTypeToString(detach.type),
+                           info->addr.pci.domain, info->addr.pci.bus,
+                           info->addr.pci.slot, info->addr.pci.function);
+            return -1;
+        }
+
+        if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug
+            == VIR_TRISTATE_SWITCH_OFF) {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("cannot hot unplug %s device with PCI guest address: "
+                             VIR_PCI_DEVICE_ADDRESS_FMT
+                             " - not allowed by controller"),
+                           virDomainDeviceTypeToString(detach.type),
+                           info->addr.pci.domain, info->addr.pci.bus,
+                           info->addr.pci.slot, info->addr.pci.function);
+            return -1;
+        }
+    }
     /*
      * Issue the qemu monitor command to delete the device (based on
      * its alias), and optionally wait a short time in case the
-- 
2.25.4

Re: [libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'

Posted by Erik Skultety 1 week ago
On Thu, May 14, 2020 at 02:12:38PM -0400, Laine Stump wrote:
> Although the original patches to support controllers with
> hotplug='off' were checking during hotplug/attach requests that the
> device was being plugged into a PCI controller that didn't have
> hotplug disabled, but I forgot to do the same for device detach (the
> main impetus for adding the feature was to prevent unplugs originating
> from within the guest, so it slipped my mind). So although the guest
> OS was ultimately unable to honor the unplug request, libvirt could
> still be used to make such a request, and since device attach/detach
> are asynchronous operations, the caller to libvirt would receive a
> success status back (the device would stubbornly/correctly remain in
> the domain status XML however)
> 
> This patch remedies that, by looking at the controller for the device
> in the detach request, and immediately failing the operation if that
> controller has hotplug=off.
> 
> r changes. Lines starting

^This looks like some accidental copy-paste :)

> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index ab5a7aef84..5fe125b1a7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>          return -1;
>      }
>  
> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
> +        int controllerIdx = virDomainControllerFind(vm->def,
> +                                                    VIR_DOMAIN_CONTROLLER_TYPE_PCI,
> +                                                    info->addr.pci.bus);
> +        if (controllerIdx < 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("cannot hot unplug %s device with PCI guest address: "
> +                             VIR_PCI_DEVICE_ADDRESS_FMT
> +                             " - controller not found"),
> +                           virDomainDeviceTypeToString(detach.type),
> +                           info->addr.pci.domain, info->addr.pci.bus,
> +                           info->addr.pci.slot, info->addr.pci.function);
> +            return -1;
> +        }
> +

To enhance readability, use a temporary variable:

virDomainControllerDefPtr controller = vm->def->controllers[controllerIdx];
if (controller->opts.pciopts.hotplug == VIR_TRISTATE_SWITCH_OFF) {...}

> +        if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug
> +            == VIR_TRISTATE_SWITCH_OFF) {
> +            virReportError(VIR_ERR_OPERATION_FAILED,

So, VIR_ERR_OPERATION_FAILED makes perfect sense here, but it feels quite
inconsistent with what error we return in the opposite scenario when the flags
are compared in virDomainPCIAddressFlagsCompatible, where we'd report internal
error on hotplug. Just my humble opinion, I agree with ^this proposal.

> +                           _("cannot hot unplug %s device with PCI guest address: "
> +                             VIR_PCI_DEVICE_ADDRESS_FMT
> +                             " - not allowed by controller"),
> +                           virDomainDeviceTypeToString(detach.type),
> +                           info->addr.pci.domain, info->addr.pci.bus,
> +                           info->addr.pci.slot, info->addr.pci.function);
> +            return -1;
> +        }
> +    }

Add an extra line please.

>      /*
>       * Issue the qemu monitor command to delete the device (based on
>       * its alias), and optionally wait a short time in case the

Reviewed-by: Erik Skultety <eskultet@redhat.com>

Re: [libvirt PATCH] qemu: prevent attempts to detach a device on a controller with hotplug='off'

Posted by Laine Stump 1 week ago
On 5/15/20 3:58 AM, Erik Skultety wrote:
> On Thu, May 14, 2020 at 02:12:38PM -0400, Laine Stump wrote:
>> Although the original patches to support controllers with
>> hotplug='off' were checking during hotplug/attach requests that the
>> device was being plugged into a PCI controller that didn't have
>> hotplug disabled, but I forgot to do the same for device detach (the
>> main impetus for adding the feature was to prevent unplugs originating
>> from within the guest, so it slipped my mind). So although the guest
>> OS was ultimately unable to honor the unplug request, libvirt could
>> still be used to make such a request, and since device attach/detach
>> are asynchronous operations, the caller to libvirt would receive a
>> success status back (the device would stubbornly/correctly remain in
>> the domain status XML however)
>>
>> This patch remedies that, by looking at the controller for the device
>> in the detach request, and immediately failing the operation if that
>> controller has hotplug=off.
>>
>> r changes. Lines starting
> ^This looks like some accidental copy-paste :)
>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>>   src/qemu/qemu_hotplug.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index ab5a7aef84..5fe125b1a7 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -5891,6 +5891,33 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>           return -1;
>>       }
>>   
>> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> +        int controllerIdx = virDomainControllerFind(vm->def,
>> +                                                    VIR_DOMAIN_CONTROLLER_TYPE_PCI,
>> +                                                    info->addr.pci.bus);
>> +        if (controllerIdx < 0) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED,
>> +                           _("cannot hot unplug %s device with PCI guest address: "
>> +                             VIR_PCI_DEVICE_ADDRESS_FMT
>> +                             " - controller not found"),
>> +                           virDomainDeviceTypeToString(detach.type),
>> +                           info->addr.pci.domain, info->addr.pci.bus,
>> +                           info->addr.pci.slot, info->addr.pci.function);
>> +            return -1;
>> +        }
>> +
> To enhance readability, use a temporary variable:
>
> virDomainControllerDefPtr controller = vm->def->controllers[controllerIdx];
> if (controller->opts.pciopts.hotplug == VIR_TRISTATE_SWITCH_OFF) {...}


I suppose it does put the two sides of the == on a single line. Other 
than that it doesn't do much, just takes up an extra line because the 
definition has to be on a separate line from its assignment (you can 
only use controllerIdx if it's >= 0, so it could be invalid up in the 
block where automatics need to be defined), and it's only used once (if 
it was used more than once I would have already done it).


Anyway, since you asked, I did it now before pushing.


(NB: I also would have made a temporary variable to point directly at 
info->addr.pci, but there was existing usage of it as is, and I would 
have felt compelled to change that usage to use a temporary as well, but 
didn't want to restructure that much. I'll send a followup patch for that)


>
>> +        if (vm->def->controllers[controllerIdx]->opts.pciopts.hotplug
>> +            == VIR_TRISTATE_SWITCH_OFF) {
>> +            virReportError(VIR_ERR_OPERATION_FAILED,
> So, VIR_ERR_OPERATION_FAILED makes perfect sense here, but it feels quite
> inconsistent with what error we return in the opposite scenario when the flags
> are compared in virDomainPCIAddressFlagsCompatible, where we'd report internal
> error on hotplug.


That depends on whether the address was generated automatically by 
libvirt, or provided by the user. virDomainPCIAddressFlagsCompatible 
will give an INTERNAL_ERROR if libvirt generated the address (shouldn't 
ever happen - libvirt should instead say that it can't find an 
appropriate slot at an earlier time). If the user provides an address, 
then it gives XML_ERROR. I guess at the time that code was written, we 
were thinking more about the fact that the address came from XML written 
by the user rather than what was being done with that XML.


Anyway, in the attach case, the user is saying "put this device *here*, 
and libvirt is saying "*here* isn't a proper place to put this device". 
In the detach case, the user is saying "unplug this device" and libvirt 
is saying "you can't unplug this device", which is a bit of a different 
thing from "this device can't be put here".

But that's just nitpicking. The more important thing is that I'm just 
following whatever error types were already there for similar things 
(and in a lot of cases the decision of which error type to use is 
inaccurate and subjective, because the same function may be called from 
different places that would suggest different error types - point in 
case is that virDomainPCIAddressFlagsCompatible tries to use a different 
error type depending on the context; if we try to go too far down that 
road the code gets ugly and hairy for no real gain - in the end it's 
only important to inform what is broken, and the user can decide 
themselves how exactly to classify that breakage)




>   Just my humble opinion, I agree with ^this proposal.
>
>> +                           _("cannot hot unplug %s device with PCI guest address: "
>> +                             VIR_PCI_DEVICE_ADDRESS_FMT
>> +                             " - not allowed by controller"),
>> +                           virDomainDeviceTypeToString(detach.type),
>> +                           info->addr.pci.domain, info->addr.pci.bus,
>> +                           info->addr.pci.slot, info->addr.pci.function);
>> +            return -1;
>> +        }
>> +    }
> Add an extra line please.
>
>>       /*
>>        * Issue the qemu monitor command to delete the device (based on
>>        * its alias), and optionally wait a short time in case the
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
>

Thanks!