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

[George Dunlap <george.dunlap@citrix.com>]

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?

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