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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

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