On 7/20/25 21:25, Rob Landley wrote:
I can change it here, let me take a closer look this evening. (I'm in
tokyo at the moment juggling other commitments...)
"git am" warned of a bunch of trailing whitespace errors, so I did a
sed -i 's/ *$//' toys/*/lsusb.c
Why did you change id1 and id2 to unsigned in get_names()? (The protocol
fields are 16 bit, int is 32 bit, how can it be negative? Do you have a
test case this changed the behavior for?)
Since unsigned defaults to int (in C99, probably C89 too) you can just
say "unsigned" without having to say "unsigned int". (Code style:
shortest representation for consistency, you only have to specify
non-default attributes.)
toys/other/lsusb.c: In function 'create_device':
toys/other/lsusb.c:207:44: warning: '%s' directive output may be truncated
writing 6 bytes into a region of size between 0 and 255 [-Wformat-truncation=]
207 | snprintf(fullpath, sizeof(fullpath), "%s/%s", path, file);
| ^~
In function 'read_sysfs_uint',
inlined from 'create_device' at toys/other/lsusb.c:284:17:
toys/other/lsusb.c:207:3: note: 'snprintf' output between 8 and 263 bytes into
a destination of size 256
207 | snprintf(fullpath, sizeof(fullpath), "%s/%s", path, file);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I generally try to have it compile without warnings, even silly ones.
Hmmm, "diff -u <(lsusb -t) <(./lsusb -t)" doesn't share a single common
line of output. I ran it through "sort" and did "diff -bu" and it's
still not matching anything. So what's different...
/usr/bin/lsusb -t says:
-/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ehci-pci/2p, 480M
toybox lsusb -t says:
+/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=hub/0p, 480M
I dunno what the "driver" field should be, but it's not the same info.
// Tree display functions
static struct usb_bus *usb_buses = NULL;
Globals go in the GLOBALS() block, it's a memory efficiency thing. (See
https://landley.net/toybox/code.html#toy_union .)
You don't need to initialize global or static variables, ELF insures
they default to null/zero when sticking them in the .bss section.
Speaking of your struct types with the fixed size buffers, notice the
difference between:
struct usb_bus {
struct usb_bus *next;
struct usb_device *first_child;
unsigned busnum;
char name[64];
};
struct dev_ids {
struct dev_ids *next, *child;
int id;
char name[];
};
Two things:
1) Those are functionally the same struct, tree of int with string, you
could presumably have reused dev_ids. (Ok, avoids a typecast for the
child...)
2) Mine had a dynamically sized string field at the end so malloc() can
measure the string it's about to copy and allocate enough space. Yours
is doing a fixed length buffer which you trust /proc never to provide
too much data to. (I don't trust /proc not to have random crap --bind
mounted over it in container du jour, things like scan_uevent() trusting
new->name to fit in 256 bytes with the null terminator is because that
data came from readdir() which came getdents() which came from the VFS
which limits each path component size to a max length of 255 bytes and
says it can't contain / or NUL.)
Gotta come back to this later...
Thanks,
Rob
Rob
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
