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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 13/10/15 14:15, 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.

Just to be clear: Reading sysfs to actually implement the core pvusb
protocol is fine, because at the moment there *is* only a Linux backend.
 You should be able to get the core pvusb functionality in without
resorting to libusb.  It's just the additional usb information that we
may want to use libusb for, to avoid having to duplicate that for other
platforms once we have qemu-usb enabled.

 -George

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