On 05/13/2013 11:48 AM, Osier Yang wrote:
> On 07/05/13 20:42, John Ferlan wrote:
>> On 05/03/2013 02:07 PM, Osier Yang wrote:
>>> From: Han Cheng <hanc.fnst@xxxxxxxxxxxxxx>
>>>
>>> This adds both attachment and detachment support for scsi host
>>> device.
>>>
>>> Signed-off-by: Han Cheng <hanc.fnst@xxxxxxxxxxxxxx>
>>> 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:
>
Right, seems better now and the visual inspection regarding the coverity
complaint is cleaned up...
ACK
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list