pci: add and use LookupDevice#240
Merged
Merged
Conversation
jaypipes
requested changes
Apr 12, 2021
jaypipes
left a comment
jaypipes
left a comment
Owner
There was a problem hiding this comment.
See inline comments @fromanirh; I'm concerned about introducing another method instead of just modifying the implementation of the existing PCIInfo.GetDevice method.
|
|
||
| The difference between the two methods is that `LookupDevice` will use | ||
| cached data, while `GetDevice` will attempt every time to get informations | ||
| from the system. |
Owner
There was a problem hiding this comment.
The caching behaviour of any ghw struct should be entirely transparent to the user, so I'm going to push back on this particular change. The user should not need to know the internal implementation details in order to know to call a different method of ghw.PCIInfo depending on whether an underlying sysfs/filesystem mount is present.
In other words, just change the PCIInfo.GetDevice method implementation to look up the device in PCIInfo.Devices by address before searching a filesystem :)
Comment on lines
+178
to
+181
| for _, dev := range info.Devices { | ||
| if dev.Address == address { | ||
| return dev | ||
| } | ||
| } | ||
| return nil |
Add a simple helper to find a device among the `pci.Info.Devices`.
This is needed because `GetDevice` accesses the system every time.
In most cases, besides a tiny performance hit, this is no issue.
When consuming snapshots, however, this may lead to surprising behaviour
because the caller need to ensure the snapshot is available when
`GetDevice` is called, or it will return unpredictable result (!!!).
The wrong code looks like that:
1. PCI info is created supplying snapshot data with automatic management
1.a ghw unpacks the snapshot on a temporary directory.
1.b ghw loads all the info from the snapshot. info.Devices contains
correct data.
1.c ghw cleans up the snapshot (see:
jaypipes#236)
2. the client (test) code calls GetDevice
2.a GetDevice will try to access the unpacked snapshot data, which is
gone.
2.b GetDevice will fail.
Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
force-pushed
the
pci-lookup-device
branch
from
c31cda6 to
c126558
Compare
ffromani
added a commit
to ffromani/performance-addon-operators
that referenced
this pull request
Apr 19, 2021
update to the tip of ghe main branch to pull in the latest fixes, most notably to avoid to leak temporary directories when consuming snapshots: jaypipes/ghw#236 jaypipes/ghw#240 Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
added a commit
to ffromani/performance-addon-operators
that referenced
this pull request
Apr 20, 2021
update to the tip of ghe main branch to pull in the latest fixes, most notably to avoid to leak temporary directories when consuming snapshots: jaypipes/ghw#236 jaypipes/ghw#240 Signed-off-by: Francesco Romani <fromani@redhat.com>
ffromani
added a commit
to ffromani/performance-addon-operators
that referenced
this pull request
Apr 29, 2021
update to the tip of ghe main branch to pull in the latest fixes, most notably to avoid to leak temporary directories when consuming snapshots: jaypipes/ghw#236 jaypipes/ghw#240 Signed-off-by: Francesco Romani <fromani@redhat.com>
cynepco3hahue
pushed a commit
to openshift-kni/performance-addon-operators
that referenced
this pull request
Jun 3, 2021
update to the tip of ghe main branch to pull in the latest fixes, most notably to avoid to leak temporary directories when consuming snapshots: jaypipes/ghw#236 jaypipes/ghw#240 Signed-off-by: Francesco Romani <fromani@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a simple helper to find a device among the
pci.Info.Devices.Up until now, client code had to reimplement the find function
every time, or to call
GetDevice. Problem is:GetDeviceaccess the system every time.In most cases, besides a tiny performance hit, this is no issue.
When consuming snapshots, however, this may lead to surprising behaviour
because the caller need to ensure the snapshot is available when
GetDeviceis called, or it will return unpredictable result (!!!).The wrong code looks like that:
1.a ghw unpacks the snapshot on a temporary dir
1.b ghw loads all the info from the snapshot. info.Devices contains
correct data
1.c ghw cleans up the snapshot (see:
context: Fix directory leakage for snapshots contexts #236)
2.a GetDevice will try to access the unpacked snapshot data, which is
gone
2.b GetDevice will fail
Signed-off-by: Francesco Romani fromani@redhat.com