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

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