[libvirt] [PATCH 01/21] qemu_hotplug: remove erroneous call to qemuDomainDetachExtensionDevice()

[Boris Fiuczynski]

I agree. This must have slipped in during the countless iterations.
Thanks for catching it.
Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>


On 3/21/19 11:28 PM, Laine Stump wrote:
> qemuDomainDetachControllerDevice() calls
> qemuDomainDetachExtensionDevice() when the *controller* type is
> PCI. This is incorrect in multiple ways:
> 
> * Any code that tears down a device should be in the
>    qemuDomainRemove*Device() function (which is called after libvirt
>    gets a DEVICE_DELETED event from qemu indicating that the guest is
>    finished with the device on its end. The qemuDomainDetach*Device()
>    functions should only contain code that ensures the requested
>    operation is valid, and sends the command to qemu to initiate the
>    unplug.
> 
> * qemuDomainDetachExtensionDevice() is a function that applies to
>    devices that plug into a PCI slot, *not* necessarily PCI controllers
>    (which is what's being checked in the offending code). The proper
>    way to check for this would be to see if the DeviceInfo for the
>    controller device had a PCI address, not to check if the controller
>    is a PCI controller (the code being removed was doing the latter).
> 
> * According to commit 1d1e264f1 that added this code (and other
>    support for hotplugging zPCI devices on s390), it's not necessary to
>    explicitly detach the zPCI device when unplugging a PCI device. To
>    quote:
> 
>         There's no need to implement hot unplug for zPCI as QEMU
>         implements an unplug callback which will unplug both PCI and
>         zPCI device in a cascaded way.
> 
>    and the evidence bears this out - all the other uses of
>    qemuDomainDetachExtensionDevice() (except one, which I believe is
>    also in error, and is being removed in a separate patch) are only to
>    remove the zPCI extension device in cases where it was successfully
>    added, but there was some other failure later in the hotplug process
>    (so there was no regular PCI device to remove and trigger removal of
>    the zPCI extension device).
> 
> * PCI controllers are not hot pluggable, so this is dead code
>    anyway. (The only controllers that can currently be
>    hotplugged/unplugged are SCSI controllers).
> 
> Signed-off-by: Laine Stump <laine at laine.org>
> ---
>   src/qemu/qemu_hotplug.c | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 41d60277d1..18075dc48e 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5457,7 +5457,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>   {
>       int idx, ret = -1;
>       virDomainControllerDefPtr detach = NULL;
> -    qemuDomainObjPrivatePtr priv = vm->privateData;
>   
>       if ((idx = virDomainControllerFind(vm->def,
>                                          dev->data.controller->type,
> @@ -5503,17 +5502,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>       if (!async)
>           qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>   
> -    if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) {
> -        int rc;
> -        qemuDomainObjEnterMonitor(driver, vm);
> -        rc = qemuDomainDetachExtensionDevice(priv->mon, &detach->info);
> -        if (qemuDomainObjExitMonitor(driver, vm) < 0)
> -            rc = -1;
> -
> -        if (rc < 0)
> -            goto cleanup;
> -    }
> -
>       if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0)
>           goto cleanup;
>   
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294