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

[Chun Yan Liu <cyliu@xxxxxxxxxx>]

On 10/13/2015 09:15 PM, George Dunlap wrote:
> On 13/10/15 02:46, Chun Yan Liu wrote:
> > 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.
> I realize I didn't give you very clear guidance; I guess I was hoping to
> get an opinion from the tools maintainers.  Or perhaps, I was hoping to
> let them be the "bad guys" and say, "You can't have this feature in
> libxl", so I wouldn't have to. :-)
>
> In the absence of guidance to the contrary, I suggest that patch series
> should focus on getting the core pvusb functionality in, without the
> extra usb-querying bits.  Then we can discuss a further series which
> either adds the usb querying functionality to libxl, or implement it in
> xl using libusb.
OK. Got it. Thanks.

-Chunyan

>   -George

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