jaypipes / ghw

Go HardWare discovery/inspection library
Apache License 2.0
1.62k stars 174 forks source link

Fix class name lookups #327

Closed eliaskoromilas closed 1 year ago

eliaskoromilas commented 1 year ago

All class IDs in https://pci-ids.ucw.cz/v2.2/pci.ids are stored as lowercase hex. The modalias file is used to extract the class IDs for a given device. The class IDs in that file can be uppercase HEX, e.g. pci:v00008086d00009D23sv00001028sd000007EAbc0Csc05i00. PCI DB lookup fails in that case.

- What I did

- How I did it

ffromani commented 1 year ago

Hi @eliaskoromilas thanks for this contribution. The change looks good to me and the rationale fully makes sense. Do we have a unit test which highlights this behavior, and, if not, could you please add it? I want to verify one last thing, which is the lowercased identifer don't leak elsewhere in the codebase, so we don't have unintended side effects.

eliaskoromilas commented 1 year ago

Do we have a unit test which highlights this behavior, and, if not, could you please add it?

In your current test data there are no problematic modalias files. I could add some real modalias file contents (with upper-case class IDs), and verify that are now parsed correctly.

I want to verify one last thing, which is the lowercased identifer don't leak elsewhere in the codebase, so we don't have unintended side effects.

It affects nothing more than Class/Subclass/ProgrammingInterface struct IDs and Names. All of which are purely informational.

eliaskoromilas commented 1 year ago

@fromanirh Unit test is included.

ffromani commented 1 year ago

thanks for your updates @eliaskoromilas . I will review as soon as possible.

eliaskoromilas commented 1 year ago

thanks for your updates @eliaskoromilas . I will review as soon as possible.

@fromanirh The MacOS build failure seems to be unrelated. Could we bring this in?

ffromani commented 1 year ago

thanks for your updates @eliaskoromilas . I will review as soon as possible.

@fromanirh The MacOS build failure seems to be unrelated. Could we bring this in?

yes, it seems like a glitch. I'll keep an eye on the lane anyway. Sorry for the the delay

ffromani commented 1 year ago

for the record: the pci.ids database (https://pci-ids.ucw.cz/read/PD/) use lowercase letters. This is further confirmation that, when in doubt, converting to lowercase is sensible.

ffromani commented 1 year ago

I'm a bit surprised the official PCI-SIG docs all seems to use the uppercase letters - at least the couple I glanced over. but still.

eliaskoromilas commented 1 year ago

We just care about how they are stored in the pci.ids file. They are all hex numbers and should be case insensitive.