5:15 p.m.
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(a)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(a)redhat.com>
Thanks!