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@xxxxxxxxxx> >>> --- >>> 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; >>> } >>> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list