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

[Erik Skultety]

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 at 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 at redhat.com>