usb_util.c [v2] #178

bp0 · 2017-08-19T02:54:21Z

A set of functions for getting information for a single USB device,
or a list of all devices.

The only implemented method is using lsusb, which is slow. A
method using sysfs would be much better. The existing sysfs and
procfs methods in devices/usb.c do not appear to work, so it would
have to be something new.

devices/usb.c modified to use usb_util, but all the old code is
still there.

Solves #162 for devices/usb.c
Before it was common for lsusb to leak the message below once for each device.
Couldn't open device, some information will be missing

Fixed a problem with #175.

bp0 · 2017-08-21T22:13:17Z

Rebased.

lpereira

Nice that you're breaking out these things into separate files. However, there are some questions/comments that I'd like to see addressed first before merging this patch.

lpereira · 2017-09-10T15:15:22Z

hardinfo/usb_util.c

+    return usbd_list_append(s, NULL);
+}
+
+char *_lsusb_lv(char *line, const char *prefix) {


Internal functions should always be marked as static. There's no need to prepend an underscore in their names. I'd prefer if their names were more descriptive as well: I have no idea what lv means here.

lpereira · 2017-09-10T15:15:58Z

hardinfo/usb_util.c

+}
+
+char *_lsusb_lv(char *line, const char *prefix) {
+    if ( g_str_has_prefix(line, prefix) ) {


No spaces after or before parenthesis: if (g_str_has_prefix(...)) {.

lpereira · 2017-09-10T15:19:13Z

hardinfo/usb_util.c

+int usb_get_device_lsusb(int bus, int dev, usbd *s) {
+    gboolean spawned;
+    gchar *out, *err, *p, *l, *t, *next_nl;
+    gchar *lsusb_cmd = g_strdup_printf("lsusb -s %d:%d -v", bus, dev);


I wonder if we would benefit from a h_popen_printf() that worked pretty much like a printf() function. Maybe with fopencookie(), this could be used like popen():

FILE *lsusb = h_popen_printf("lsusb -s %d:%d -v", bus, dev); char buffer[1024]; if (!lsusb) return -errno; while (fgets(buffer, sizeof(buffer), lsusb)) { ... } h_popen_close(lsusb);

This would simplify a lot all these places where strchr() and strend() are used inside a loop: these could be moved to one of the cookie functions.

(No need to do this for this patch, just thinking out loud.)

lpereira · 2017-09-10T15:21:08Z

hardinfo/usb_util.c

+        }
+        g_free(out);
+        g_free(err);
+        return 1;


If a function is returning only 1 or 0, it should return a boolean value instead.

lpereira · 2017-09-10T15:21:58Z

modules/devices/usb.c

-             __scan_usb_lsusb();
+#define UNKIFNULL_AC(f) (f != NULL) ? f : _("(Unknown)");
+
+void _usb_dev(const usbd *u) {


lpereira · 2017-09-10T15:24:38Z

hardinfo/usb_util.c

+        p = out;
+        while(next_nl = strchr(p, '\n')) {
+            strend(p, '\n');
+            if (ec = sscanf(p, "Bus %d Device %d: ID %x:%x", &bus, &dev, &vend, &prod) ) {


Won't this issue a warning? You're likely to need two parenthesis here. Or, better yet, just write in two lines:

ec = sscanf(...); if (ec == 4) { ... }

lpereira · 2017-09-10T15:25:12Z

hardinfo/usb_util.c

+}
+
+/* returns number of items after append */
+int usbd_list_append(usbd *l, usbd *n) {


Why not just use a GSList?

lpereira · 2017-09-10T15:26:37Z

hardinfo/usb_util.c

+#include "usb_util.h"
+
+usbd *usbd_new() {
+    usbd *s = malloc(sizeof(usbd));


Use g_new0() instead. It already checks if the allocation fails and will most likely allocate memory from a pool that has been memset() to zero already (usually fresh from the kernel). Be sure to use g_free() for things allocated with g_new().

lpereira · 2017-09-10T15:27:24Z

hardinfo/usb_util.c

+            g_strstrip(p);
+            if (l = _lsusb_lv(p, "idVendor")) {
+                s->vendor_id = strtol(l, NULL, 0);
+                if (t = strchr(l, ' '))


This will generate a warning. You need two parenthesis enclosing the expression here: if ((t = strchr(...)))

lpereira · 2017-09-10T15:27:47Z

hardinfo/usb_util.c

+            if (l = _lsusb_lv(p, "idVendor")) {
+                s->vendor_id = strtol(l, NULL, 0);
+                if (t = strchr(l, ' '))
+                    s->vendor = strdup(t + 1);


Please use g_strdup(), it already NULL-checks the return value and aborts the program if allocation fails

A set of functions for getting information for a single USB device, or a list of all devices. The only implemented method is using `lsusb`, which is slow. A method using sysfs would be much better. The existing sysfs and procfs methods in devices/usb.c do not appear to work, so it would have to be something new. devices/usb.c modified to use usb_util, but all the old code is still there. Signed-off-by: Burt P <pburt0@gmail.com>

Signed-off-by: Burt P <pburt0@gmail.com>

lpereira

Just some nits, and this can be merged. :)

lpereira · 2017-10-09T15:07:58Z

hardinfo/usb_util.c

+    return c;
+}
+
+int usbd_list_count(usbd *s) {


It's very surprising that a function to count the elements of a linked list actually modifies it. This should really be just a wrapper around GSList.

Because usbd_list_append() appends an element and returns the length, usbd_list_count() can just call it without a new element and get the length. append is already stepping through the list, so I just re-used it.

lpereira · 2017-10-09T15:09:12Z

includes/usb_util.h

+#define __USB_UTIL_H__
+
+/* this is a linked list */
+typedef struct usbd {


For things that are global identifiers, it's better to have "namespaces", or at least longer names. Shorter names are useful for static functions or local symbols within functions.

Would something like usb_device_info be enough?

mckaygerhard · 2017-10-24T21:40:49Z

hi now that some review was made .. this will be compatible with some older hardware? i mean this uses sysfs, what happened with those older systems that are kernel usage << 3.0.24 ? for some embebed linux

bp0 · 2017-11-25T16:16:24Z

@mckaygerhard, this adds a new method, but falls back to the previously existing methods. If they ever worked, which I have not been able to confirm, then they should still work.

lpereira · 2018-03-12T16:20:22Z

Merged, thanks!

bp0 mentioned this pull request Aug 19, 2017

Check usage of popen() for leaks via stderr #162

Open

14 tasks

bp0 force-pushed the usb2 branch 4 times, most recently from a80b27e to 356c541 Compare August 21, 2017 22:11

bp0 force-pushed the usb2 branch from 356c541 to db064fa Compare August 28, 2017 21:55

lpereira requested changes Sep 10, 2017

View reviewed changes

bp0 added 2 commits September 10, 2017 12:10

usb: requested changes

5683d3d

Signed-off-by: Burt P <pburt0@gmail.com>

bp0 force-pushed the usb2 branch from db064fa to 5683d3d Compare September 10, 2017 17:11

lpereira requested changes Oct 9, 2017

View reviewed changes

bp0 mentioned this pull request Mar 9, 2018

lsusb from usbutils currently required for usb information #226

Closed

lpereira merged commit 6f0a3e0 into lpereira:master Mar 12, 2018

bp0 deleted the usb2 branch March 12, 2018 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb_util.c [v2] #178

usb_util.c [v2] #178

bp0 commented Aug 19, 2017

bp0 commented Aug 21, 2017

lpereira left a comment

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira Sep 10, 2017

lpereira left a comment

lpereira Oct 9, 2017

bp0 Nov 25, 2017

lpereira Oct 9, 2017

bp0 Nov 25, 2017

mckaygerhard commented Oct 24, 2017

bp0 commented Nov 25, 2017

lpereira commented Mar 12, 2018

usb_util.c [v2] #178

usb_util.c [v2] #178

Conversation

bp0 commented Aug 19, 2017

bp0 commented Aug 21, 2017

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckaygerhard commented Oct 24, 2017

bp0 commented Nov 25, 2017

lpereira commented Mar 12, 2018