[Qemu-devel] Re: [PATCH] Add USB sys file-system support (v4)

[Anthony Liguori]

TJ wrote:
> This is a white-space clean-up based on Jason Wessel's review on 8th
> September.
>
> It includes a small addition in the form of a correction for a dprintf()
> that was part of the patch context before.
>
> It doesn't include the suggested alteration to "printf("husb: config #%d
> need %d\n" since that line is well outside the patch (I believe that
> belongs to the code Jason was patching with the ioctl issue).
>
> --------------------------------------
> This patch adds support for host USB devices discovered via:
>
> /sys/bus/usb/devices/* and opened from /dev/bus/usb/*/*
> /dev/bus/usb/devices and opened from /dev/bus/usb/*/*
>
> in addition to the existing discovery via:
>
> /proc/bus/usb/devices and opened from /proc/bus/usb/*/*
>
> Signed-off-by: TJ <address@hidden>
>   

A valid Signed-off-by requires a full name, not just an abbreviation.

> +
> +static char *USBHostDevicePath = 0;
>   

usb_host_device_path please.  Also, the explicit initialization to 0 is 
unnecessary.  This should probably be a const char * too?

> +#define USB_FS_NONE 0
> +#define USB_FS_PROC 1
> +#define USB_FS_DEV 2
> +#define USB_FS_SYS 3
> +
> +static int USBfs = 0;
>   

usb_fs_type.

