[Toybox] [PATCH v2] lsusb: add -t option for tree format display

[Patel, Jignesh1]

Hi Rob,

Thank you for the detailed review. I've tried to address all your concerns in v2. Here are my inline responses:

> Roughly doubles the size of the command (diffstat says it adds 200 lines to a command that previously had ~220 lines after the header), and does not seem to share any infrastructure outside the main() function except get_names()...
- In v2, I've reused existing parse_dev_ids() for usb.ids parsing with class support, 
- Leveraged dev_ids structure throughout instead of custom structs  
- Eliminated custom read functions in favor of standard sysfs operations

> Not just the order changing but what it's outputting... Pity there isn't a standard format for this information...
- Fixed in v2. The output now matches with usbutils exactly, including proper bus numbering, port hierarchy, and device information. 
- The driver field now correctly shows actual controller names and port counts.

e.g.
server at ubuntu: $ ./lsusb -t -i ~/usb.ids
/:  Bus 001.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/12p, 480M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 480M
    |__ Port 010: Dev 003, If 0, Class=Wireless, Driver=btusb, 12M
    |__ Port 010: Dev 003, If 1, Class=Wireless, Driver=btusb, 12M
/:  Bus 002.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/6p, 10000M
    |__ Port 002: Dev 002, If 0, Class=Hub, Driver=hub/4p, 5000M
        |__ Port 004: Dev 003, If 0, Class=Vendor Specific Class, Driver=r8152, 5000M
/:  Bus 003.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 480M
/:  Bus 004.Port 001: Dev 001, Class=root_hub, Driver=xhci_hcd/2p, 10000M

> 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?)
You're correct - reverted to original int types in v2. No functional change was needed for 16-bit protocol fields in 32-bit int.

> Since unsigned defaults to int (in C99, probably C89 too) you can just say "unsigned" without having to say "unsigned int".
Noted for future - using shortest representation for consistency.

> I generally try to have it compile without warnings, even silly ones.
In v2, I have adopted your dynamic allocation pattern.

> 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.
Fixed in v2, reads actual driver information from sysfs: Extracts the driver name from the full symlink path.

> Globals go in the GLOBALS() block, it's a memory efficiency thing.
All globals moved to GLOBALS() block in v2.

> You don't need to initialize global or static variables, ELF insures they default to null/zero
Removed unnecessary initializations.

> 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 ...
In v2, reused the existing dev_ids struct with a flexible char name[] field, and used dynamic memory allocation wherever necessary.

> Those are functionally the same struct, tree of int with string, you could presumably have reused dev_ids.
v2 reuses struct dev_ids throughout, eliminating custom struct duplication.


Best Regards,
Jignesh

-----Original Message-----
From: Rob Landley <rob at landley.net> 
Sent: Tuesday, July 22, 2025 6:44 AM
To: Patel, Jignesh1 <jignesh1.patel at intel.com>; toybox at lists.landley.net
Subject: Re: [Toybox] lsusb: add -t option for tree format display

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PATCH-v2-lsusb-add-t-option-for-tree-format-display.patch
Type: application/octet-stream
Size: 15319 bytes
Desc: 0001-PATCH-v2-lsusb-add-t-option-for-tree-format-display.patch
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20250726/eda9ff69/attachment-0001.obj>