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

[Laine Stump]

On 10/06/2014 09:36 AM, John Ferlan wrote:
>
> On 10/03/2014 12:06 PM, Laine Stump wrote:
>> 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 @@ (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.
>>
> Oh yeah - right - this needs a "if (hostdev->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)" around it...
>
>
> $ git diff -w
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 55f4bb3..0bd4e8a 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2630,6 +2630,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>      VIR_DEBUG("Removing host device %s from domain %p %s",
>                hostdev->info->alias, vm, vm->def->name);
>
> +    if (hostdev->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) {
>          /* build the actual drive id string as generated during
>           * qemuBuildSCSIHostdevDrvStr that is passed to qemu */
>          if (virAsprintf(&drivestr, "%s-%s",
> @@ -2640,6 +2641,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>          qemuDomainObjEnterMonitor(driver, vm);
>          qemuMonitorDriveDel(priv->mon, drivestr);
>          qemuDomainObjExitMonitor(driver, vm);
> +    }
>
>      event = virDomainEventDeviceRemovedNewFromObj(vm,
> hostdev->info->alias);
>      if (event)
>
> OK?

OK. ACK. It's too bad that it can't just be a part of the switch
statement further down in the same function, but I think this needs to
be done prior to queuing the event.

>
>> (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?)
>>
> To make them available to the host again?
>
> If I follow the history - I see commit '53f3739a' makes the standalone
> function 'qemuDomainRemoveSCSIHostDevice' from what used to be an inline
> call to ReAttach added by commit id '8f76ad99'. The original ReAttach
> was added via commit 'ea74c076' (searching for Scsi instead of SCSI).

Yeah, never mind. It had been just a tad too long since I looked at this
code, and I forgot the confusing nomenclature of "ReAttach", which is
talking about reattaching the device to a host-side driver, *not*
reattaching it to the guest. (I suffered the same confusion the first
time I dug into this code, you'd think that I would remember it from
that...)

> It seems from the commit id comment, the code was added to be just like
> PCI and USB.
>
> John
>>> +
>>>      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;
>>>  }
>>>