Re: [libvirt] [PATCH 2/2] libxl: support hotplug USB host device

>> On 5/13/2016 at 07:20 AM, in message <57350FA5.3070307(a)suse.com&gt;, Jim Fehlig

Chunyan Liu wrote: > Support hot attach/detach a USB host device to guest. > Curretnly libxl only supports xen PV guest, and only s/Curretnly/Currently/ > supports specifying USB host device by 'bus number' > and 'device number'. > > Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; > --- > src/libxl/libxl_driver.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 129 insertions(+), 1 deletion(-) > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 18a0891..8900644 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -3024,6 +3024,55 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr driver, > } > > static int > +libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver, > + virDomainObjPtr vm, > + virDomainHostdevDefPtr hostdev) > +{ > + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); > + libxl_device_usbdev usbdev; > + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; > + int ret = -1; > + > + libxl_device_usbdev_init(&usbdev); Move the init below the check for hostdev->mode and hostdev->source.subsys.type, otherwise it is not disposed if the check fails.

> + > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || > + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) > + return ret; > + > + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) > + goto cleanup; > + > + if (virHostdevPrepareUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, &hostdev, 1, 0) < 0) > + goto cleanup; > + > + if (libxlMakeUSB(hostdev, &usbdev) < 0) > + goto error; > + > + if (libxl_device_usbdev_add(cfg->ctx, vm->def->id, &usbdev, 0) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("libxenlight failed to attach usb device Busnum:%3x, Devnum:%3x"), > + hostdev->source.subsys.u.usb.bus, > + hostdev->source.subsys.u.usb.device); > + goto error; > + } > + > + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; > + ret = 0; > + goto cleanup; > + > + error: I think 'reattach' is a better name for this label.

> + virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, &hostdev, 1); > + > + cleanup: > + virObjectUnref(cfg); > + libxl_device_usbdev_dispose(&usbdev); > + return ret; > +} > + > + > +static int > libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev) > @@ -3041,6 +3090,11 @@ libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver, > return -1; > break; > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: > + if (libxlDomainAttachHostUSBDevice(driver, vm, hostdev) < 0) > + return -1; > + break; > + > default: > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("hostdev subsys type '%s' not supported"), > @@ -3266,7 +3320,8 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) > case VIR_DOMAIN_DEVICE_HOSTDEV: > hostdev = dev->data.hostdev; > > - if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > + if (hostdev->source.subsys.type != > + (VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB || VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)) if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB || hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) > return -1; > > if (virDomainHostdevFind(vmdef, hostdev, &found) >= 0) { > @@ -3385,6 +3440,76 @@ libxlDomainDetachHostPCIDevice(libxlDriverPrivatePtr driver, > } > > static int > +libxlDomainDetachHostUSBDevice(libxlDriverPrivatePtr driver, > + virDomainObjPtr vm, > + virDomainHostdevDefPtr hostdev) > +{ > + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); > + virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; > + virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; > + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; > + libxl_device_usbdev usbdev; > + libxl_device_usbdev *usbdevs = NULL; > + int num = 0; > + virDomainHostdevDefPtr detach; > + int idx; > + size_t i; > + bool found = false; > + int ret = -1; > + > + libxl_device_usbdev_init(&usbdev); > + > + idx = virDomainHostdevFind(vm->def, hostdev, &detach); > + if (idx < 0) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("host USB device Busnum: %3x, Devnum:%3x not found"), Space after Busnum, but not Devnum.

> + usbsrc->bus, usbsrc->device); > + goto cleanup; > + } > + > + usbdevs = libxl_device_usbdev_list(cfg->ctx, vm->def->id, &num); > + for (i = 0; i < num; i++) { > + if (usbdevs[i].u.hostdev.hostbus == usbsrc->bus && > + usbdevs[i].u.hostdev.hostaddr == usbsrc->device) { > + libxl_device_usbdev_copy(cfg->ctx, &usbdev, &usbdevs[i]); > + found = true; > + break; > + } > + } > + libxl_device_usbdev_list_free(usbdevs, num); > + > + if (!found) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("host USB device Busnum: %3x, Devnum:%3x not found"), Same here. > + usbsrc->bus, usbsrc->device); > + goto cleanup; > + } > + > + if (libxl_device_usbdev_remove(cfg->ctx, vm->def->id, &usbdev, 0) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("libxenlight failed to detach USB device\ > + Busnum: %3x, Devnum:%3x"), And here. > + usbsrc->bus, usbsrc->device); > + goto error; > + } > + > + virDomainHostdevRemove(vm->def, idx); > + > + virHostdevReAttachUSBDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > + vm->def->name, &hostdev, 1); > + > + ret = 0; > + > + error: > + virDomainHostdevDefFree(detach); The error label will be hit on success of the function. Should virDomainHostdevDefFree() be called in 'cleanup' and 'error' dropped?

Regards, Jim > + > + cleanup: > + virObjectUnref(cfg); > + libxl_device_usbdev_dispose(&usbdev); > + return ret; > +} > + > +static int > libxlDomainDetachHostDevice(libxlDriverPrivatePtr driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev) > @@ -3402,6 +3527,9 @@ libxlDomainDetachHostDevice(libxlDriverPrivatePtr driver, > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: > return libxlDomainDetachHostPCIDevice(driver, vm, hostdev); > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: > + return libxlDomainDetachHostUSBDevice(driver, vm, hostdev); > + > default: > virReportError(VIR_ERR_INTERNAL_ERROR, > _("unexpected hostdev type %d"), subsys->type);