On Fri, 2016-09-02 at 15:24 +0200, Martin Kletzander wrote:
> On Wed, Aug 10, 2016 at 06:39:12PM -0600, Jim Fehlig wrote:
> >
> > From: Cédric Bosdonnat <cbosdonnat@xxxxxxxx>
> >
> > libxl only has API to address the host USB devices by bus/device.
> > Find the bus/device if the user only provided the vendor/product
> > of the USB device.
> >
> > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> > ---
> >
> > V2:
> > - Initialize local 'usb' variable
> > - Use 'bus' and 'device' from virUSBDevice retrieved from
> > virHostdevFindUSBDevice instead of relying on the function's
> > side-affects
> >
> > src/libxl/libxl_conf.c | 33 +++++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index 5202ca1..06cbc2c 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -1559,23 +1559,36 @@ int
> > libxlMakeUSB(virDomainHostdevDefPtr hostdev, libxl_device_usbdev *usbdev)
> > {
> > virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
> > + virUSBDevicePtr usb = NULL;
> > + int ret = -1;
> >
> > if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> > - return -1;
> > + goto cleanup;
>
> There's no need to have 'ret' and cleanup section, you can still return
> -1 here, since...
>
> >
> > if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> > - return -1;
> > + goto cleanup;
> >
> > - if (usbsrc->bus <= 0 || usbsrc->device <= 0) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > - _("libxenlight supports only USB device "
> > - "specified by busnum:devnum"));
> > - return -1;
> > + if (usbsrc->bus > 0 && usbsrc->device > 0) {
> > + usbdev->u.hostdev.hostbus = usbsrc->bus;
> > + usbdev->u.hostdev.hostaddr = usbsrc->device;
> > + } else {
> > + if (virHostdevFindUSBDevice(hostdev, true, &usb) < 0) {
> > + virReportError(VIR_ERR_OPERATION_FAILED,
> > + _("failed to find USB device busnum:devnum "
> > + "for %x:%x"),
> > + usbsrc->vendor, usbsrc->product);
>
> ... this and ...
>
> >
> > + goto cleanup;
> > + }
> > +
> > + usbdev->u.hostdev.hostbus = virUSBDeviceGetBus(usb);
> > + usbdev->u.hostdev.hostaddr = virUSBDeviceGetDevno(usb);
>
> ... this is the only place you need to clean stuff up. But whatever
> floats your boat, this is just a suggestion ;)
>
> ACK, preferably after release, unless this fixes something for you guys.
>
Thanks for the review, I'll push that with your suggestion.
--
Cedric
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list