Re: [Xen-devel] [PATCH V7 3/7] libxl: add pvusb API

[Chun Yan Liu <cyliu@xxxxxxxxxx>]

On 10/12/2015 09:46 PM, George Dunlap wrote:
> On 12/10/15 08:19, Chun Yan Liu wrote:
> > > > +
> > > > +    usbinfo->devnum = usb->u.hostdev.hostaddr;
> > > > +    usbinfo->busnum = usb->u.hostdev.hostbus;
> > > > +
> > > > +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> > > > +                                 usb->u.hostdev.hostaddr);
> > > > +    if (!busid) {
> > > > +        rc = ERROR_FAIL;
> > > > +        goto out;
> > > > +    }
> > > > +
> > > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
> > > > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> > > > +        sscanf(buf, "%" SCNx16, &usbinfo->idVendor);
> > > > +
> > > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
> > > > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> > > > +        sscanf(buf, "%" SCNx16, &usbinfo->idProduct);
> > > > +
> > > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
> > > > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
> > > > +        buflen > 0) {
> > > > +        /* replace \n to \0 */
> > > > +        if (((char *)buf)[buflen - 1] == '\n')
> > > > +            ((char *)buf)[buflen - 1] = '\0';
> > > > +        usbinfo->manuf = libxl__strdup(NOGC, buf);
> > > > +   }
> > > > +
> > > > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
> > > > +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
> > > > +        buflen > 0) {
> > > > +        /* replace \n to \0 */
> > > > +        if (((char *)buf)[buflen - 1] == '\n')
> > > > +            ((char *)buf)[buflen - 1] = '\0';
> > > > +        usbinfo->prod = libxl__strdup(NOGC, buf);
> > > > +    }
> > >   
> > > Basically, starting here, we have information which won't be available
> > > if we're using a pvusb driver domain.
> > >   
> > > This information is nice-to-have, but I don't think this information is
> > > directly relevant to libxl or xl; the funcitonality to get this
> > > information is available from other libraries like libusb.  I'm inclined
> > > to say that if we want to have pvusb driver domains (and I think we do),
> > > we should just get rid of this information.
> > For command 'xl usb-list', those information should be reported to user.
> > Do you mean we could call libusb to get thoes information directly in
> > xl toolstack and get rid of this function?
> >
> > I think we can keep the function, since every other device type has the
> > function XXX_getinfo. But here we could check backend_domid, for
> > backend=dom0, doing above work; for backend!=dom0 (e.g. pvusb
> > driver domain, no matter how, it should also be able to let users getting
> > those information. Can add code  in future.)
> Does xl need that information?  Can't the user get that information from
> lsusb?
>
> In any case, I can see why it would be *useful* to have in xl.  But
> about having it in libxl, I think this question sort of goes along with
> the question about the next patch -- whether libxl should be in the
> business of providing general information about the USB devices it's
> handling, or whether it should limit itself to doing what is absolutely
> necessary to talk to usbback.
>
> There's a part of me that sees the point of it: it's not actually that
> much extra code (at least for Linux), and it makes it easy to add some
> very useful features to xl.
>
> On the other hand, it's not portable to other OSes.  At the moment of
> course pvusb isn't portable either, but once we have qemu USB (providing
> either emulated or PV usb) then I *think* most of the other
> functionality will Just Work on any platform that can compile qemu
> (incl. FreeBSD, NetBSD, &c), won't it?  The code you're introducing here
> would have to be re-implented for those platforms, and for every new
> platform that wanted to include this functionality, wouldn't it?
So, about the portability problem, I think it's back to: do need to
update code to call libusb instead of reading sysfs now? Except
for this function, still have places reading sysfs to get hostbus,
hostaddr, vendorId, deviceId. Those are not portable for other
platform.

And about getting rid of the function, currently it's only needed in
'xl usb-list' and 'xl usb-assignable-list', if not providing general
information, yes, we can remove it.

-Chunyan

> The libusb interface does look a bit clunky; it would certainly be more
> convenient for developers to use the interface you've provided here.
>
>   -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel