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

[Osier Yang]

On 14/05/13 00:08, John Ferlan wrote:
> 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 at cn.fujitsu.com>
>>>>
>>>> This adds both attachment and detachment support for scsi host
>>>> device.
>>>>
>>>> Signed-off-by: Han Cheng <hanc.fnst at cn.fujitsu.com>
>>>> Signed-off-by: Osier Yang <jyang at 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
>
Thanks, pushed with the diff.