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

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(a)redhat.com&gt; > --- > 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.

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