Re: [libvirt] [PATCH 12/25] qemu: Add hotplug support for scsi host device

On 07/05/13 20:42, John Ferlan wrote: > On 05/03/2013 02:07 PM, Osier Yang wrote: >> From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; >> >> This adds both attachment and detachment support for scsi host >> device. >> >> Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; >> Signed-off-by: Osier Yang <jyang@redhat> >> --- >> src/qemu/qemu_hotplug.c | 141 >> ++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 136 insertions(+), 5 deletions(-) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 422d336..e6bc3a2 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -1194,6 +1194,74 @@ cleanup: >> return ret; >> } >> +static int >> +qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainHostdevDefPtr hostdev) >> +{ >> + int ret = -1; >> + qemuDomainObjPrivatePtr priv = vm->privateData; >> + char *devstr = NULL; >> + char *drvstr = NULL; >> + >> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || >> + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || >> + !virQEMUCapsGet(priv->qemuCaps, >> QEMU_CAPS_DEVICE_SCSI_GENERIC)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("SCSI passthrough is not supported by this >> version of qemu")); >> + return -1; >> + } >> + >> + if (qemuPrepareHostdevSCSIDevices(driver, vm->def->name, >> + &hostdev, 1)) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Unable to prepare scsi hostdev: >> %s:%d:%d:%d"), >> + hostdev->source.subsys.u.scsi.adapter, >> + hostdev->source.subsys.u.scsi.bus, >> + hostdev->source.subsys.u.scsi.target, >> + hostdev->source.subsys.u.scsi.unit); >> + return -1; >> + } >> + >> + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) >> + goto cleanup; >> + >> + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, >> priv->qemuCaps))) >> + goto cleanup; >> + >> + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, >> priv->qemuCaps))) >> + goto cleanup; >> + >> + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) { >> + virReportOOMError(); >> + goto cleanup; >> + } >> + >> + qemuDomainObjEnterMonitor(driver, vm); >> + if ((ret = qemuMonitorAddDrive(priv->mon, drvstr)) == 0) { >> + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) { >> + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", >> + drvstr, devstr); >> + qemuMonitorDriveDel(priv->mon, drvstr); >> + } >> + } >> + qemuDomainObjExitMonitor(driver, vm); >> + >> + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); > It may be better to more closely follow code flow of other modules as I > think you could be missing a nuance of a failure mode by trying to be > more generic. Just check all callers of AddDrive and AddDevice... > >> + if (ret < 0) >> + goto cleanup; >> + >> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; >> + >> + ret = 0; >> +cleanup: >> + if (ret < 0) >> + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, >> &hostdev, 1); >> + VIR_FREE(drvstr); >> + VIR_FREE(devstr); >> + return ret; >> +} >> + >> int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, >> virDomainObjPtr vm, >> virDomainHostdevDefPtr hostdev) >> @@ -1225,6 +1293,12 @@ int >> qemuDomainAttachHostDevice(virQEMUDriverPtr driver, >> goto error; >> break; >> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: >> + if (qemuDomainAttachHostScsiDevice(driver, vm, >> + hostdev) < 0) >> + goto error; >> + break; >> + >> default: >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("hostdev subsys type '%s' not supported"), >> @@ -2441,11 +2515,59 @@ >> qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, >> return ret; >> } >> -static >> -int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, >> - virDomainObjPtr vm, >> - virDomainHostdevDefPtr detach, >> - int idx) >> +static int >> +qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainHostdevDefPtr detach) >> +{ >> + qemuDomainObjPrivatePtr priv = vm->privateData; >> + char *drvstr = NULL; >> + char *devstr = NULL; >> + int ret = -1; >> + >> + if (!detach->info->alias) { >> + virReportError(VIR_ERR_OPERATION_FAILED, >> + "%s", _("device cannot be detached without a >> device alias")); >> + return -1; >> + } >> + >> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { >> + virReportError(VIR_ERR_OPERATION_FAILED, >> + "%s", _("device cannot be detached with this >> QEMU version")); >> + return -1; >> + } >> + >> + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps))) >> + goto cleanup; >> + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, >> priv->qemuCaps))) >> + goto cleanup; >> + >> + qemuDomainObjEnterMonitor(driver, vm); >> + if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) >> == 0) { >> + if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) { >> + VIR_WARN("qemuMonitorDriveDel failed on %s (%s)", >> + detach->info->alias, drvstr); >> + qemuMonitorAddDevice(priv->mon, devstr); > Coverity complains right here about no checking of return status: > > (9) Event check_return: Calling function > "qemuMonitorAddDevice(qemuMonitorPtr, char const *)" without checking > return value (as is done elsewhere 8 out of 9 times). > > > As with attach, the flow of this code is a bit different than other > places where DelDevice() and DriveDel() are called - I would think you > may want to follow those other models more closely... > > I think you meant keeping the original error? here is the diff: