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

[John Ferlan]

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?

> (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).

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;
>>  }
>>  
>