jaypipes / ghw

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

expose SRIOV information as PCI devices #315

Open ffromani opened 2 years ago

ffromani commented 2 years ago

Expose informations about SRIOV devices. After some more design iterations (https://github.com/jaypipes/ghw/pull/230#discussion_r755312597), we fixed these goals:

There are few more noteworthy items in this PR:

Fixes: https://github.com/jaypipes/ghw/issues/92

Signed-off-by: Francesco Romani fromani@redhat.com

ffromani commented 2 years ago

this PR is reviewable, but tests are still WIP. The alternate API is pretty much stable though.

jaypipes commented 2 years ago

@fromanirh did you want to rebase this and have it reviewed?

ffromani commented 2 years ago

@fromanirh did you want to rebase this and have it reviewed?

Sure, I'll rebase shortly! other than that this is not very urgent. Up to us if we like more this direction or the original one in #230

ffromani commented 2 years ago

note that most of the tests are missing - deferred until we settle about the direction. Should not be merged without tests, though.

ffromani commented 2 years ago

@fromanirh apologies for the delayed review.

I think I would prefer to have the SRIOV-specific attributes added to the pkg/pci.Device struct itself instead of having an "extended" Function struct that derives from Device.

You could have a helper method on pkg/pci.Info called GetSRIOVDevices() that would return only the PCI devices that were SR-IOV capable. Something like this?

// GetSRIOVDevices returns only the PCI devices that are
// Single Root I/O Virtualization (SR-IOV) capable -- either
// physical of virtual functions.
func (i *Info) GetSRIOVDevices() []*Device {
    res := []*Device{}
    for _, dev := range i.Devices {
        if dev.Parent != nil || len(dev.Functions) != 0 {
            res = append(res, dev)
        }
    }
    return res
}

No worries @jaypipes and thank you for the review. I'm kinda reluctant to conflate even more data in the pci.Device struct (if we do than sooner or later we should also dissolve the gpu pkg into pci - which I'd like! :) ) but I don't feel so strongly to oppose this direction, so let's try and see how it looks.

ffromani commented 1 year ago

@jaypipes eventually I come to like this approach. We may need a couple more iteration to sort out the details of the API (naming, printing) but overall I'm happy to pursue this direction. PR is updated, rebased and ready for review

ffromani commented 1 year ago

depends on https://github.com/jaypipes/ghw/pull/351

ffromani commented 1 year ago

Now that we agreed on the direction, I need to add tests. Will do ASAP.