[Spice-devel] [PATCH spice-gtk 10/10] usbredir: Give devices a user friendly description

[Hans de Goede]

Hi,

On 12/23/2011 07:16 PM, Christophe Fergeau wrote:
> On Mon, Dec 19, 2011 at 12:24:43PM +0100, Hans de Goede wrote:

<snip>

>> +
>> +#ifdef __linux__
>> +/*<Sigh>  libusb does not allow getting the manufacturer and product strings
>> +   without opening the device, so grab them directly from sysfs */
>> +static void spice_usb_device_manager_get_sysfs_attribute(int bus, int address,
>> +    const char *attribute, char *buf, int buf_size)
>
> Any idea if using /usr/share/hwdata/usb.ids would give better info?

No I believe it does not, example:

Manufacturer + Product strings from sysfs:
SanDisk Cruzer Blade
USB Flash Disk

usb.ids:
SanDisk Corp. Cruzer Blade
Alcor Micro Corp. Transcend JetFlash Flash Drive


The usb.ids is certainly more detailed for the 2nd example
(the no name usb stick which was handed out at guadec The Hague),
but I believe that the sysfs strings are more user friendly,
also:
1) as the sysfs strings are read from the actual device they
    will also work with very new devices
2) if we want to use /usr/share/hwdata/usb.ids we will need
    an additional dependency

>
>> +{
>> +    struct stat stat_buf;
>> +    char filename[256];
>> +    FILE *f;
>> +
>> +    buf[0] = '\0';
>> +
>> +    snprintf(filename, sizeof(filename), "/dev/bus/usb/%03d/%03d",
>> +             bus, address);
>> +    if (stat(filename,&stat_buf) != 0)
>> +        return;
>> +
>> +    snprintf(filename, sizeof(filename), "/sys/dev/char/%d:%d/%s",
>> +             major(stat_buf.st_rdev), minor(stat_buf.st_rdev), attribute);
>
>
>> +    f = fopen(filename, "r");
>> +    if (!f)
>> +        return;
>> +
>> +    if (fgets(buf, buf_size, f))
>> +        buf[strlen(buf) - 1] = '\0';
>> +    else
>> +        buf[0] = '\0';
>> +
>> +    fclose(f);
>
> I'd use g_file_get_contents here.

Ah I didn't know about that one, good idea, I'll send a new revision of this
patch using g_file_get_contents.





>
>> +}
>> +#endif
>>   #endif
>>
>>   /* ------------------------------------------------------------------ */
>> @@ -843,14 +881,36 @@ gchar *spice_usb_device_get_description(SpiceUsbDevice *_device)
>>   {
>>   #ifdef USE_USBREDIR
>>       libusb_device *device = (libusb_device *)_device;
>> -    int bus, address;
>> +    struct libusb_device_descriptor desc;
>> +    int rc, bus, address;
>> +    /* USB descriptor strings are max 128 bytes */
>> +    char manufacturer[128] = "";
>> +    char product[128] = "";
>>
>>       g_return_val_if_fail(device != NULL, NULL);
>>
>>       bus     = libusb_get_bus_number(device);
>>       address = libusb_get_device_address(device);
>>
>> -    return g_strdup_printf("USB device at %d-%d", bus, address);
>> +#if __linux__
>> +    spice_usb_device_manager_get_sysfs_attribute(bus, address, "manufacturer",
>> +                                           manufacturer, sizeof(manufacturer));
>> +    spice_usb_device_manager_get_sysfs_attribute(bus, address, "product",
>> +                                                 product, sizeof(product));
>> +#endif
>> +    if (!manufacturer[0])
>> +        strcpy(manufacturer, "USB");
>> +    if (!product[0])
>> +        strcpy(product, "Device");
>> +
>> +    rc = libusb_get_device_descriptor(device,&desc);
>> +    if (rc != LIBUSB_SUCCESS) {
>> +        return g_strdup_printf("%s %s at %d-%d", manufacturer, product,
>> +                               bus, address);
>> +    }
>> +
>> +    return g_strdup_printf("%s %s [%04x:%04x] at %d-%d", manufacturer, product,
>> +                           desc.idVendor, desc.idProduct, bus, address);
>
> Should these strings be marked as translatable?

Good one, yes they should. I will change that in the 2nd revision too.

Regards,

Hans