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

[Anthony Liguori <anthony@codemonkey.ws>]

Jason Wessel wrote:
> Anthony Liguori wrote:
>   
> My comment on technical content would be that the several places that
> atoi() and strtoul() the values are not checked for any kind of
> correctness and subsequently pass on.  Perhaps no validity check is
> needed because it is dealt with in the later call to the passed in
> function...  It just was not immediately obvious.
>   

Okay, the patch needs some refactoring.  I can commit and refactor or 
someone else can refactor and resubmit.  Either works for me.  Anyway:

> +/*
> + * 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))) {
> +        if (de->d_name[0] != '.' && ! strchr(de->d_name, ':')) {
> +            char filename[PATH_MAX];
> +            char* tmpstr = de->d_name;
> +            if (!strncmp(de->d_name, "usb", 3))
> +                tmpstr += 3;
> +
> +            bus_num = atoi(tmpstr);
> +            snprintf(filename, PATH_MAX, USBSYSBUS_PATH "/devices/%s/devnum", de->d_name);
> +            f = fopen(filename, "r");
> +            if (!f) {
> +                term_printf("Could not open %s\n", filename);
> +                goto the_end;
> +            }
> +            fgets(line, sizeof(line), f);
> +            fclose(f);
> +            addr = atoi(line);
>   

This pattern, snprintf() of a filename, open, read from it, then parse 
the output is very frequently repeated.  It could be collapsed into:

if (usb_host_read_file(buffer, sizeof(buffer), USBSYSBUS_PATH 
"/devices/%s/devnum", de->d_name))
    goto the_end;
if (sscanf(buffer, "%d", &addr) != 1)
    goto the_end;

And it would greatly simplify the code.
> +/*
> + * Determine how to access the host's USB devices and call the
> + * specific support function.
> + */
> +static int usb_host_scan(void *opaque, USBScanFunc *func)
> +{
> +    FILE *f = 0;
> +    DIR *dir = 0;
> +    int ret = 0;
> +    const char *devices = "/devices";
> +    const char *trying = "husb: trying to open %s%s\n";
> +    const char *failed = "husb: could not open %s%s\n";
> +    char devpath[PATH_MAX];
> +
> +    /* only check the host once */
> +    if (!usb_fs_type) {
> +        /* test for dev file-system access in /proc/ */
> +        dprintf(trying, USBPROCBUS_PATH, devices);
> +        f = fopen(USBPROCBUS_PATH "/devices", "r");
> +        if (!f) {
>   

We really want an affirmative test here, that goto's the switch 
statement instead of nesting deeply.

With those changes, the patch is ready to apply.

Regards,

Anthony Liguori