[libvirt] [PATCH 1/3] qemu: Fix hot unplug of SCSI_HOST device

[Laine Stump]

On 09/24/2014 09:11 AM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1141732
>
> Introduced by commit id '8f76ad99' the logic to detach a scsi_host
> device (SCSI or iSCSI) fails when attempting to remove the 'drive'
> because as I found in my investigation - the DelDevice takes care of
> that for us.
>
> The investigation turned up commits to adjust the logic for the
> qemuMonitorDelDevice and qemuMonitorDriveDel processing for interfaces
> (commit id '81f76598'), disk bus=VIRTIO,SCSI,USB (commit id '0635785b'),
> and chr devices (commit id '55b21f9b'), but nothing with the host devices.
>
> This commit uses the model for the previous set of changes and applies
> it to the hostdev path. The call to qemuDomainDetachHostSCSIDevice will
> return to qemuDomainDetachThisHostDevice handling either the audit of
> the failure or the wait for the removal and then call into
> qemuDomainRemoveHostDevice for the event, removal from the domain hostdev
> list, and audit of the removal similar to other paths.
>
> NOTE: For now the 'conn' param to +qemuDomainDetachHostSCSIDevice is left
> as ATTRIBUTE_UNUSED.  Removing requires a cascade of other changes to be
> left for a future patch.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7bc19cd..cf1e4dc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2623,10 +2623,24 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>      virDomainNetDefPtr net = NULL;
>      virObjectEventPtr event;
>      size_t i;
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    char *drivestr;
>  
>      VIR_DEBUG("Removing host device %s from domain %p %s",
>                hostdev->info->alias, vm, vm->def->name);
>  
> +    /* build the actual drive id string as generated during
> +     * qemuBuildSCSIHostdevDrvStr that is passed to qemu */
> +    if (virAsprintf(&drivestr, "%s-%s",
> +                    virDomainDeviceAddressTypeToString(hostdev->info->type),
> +                    hostdev->info->alias) < 0)
> +        goto cleanup;
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    qemuMonitorDriveDel(priv->mon, drivestr);
> +    qemuDomainObjExitMonitor(driver, vm);

I think this function is used  by devices other than just SCSI host
devices, which I *think* means you are breaking proper detach of those
other kinds of devices.

(beyond that, but unrelated to your changes - I don't understand why
qemuDomainRemoveSCSIHostDevice() (which is already called by
qemuDomainRemoveHostDevice in the switch at the bottom) calls
qemuDomain*ReAttach*HostSCSIDevices(). What's up with that?)

> +
>      event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias);
>      if (event)
>          qemuDomainEventQueue(driver, event);
> @@ -2679,8 +2693,12 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>          networkReleaseActualDevice(vm->def, net);
>          virDomainNetDefFree(net);
>      }
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(drivestr);
>      virObjectUnref(cfg);
> -    return 0;
> +    return ret;
>  }
>  
>  
> @@ -3305,14 +3323,12 @@ qemuDomainDetachHostUSBDevice(virQEMUDriverPtr driver,
>  }
>  
>  static int
> -qemuDomainDetachHostSCSIDevice(virConnectPtr conn,
> +qemuDomainDetachHostSCSIDevice(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                 virQEMUDriverPtr driver,
>                                 virDomainObjPtr vm,
>                                 virDomainHostdevDefPtr detach)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> -    char *drvstr = NULL;
> -    char *devstr = NULL;
>      int ret = -1;
>  
>      if (!detach->info->alias) {
> @@ -3327,33 +3343,17 @@ qemuDomainDetachHostSCSIDevice(virConnectPtr conn,
>          return -1;
>      }
>  
> -    if (!(drvstr = qemuBuildSCSIHostdevDrvStr(conn, detach, priv->qemuCaps,
> -                                              &buildCommandLineCallbacks)))
> -        goto cleanup;
> -    if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps)))
> -        goto cleanup;
> -
>      qemuDomainMarkDeviceForRemoval(vm, detach->info);
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> -    if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) {
> -        if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) {
> -            virErrorPtr orig_err = virSaveLastError();
> -            if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> -                VIR_WARN("Unable to add device %s (%s) after failed "
> -                         "qemuMonitorDriveDel",
> -                         drvstr, devstr);
> -            if (orig_err) {
> -                virSetError(orig_err);
> -                virFreeError(orig_err);
> -            }
> -        }
> +    if (qemuMonitorDelDevice(priv->mon, detach->info->alias) < 0) {
> +        qemuDomainObjExitMonitor(driver, vm);
> +        goto cleanup;
>      }
>      qemuDomainObjExitMonitor(driver, vm);
> +    ret = 0;
>  
>   cleanup:
> -    VIR_FREE(drvstr);
> -    VIR_FREE(devstr);
>      return ret;
>  }
>