From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Kg3gv-0007ug-GX for qemu-devel@nongnu.org; Wed, 17 Sep 2008 16:32:01 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Kg3gu-0007uG-IB for qemu-devel@nongnu.org; Wed, 17 Sep 2008 16:32:00 -0400 Received: from [199.232.76.173] (port=51984 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Kg3gu-0007u4-Ad for qemu-devel@nongnu.org; Wed, 17 Sep 2008 16:32:00 -0400 Received: from mail-gx0-f19.google.com ([209.85.217.19]:34812) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Kg3gt-0008UA-RO for qemu-devel@nongnu.org; Wed, 17 Sep 2008 16:32:00 -0400 Received: by gxk12 with SMTP id 12so28789425gxk.10 for ; Wed, 17 Sep 2008 13:31:57 -0700 (PDT) Message-ID: <48D16904.30104@codemonkey.ws> Date: Wed, 17 Sep 2008 15:31:00 -0500 From: Anthony Liguori MIME-Version: 1.0 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> In-Reply-To: <1221679892.17792.6.camel@hephaestion> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] Add USB sys file-system support (v4) Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: TJ Cc: qemu-devel@nongnu.org, kvm 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 > 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