>  struct sigaction sigact;
>  
> @@ -600,13 +615,18 @@ static USBDevice *usb_host_device_open_addr(int bus_num, int addr, const char *p
>  
>      printf("husb: open device %d.%d\n", bus_num, addr);
>  
> -    snprintf(buf, sizeof(buf), USBDEVFS_PATH "/%03d/%03d",
> +       if (!USBHostDevicePath) {
> +               perror("husb: USB Host Device Path not set");
> +               goto fail;
> +       }
> +    snprintf(buf, sizeof(buf), "%s/%03d/%03d", USBHostDevicePath,
>               bus_num, addr);
>      fd = open(buf, O_RDWR | O_NONBLOCK);
>      if (fd < 0) {
>          perror(buf);
>          goto fail;
>      }
> +    printf("husb: opened %s\n", buf);
>   

Please don't add unconditional printfs.
> +/*
> + Use /proc/bus/usb/devices or /dev/bus/usb/devices file to determine
> + host's USB devices. This is legacy support since many distributions
> + are moving to /sys/bus/usb
> +*/
> +static int usb_host_scan_dev(void *opaque, USBScanFunc *func)
> +{
> +       FILE *f = 0;
> +       char line[1024];
> +       char buf[1024];
> +       int bus_num, addr, speed, device_count, class_id, product_id, vendor_id;
> +       char product_name[512];
> +       int ret = 0;
> +
> +       snprintf(line, sizeof(line), "%s/devices", USBHostDevicePath);
> +       f = fopen(line, "r");
> +       if (!f) {
> +               perror("husb: cannot open devices file");
> +               goto the_end;
> +       }
> +
> +       device_count = 0;
> +       bus_num = addr = speed = class_id = product_id = vendor_id = 0;
> +       for(;;) {
> +               if (fgets(line, sizeof(line), f) == NULL)
> +                       break;
> +               if (strlen(line) > 0)
> +                       line[strlen(line) - 1] = '\0';
> +               if (line[0] == 'T' && line[1] == ':') {
> +                       if (device_count && (vendor_id || product_id)) {
> +                               /* New device.  Add the previously discovered 
> device.  */
> +                               ret = func(opaque, bus_num, addr, class_id, 
> vendor_id,
> +                                                       product_id, 
> product_name, speed);
> +                               if (ret)
> +                                       goto the_end;
> +                       }
> +                       if (get_tag_value(buf, sizeof(buf), line, "Bus=", " ") 
> < 0)
> +                               goto fail;
> +
> +                       bus_num = atoi(buf);
> +                       if (get_tag_value(buf, sizeof(buf), line, "Dev#=", " ") 
> < 0)
> +                               goto fail;
> +
> +                       addr = atoi(buf);
> +                       if (get_tag_value(buf, sizeof(buf), line, "Spd=", " ") 
> < 0)
> +                               goto fail;
> +
> +                       if (!strcmp(buf, "480"))
> +                               speed = USB_SPEED_HIGH;
> +                       else if (!strcmp(buf, "1.5"))
> +                               speed = USB_SPEED_LOW;
> +                       else
> +                               speed = USB_SPEED_FULL;
> +                       product_name[0] = '\0';
> +                       class_id = 0xff;
> +                       device_count++;
> +                       product_id = 0;
> +                       vendor_id = 0;
> +               }
> +               else if (line[0] == 'P' && line[1] == ':') {
> +                       if (get_tag_value(buf, sizeof(buf), line, "Vendor=", " ") 
> < 0)
> +                               goto fail;
> +
> +                       vendor_id = strtoul(buf, NULL, 16);
> +                       if (get_tag_value(buf, sizeof(buf), line, "ProdID=", " ") 
> < 0)
> +                               goto fail;
> +
> +                       product_id = strtoul(buf, NULL, 16);
> +               }
> +               else if (line[0] == 'S' && line[1] == ':') {
> +                       if (get_tag_value(buf, sizeof(buf), line, "Product=", "") 
> < 0)
> +                               goto fail;
> +
> +                       pstrcpy(product_name, sizeof(product_name), buf);
> +               }
> +               else if (line[0] == 'D' && line[1] == ':') {
> +                       if (get_tag_value(buf, sizeof(buf), line, "Cls=", " (") 
> < 0)
> +                               goto fail;
> +
> +                       class_id = strtoul(buf, NULL, 16);
> +               }
> + fail: ;
> +       }
> +       if (device_count && (vendor_id || product_id)) {
> +               /* Add the last device.  */
> +               ret = func(opaque, bus_num, addr, class_id, vendor_id,
> +                                       product_id, product_name, speed);
> +       }
> + the_end:
> +       if (f) fclose(f);
> +       return ret;
> +}
> +
> +/*
> + Use /sys/bus/usb/devices/ directory to determine host's USB devices.
> +
> + This code is taken from Robert Schiele's original patches posted to the
> + Novell bug-tracker https://bugzilla.novell.com/show_bug.cgi?id=241950
> +*/
> +static int usb_host_scan_sys(void *opaque, USBScanFunc *func)
> +{
> +       FILE *f;
> +       DIR *dir = 0;
> +       char line[1024];
> +       int bus_num, addr, speed, class_id, product_id, vendor_id;
> +       int ret = 0;
> +       char product_name[512];
> +       struct dirent* de;
> +
> +       dir = opendir(USBSYSBUS_PATH "/devices");
> +       if (!dir) {
> +               perror("husb: cannot open devices directory");
> +               goto the_end;
> +       }
> +
> +       while ((de = readdir(dir))) {
>   

This indenting is wrong.  QEMU uses 4-space tabs.

> +
> +/*
> + Determine how to access the host's USB devices and call the specific
> + support function.
> +
> + Introduced to fix Ubuntu LP #156085
> + https://bugs.launchpad.net/ubuntu/+bug/156085
> + */
>   

Please don't add these sorts of references.  They quickly become stale 
as code changes.

>  static int usb_host_scan(void *opaque, USBScanFunc *func)
>  {
> -    FILE *f;
> -    char line[1024];
> -    char buf[1024];
> -    int bus_num, addr, speed, device_count, class_id, product_id, vendor_id;
> -    int ret;
> -    char product_name[512];
> +       FILE *f = 0;
> +       DIR *dir = 0;
> +       int ret = 0;
> +       char *devices = "/devices";
> +       char *trying = "husb: trying to open %s%s\n";
> +       char *failed = "husb: could not open %s%s\n";
> +       char devpath[PATH_MAX];
> +
> +       // only check the host once
> +       if (!USBfs) {
> +               // test for dev file-system access in /proc/
> +               term_printf(trying, USBPROCBUS_PATH, devices);
> +               f = fopen(USBPROCBUS_PATH "/devices", "r");
> +               if (!f) {
> +                       term_printf(failed, USBPROCBUS_PATH, devices);
> +                       // maybe it has been moved to the /dev/ base
> +                       term_printf(trying, USBDEVBUS_PATH, devices);
> +                       f = fopen(USBDEVBUS_PATH "/devices", "r");
> +                       if (!f) {
> +                               term_printf(failed, USBDEVBUS_PATH, devices);
> +                               // test for newer sys file-system access
> +                               term_printf(trying, USBSYSBUS_PATH, devices);
> +                               dir = opendir(USBSYSBUS_PATH "/devices");
> +                               if (!dir) {
> +                                       term_printf(failed, USBSYSBUS_PATH, 
> devices);
> +                                       goto the_end;
> +                               }
> +                               else { // devices found in /dev/bus/usb/ (yes - 
> not a mistake!)
> +                                       strcpy(devpath, USBDEVBUS_PATH);
> +                                       USBfs = USB_FS_SYS;
> +                               }
> +                               if (dir) closedir(dir);
> +                       }
> +                       else { // devices found in /dev/bus/usb/
> +                               strcpy(devpath, USBDEVBUS_PATH);
> +                               USBfs = USB_FS_DEV;
> +                       }
> +               }
> +               else { // devices found in /proc/bus/usb/
> +                       strcpy(devpath, USBPROCBUS_PATH);
> +                       USBfs = USB_FS_PROC;
> +               }
> +               if (f) fclose(f);
>   

The depth of this indentation is nuts.  The term_printfs are also way 
too verbose.

Did you reindent a bunch of existing code?  It's hard to tell from this 
patch.  If so, please don't do that sort of thing in a patching adding 
functionality.  It makes it very hard to review.

Regards,

Anthony Liguori