This repository was archived by the owner on May 12, 2021. It is now read-only.
Consistent device address matching between getDeviceName() and listenToUdevEvents()#849
Merged
Conversation
dgibson
added
the
needs-forward-port
Contributor
Author
|
Not sure if this should get a backport or not. It is, technically, a bug, but the chances of hitting it in practice are pretty small AFAICT. |
Codecov Report
@@ Coverage Diff @@
## master #849 +/- ##
==========================================
+ Coverage 57.85% 57.94% +0.08%
==========================================
Files 17 17
Lines 2373 2373
==========================================
+ Hits 1373 1375 +2
+ Misses 839 838 -1
+ Partials 161 160 -1 |
dgibson
changed the title
getDeviceName() and listenToUdevEvents()getDeviceName() and listenToUdevEvents()
Contributor
Author
|
I don't really understand how my change can be reducing test coverage, since I've removed code and not changed the tests. |
jodh-intel
added
the
needs-backport
jodh-intel
approved these changes
Sep 22, 2020
jodh-intel
left a comment
jodh-intel
left a comment
There was a problem hiding this comment.
Thanks @dgibson.
lgtm
(And yes, we need someone to dig into codecov results as i've seen some oddities like this).
lifupan
approved these changes
Sep 22, 2020
|
/test |
Contributor
Author
Yes. I'm working on it. |
|
/test |
pciDeviceMap is essentially just a map from sysfs paths to corresponding device nodes. The only thing PCI related is that the sysfs paths we're usually looking up in it usually involve PCI addresses. There's no reason that has to be the case, however, and I have future uses which will not be PCI. Therefore, rename it (and change some surrounding comments) to reflect its more general nature. While we're there, fix a typo in a related comment. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
This function watches for uvents that match addresses stored in s.deviceWatchers() and if there's a matching entry signals the associated channel. "Matching" in this instance means soem sort of comparison between the address we're watching for and the sysfs path in the uevent. Currently we match any of a bunch of different cases for different types of device. Being very specific with how we match *sounds* like a good idea, but it's complicated, tightly coupled to other parts of the code and, ultimately, pointless. The user of the data we're collecting here is getDeviceName(). Before registering a watcher it checks s.sysToDevMap in case the uevent it's interested in has already happened. To have consistent behaviour regardless of when the uevent arrives, the matching that getDeviceName() uses against s.sysToDevMap must be the same as we use in listenToUdevEvents() and right now, it's not. So, change the matching in listenToUdevEvents() to be the same as the much simpler version in getDeviceName(). fixes #848 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Contributor
Author
|
I'm fine either way. These patches are essentially a prerequisite for #850 , so I don't mind if they go in as two PRs or one. |
Contributor
Author
|
/test-ubuntu |
Contributor
Author
|
/test |
Contributor
Author
|
Hmmm the CI says I don't think that's my fault... |
|
@dgibson - hahaa! That's a pretty brutal tear down script eh? 😄 @GabyCT - That But I think the main problem is the previous error: |
|
/test-centos |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
This is a fix for issue #848. While we're there, simplify the code a bit and rename a poorly named data structure.