Re: [libvirt] [PATCH v2 09/10] qemu: hotplug support for scsi hostdev

On 01/04/13 20:01, Han Cheng wrote:

This patch add hotplug for scsi hostdev.

s/add/adds/

And user should hotplug a virtio-scsi controller if doesn't exist.

I'm wondering if it could be implicitly added.

Usb hostdev related codes are in qemuDomainAttachHostDevice, push down to qemuDomainAttachHostUsbDevice.

Signed-off-by: Han Cheng <hanc.fnst@cn.fujitsu.com> --- src/qemu/qemu_hotplug.c | 211 ++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 173 insertions(+), 38 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b978b97..a78b410 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1116,6 +1116,98 @@ error:

}

+int qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret; + 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_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, "%s", + _("Unable to prepare scsi hostdev"));

Giving a prompt for what the device is should be better.

+ return -1; + } + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto error; + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto error; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto error; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs+1) < 0) {

vm->def->nhostdevs + 1

+ virReportOOMError(); + goto error; + }

vm->def->hostdevs[def->nhostdevs] is leaked if it errors out later.

+ + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { + virCgroupPtr cgroup = NULL; + virSCSIDevicePtr scsi; + qemuCgroupData data; + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find cgroup for %s"), + vm->def->name); + goto error; + } + + if ((scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly)) == NULL) + goto error; + + data.vm = vm; + data.cgroup = cgroup; + if (virSCSIDeviceFileIterate(scsi, qemuSetupHostScsiDeviceCgroup, + &data) < 0) { + virSCSIDeviceFree(scsi); + goto error; + } + virSCSIDeviceFree(scsi); + } + + 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);

Indention.

+ qemuMonitorDriveDel(priv->mon, drvstr); + } + } + qemuDomainObjExitMonitor(driver, vm); + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); + if (ret < 0) + goto error; +

You can reallocate the vm->def->hostdevs right here to avoid the leak.

+ vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + + VIR_FREE(drvstr); + VIR_FREE(devstr);

As I commented in 5/10, these frees can be destroyed.

+ + return 0; + +error: + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); + VIR_FREE(drvstr); + VIR_FREE(devstr); + return -1; +} + int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -1123,6 +1215,26 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, int ret; qemuDomainObjPrivatePtr priv = vm->privateData; char *devstr = NULL; + virUSBDeviceList *list; + virUSBDevicePtr usb = NULL; + + + if (!(list = virUSBDeviceListNew())) + goto error; + + if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) + goto error; + + if (virUSBDeviceListAdd(list, usb) < 0) { + virUSBDeviceFree(usb); + usb = NULL; + goto error; + } + + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { + usb = NULL; + goto error; + }

if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) @@ -1138,7 +1250,6 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,

if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { virCgroupPtr cgroup = NULL; - virUSBDevicePtr usb; qemuCgroupData data;

if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -1148,19 +1259,12 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver, goto error; }

- if ((usb = virUSBDeviceNew(hostdev->source.subsys.u.usb.bus, - hostdev->source.subsys.u.usb.device, - NULL)) == NULL) - goto error; - data.vm = vm; data.cgroup = cgroup; if (virUSBDeviceFileIterate(usb, qemuSetupHostUsbDeviceCgroup, &data) < 0) { - virUSBDeviceFree(usb); goto error; } - virUSBDeviceFree(usb); }

qemuDomainObjEnterMonitor(driver, vm); @@ -1177,11 +1281,16 @@ int qemuDomainAttachHostUsbDevice(virQEMUDriverPtr driver,

vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;

+ virUSBDeviceListSteal(list, usb); + virObjectUnref(list); VIR_FREE(devstr);

return 0;

error: + if (usb) + virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); + virObjectUnref(list); VIR_FREE(devstr); return -1; } @@ -1190,9 +1299,6 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { - virUSBDeviceList *list; - virUSBDevicePtr usb = NULL; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev mode '%s' not supported"), @@ -1200,30 +1306,10 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, return -1; }

- if (!(list = virUSBDeviceListNew())) - goto cleanup; - - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { - if (qemuFindHostdevUSBDevice(hostdev, true, &usb) < 0) - goto cleanup; - - if (virUSBDeviceListAdd(list, usb) < 0) { - virUSBDeviceFree(usb); - usb = NULL; - goto cleanup; - } - - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0) { - usb = NULL; - goto cleanup; - } - - virUSBDeviceListSteal(list, usb); - }

if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) - goto cleanup; + return -1;

switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1238,6 +1324,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"), @@ -1245,18 +1337,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; }

- virObjectUnref(list); return 0;

error: if (virSecurityManagerRestoreHostdevLabel(driver->securityManager, vm->def, hostdev, NULL) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); - -cleanup: - virObjectUnref(list); - if (usb) - virUSBDeviceListSteal(driver->activeUsbHostdevs, usb); return -1; }

@@ -2439,6 +2525,46 @@ cleanup: }

static int +qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *drvstr = NULL; + char *devstr = NULL; + int ret; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps))) + goto error; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps))) + goto error; + + 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); + } + } + qemuDomainObjExitMonitor(driver, vm); + virDomainAuditHostdev(vm, detach, "detach", ret == 0); + + if (ret == 0) + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &detach, 1); + +error: + VIR_FREE(drvstr); + VIR_FREE(devstr); + return ret; +} + +static int qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr detach) @@ -2511,6 +2637,9 @@ int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: ret = qemuDomainDetachHostUsbDevice(driver, vm, detach); break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + ret = qemuDomainDetachHostScsiDevice(driver, vm, detach); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -2567,6 +2696,12 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, subsys->u.usb.vendor, subsys->u.usb.product); } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virReportError(VIR_ERR_OPERATION_FAILED, + _("host pci device %s:%d:%d.%d not found"),

s/pci/scsi/, others look good.