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

[Chun Yan Liu <cyliu@novell.com>]

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
>