From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KjFPO-0004dh-SB for qemu-devel@nongnu.org; Fri, 26 Sep 2008 11:39:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KjFPM-0004bm-Bg for qemu-devel@nongnu.org; Fri, 26 Sep 2008 11:39:05 -0400 Received: from [199.232.76.173] (port=45007 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KjFPL-0004bX-Sp for qemu-devel@nongnu.org; Fri, 26 Sep 2008 11:39:03 -0400 Received: from rn-out-0910.google.com ([64.233.170.184]:38896) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KjFPK-00055r-Qy for qemu-devel@nongnu.org; Fri, 26 Sep 2008 11:39:03 -0400 Received: by rn-out-0910.google.com with SMTP id m61so707486rnd.8 for ; Fri, 26 Sep 2008 08:39:02 -0700 (PDT) Message-ID: <48DD01D6.50909@codemonkey.ws> Date: Fri, 26 Sep 2008 10:37:58 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] Add USB sys file-system support (v6) References: <1220571341.2638.6.camel@hephaestion> <1220580385.2638.15.camel@hephaestion> <48C1346F.3000405@windriver.com> <1220640699.5470.15.camel@hephaestion> <48C1862C.3050307@windriver.com> <1220649226.9611.13.camel@hephaestion> <48C53B04.9030006@windriver.com> <1221679892.17792.6.camel@hephaestion> <48D16904.30104@codemonkey.ws> <1221691647.17792.55.camel@hephaestion> <48D81E26.9080802@codemonkey.ws> <1222133639.18297.13.camel@hephaestion> <48DBCE35.4030607@codemonkey.ws> <48DCEA3E.7010006@windriver.com> <48DCF168.5080502@codemonkey.ws> <48DCFC18.5010506@windriver.com> In-Reply-To: <48DCFC18.5010506@windriver.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wessel Cc: TJ , qemu-devel@nongnu.org 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