Add USB devices#424
Conversation
215b125 to
03aa3e7
Compare
|
Fixed the build errors for Mac OS X and the linting errors about missing error-return-checks. |
11861e0 to
67402b7
Compare
|
@christoph-zededa thanks so much for this submission! I'll try to do a full review later today or tomorrow :) |
67402b7 to
1a152f1
Compare
ffromani
left a comment
There was a problem hiding this comment.
many thanks for the submission. I think that a usb package will benefit the project.
I'm very wary to add more deps, we should rather head to remove them all.
This is especially true if we can get similar (or identical?) results using golang code and peeking around in /sys.
Other than that, the code looks reasonnable, but I think we can use a bit more tests.
At glance, API additions (user-facing APIs) looks good but I'll leave the final word to @jaypipes
| } | ||
| if err := showAccelerator(cmd, args); err != nil { | ||
| return err | ||
| fmt.Println("------------------------------") |
There was a problem hiding this comment.
minor: we should avoid fixed size separators; either avoid them for now or automatically align to the longest line, this approach will likely require extra code and/or a 3rd party helper.
There was a problem hiding this comment.
I forgot to remove it after debugging. Now it is gone.
| info.Baseboard.String(), | ||
| info.Product.String(), | ||
| info.PCI.String(), | ||
| info.USB.String(), |
There was a problem hiding this comment.
unrelated: this scales pretty poorly and is too hard to extend, I'll file a PR to send an improvement ASAP
| } | ||
|
|
||
| type Paths struct { | ||
| ChrootSys string |
There was a problem hiding this comment.
this should probably be "SysRoot"
There was a problem hiding this comment.
I renamed it to SysRoot
| fileSpecs := ExpectedCloneStaticContent() | ||
| fileSpecs = append(fileSpecs, ExpectedCloneNetContent()...) | ||
| fileSpecs = append(fileSpecs, ExpectedCloneUSBContent()...) | ||
| fileSpecs = append(fileSpecs, ExpectedClonePCIContent()...) | ||
| fileSpecs = append(fileSpecs, ExpectedCloneGPUContent()...) |
There was a problem hiding this comment.
likewise, this code probably deserves some cleanup, I'll take care in a different PR
| /* | ||
| #cgo pkg-config: libusb-1.0 | ||
| */ |
There was a problem hiding this comment.
Unless this provide critical functionalities we can't replace with our code, I think we should avoid this especially becase it requires C code
There was a problem hiding this comment.
The idea behind this is to use libusb on Mac OS X and use sysfs directly on Linux.
Perhaps it is better that I remove the OS X implementation now and later create another PR so this can be discussed there?
There was a problem hiding this comment.
ok, this is an important clarification which changes the perspective a bit. I would prefer this approach you outline in your last comment, but please don't do any change just yet, let's wait for @jaypipes 's review.
There was a problem hiding this comment.
@christoph-zededa yes, let's separate out the MacOSX support from the Linux support please. That way we can do a clean PR review on the Linux support and get that merged while we decide on a path forward for Mac.
There was a problem hiding this comment.
I removed it from this PR.
| import ( | ||
| "runtime" | ||
|
|
||
| "github.com/pkg/errors" |
There was a problem hiding this comment.
please use the native errors in new code
f2e3a5f to
8031e2b
Compare
jaypipes
left a comment
There was a problem hiding this comment.
@christoph-zededa please pull the latest main and rebase this branch in order to bring in the latest go.mod and other changes. I've left some comments inline for you as well.
So sorry for the long delay in reviewing and thank you again for the submission!
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| // netCmd represents the install command |
There was a problem hiding this comment.
| // netCmd represents the install command | |
| // usbCmd represents the `usb` command |
| RunE: showUSB, | ||
| } | ||
|
|
||
| // showNetwork show network information for the host system. |
There was a problem hiding this comment.
| // showNetwork show network information for the host system. | |
| // showUSB shows USB information for the host system. |
| "github.com/jaypipes/ghw/pkg/option" | ||
| ) | ||
|
|
||
| type USB struct { |
There was a problem hiding this comment.
I think this struct should be called Device.
There was a problem hiding this comment.
I renamed it to Device.
|
|
||
| } | ||
|
|
||
| return str.String() |
There was a problem hiding this comment.
This would probably be easier to read like this:
attrs := map[string]string{
"driver": u.Driver,
"type", u.Type,
"vendor_id", u.VendorID,
"product_id", u.ProductID,
"product", u.Product,
"revision_id": u.RevisionID,
"interface": u.Interface,
}
return strings.Join(
lo.MapToSlice(
attrs,
func (k, v string) string {
return fmt.Sprintf("%s=%q", k, v)
},
), " ",
)We use the samber/lo library for generic manipulations like the above.
There was a problem hiding this comment.
I don't see this library yet in go.mod, but I can add it.
One thing I see that with the map it is now unordered:
before:
christoph@ponyhof ~/p/g/c/ghwc (usb)> go build && ./ghwc usb
USB (19 USBs)
driver=hub type=9/0/1 vendorID=1d6b productID=2 revisionID=617
driver=hub type=9/0/3 vendorID=1d6b productID=3 revisionID=617
driver=hub type=9/0/1 vendorID=1d6b productID=2 revisionID=617
driver=usb type=224/1/1 vendorID=8087 productID=33 revisionID=0
driver=btusb type=224/1/1 vendorID=8087 productID=33 revisionID=0
driver=btusb type=224/1/1 vendorID=8087 productID=33 revisionID=0
driver=usb type=255/16/255 vendorID=6cb productID=fc revisionID=0
type=255/16/255 vendorID=6cb productID=fc revisionID=0
driver=usb type=239/2/1 vendorID=174f productID=1813 product="Integrated Camera" revisionID=1015
driver=uvcvideo type=239/2/1 vendorID=174f productID=1813 revisionID=1015 interface="Integrated Camera"
driver=uvcvideo type=239/2/1 vendorID=174f productID=1813 revisionID=1015
driver=uvcvideo type=239/2/1 vendorID=174f productID=1813 revisionID=1015 interface="Integrated IR Camera"
driver=uvcvideo type=239/2/1 vendorID=174f productID=1813 revisionID=1015
type=239/2/1 vendorID=174f productID=1813 revisionID=1015 interface="Camera DFU Device"
driver=hub type=9/0/3 vendorID=1d6b productID=3 revisionID=617
driver=usb type=9/0/1 vendorID=1d6b productID=2 product="xHCI Host Controller" revisionID=617
driver=usb type=9/0/3 vendorID=1d6b productID=3 product="xHCI Host Controller" revisionID=617
driver=usb type=9/0/1 vendorID=1d6b productID=2 product="xHCI Host Controller" revisionID=617
driver=usb type=9/0/3 vendorID=1d6b productID=3 product="xHCI Host Controller" revisionID=617
after:
christoph@ponyhof ~/p/g/c/ghwc (usb)> go build && ./ghwc usb
USB (19 USBs)
vendor_id="1d6b" product_id="2" product="" revision_id="617" interface="" driver="hub" type="9/0/1"
interface="" driver="hub" type="9/0/3" vendor_id="1d6b" product_id="3" product="" revision_id="617"
product="" revision_id="617" interface="" driver="hub" type="9/0/1" vendor_id="1d6b" product_id="2"
product_id="33" product="" revision_id="0" interface="" driver="usb" type="224/1/1" vendor_id="8087"
revision_id="0" interface="" driver="btusb" type="224/1/1" vendor_id="8087" product_id="33" product=""
product="" revision_id="0" interface="" driver="btusb" type="224/1/1" vendor_id="8087" product_id="33"
vendor_id="6cb" product_id="fc" product="" revision_id="0" interface="" driver="usb" type="255/16/255"
revision_id="0" interface="" driver="" type="255/16/255" vendor_id="6cb" product_id="fc" product=""
vendor_id="174f" product_id="1813" product="Integrated Camera" revision_id="1015" interface="" driver="usb" type="239/2/1"
type="239/2/1" vendor_id="174f" product_id="1813" product="" revision_id="1015" interface="Integrated Camera" driver="uvcvideo"
type="239/2/1" vendor_id="174f" product_id="1813" product="" revision_id="1015" interface="" driver="uvcvideo"
driver="uvcvideo" type="239/2/1" vendor_id="174f" product_id="1813" product="" revision_id="1015" interface="Integrated IR Camera"
type="239/2/1" vendor_id="174f" product_id="1813" product="" revision_id="1015" interface="" driver="uvcvideo"
product_id="1813" product="" revision_id="1015" interface="Camera DFU Device" driver="" type="239/2/1" vendor_id="174f"
driver="hub" type="9/0/3" vendor_id="1d6b" product_id="3" product="" revision_id="617" interface=""
vendor_id="1d6b" product_id="2" product="xHCI Host Controller" revision_id="617" interface="" driver="usb" type="9/0/1"
revision_id="617" interface="" driver="usb" type="9/0/3" vendor_id="1d6b" product_id="3" product="xHCI Host Controller"
revision_id="617" interface="" driver="usb" type="9/0/1" vendor_id="1d6b" product_id="2" product="xHCI Host Controller"
interface="" driver="usb" type="9/0/3" vendor_id="1d6b" product_id="3" product="xHCI Host Controller" revision_id="617"
There was a problem hiding this comment.
for now I have not included the change in the PR
There was a problem hiding this comment.
I don't see this library yet in
go.mod, but I can add it.
Doh, you are totally correct @christoph-zededa! I've used samber/lo in so many of my other libraries I just assumed we were using it in ghw!
There was a problem hiding this comment.
@christoph-zededa good point about the ordering. OK, let's leave this alone then :)
| /* | ||
| #cgo pkg-config: libusb-1.0 | ||
| */ |
There was a problem hiding this comment.
@christoph-zededa yes, let's separate out the MacOSX support from the Linux support please. That way we can do a clean PR review on the Linux support and get that merged while we decide on a path forward for Mac.
51cebaf to
e8bf258
Compare
|
I removed the Mac support. |
instead of checking every return manually, use a for-loop Signed-off-by: Christoph Ostarek <christoph@zededa.com>
e8bf258 to
0c52706
Compare
jaypipes
left a comment
There was a problem hiding this comment.
sorry @christoph-zededa noticed one thing, see inline comment.
| // Info describes all network interface controllers (NICs) in the host system. | ||
| type Info struct { | ||
| ctx *context.Context | ||
| USBs []*Device `json:"usbs"` |
There was a problem hiding this comment.
Just noticed, this should be Devices not USBs
| // Info describes all network interface controllers (NICs) in the host system. | ||
| type Info struct { | ||
| ctx *context.Context | ||
| Devices []*Device `json:"usbs"` |
There was a problem hiding this comment.
| Devices []*Device `json:"usbs"` | |
| Devices []*Device `json:"devices"` |
There was a problem hiding this comment.
Fixed. Sorry, I am still in Monday mode ;-)
There was a problem hiding this comment.
LOL, no worries @christoph-zededa! Really glad you have been so patient with us in this review! :)
basically this code iterates over entries in `/sys/bus/usb/devices/` and reads out information from the `uevent` file and some other files Signed-off-by: Christoph Ostarek <christoph@zededa.com>
jaypipes
left a comment
There was a problem hiding this comment.
Thank you @christoph-zededa! :)
This PR adds USB devices to the list of devices.
The code basically iterates over
/sys/bus/usb/devices, follows the symlinks and readsuevent,interfaceandproduct.For the snapshot, the symlinks in
/sys/bus/usb/devicesare added as well as the aforementioned files.