Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ifacestate/udevmonitor: added callback to signal end of enumeration #6072

Merged
merged 8 commits into from Nov 9, 2018

Conversation

stolowski
Copy link
Contributor

This PR adds a callback to signal the end of initial enumeration of devices (via udevadm) to interface manager. This is required to know when to compute the "delta" between currently present devices and the past (not in this PR).
Also made the two existing ifacemgr hotplug callbacks private as they don't need to be exported.

@stolowski stolowski added Hotplug 🔌 Simple 😃 A small PR which can be reviewed quickly labels Oct 30, 2018
@codecov-io
Copy link

Codecov Report

Merging #6072 into master will increase coverage by <.01%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6072      +/-   ##
==========================================
+ Coverage   78.97%   78.98%   +<.01%     
==========================================
  Files         546      546              
  Lines       42782    42792      +10     
==========================================
+ Hits        33789    33798       +9     
- Misses       6248     6249       +1     
  Partials     2745     2745
Impacted Files Coverage Δ
overlord/ifacestate/hotplug.go 95.16% <0%> (-1.57%) ⬇️
overlord/ifacestate/ifacemgr.go 85.71% <100%> (ø) ⬆️
overlord/ifacestate/udevmonitor/udevmon.go 77.27% <100%> (+2.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71c69f7...d0869b0. Read the comment docs.

@@ -38,13 +38,15 @@ type Interface interface {

type DeviceAddedFunc func(device *hotplug.HotplugDeviceInfo)
type DeviceRemovedFunc func(device *hotplug.HotplugDeviceInfo)
type EnumerationDoneFunc func()

// Monitor monitors kernel uevents making it possible to find USB devices.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW. this should probably say hotpluggable devices, there are more interfaces supporting hotplug than just USB.

m.monitorStop = m.netlinkConn.Monitor(m.netlinkEvents, m.netlinkErrors, nil)
var filter netlink.Matcher
// TODO: extend with other criteria based on the hotplug interfaces
filter = &netlink.RuleDefinitions{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a part of a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, removed.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, once suggestion inline.

// TODO: consider passing a device filter to reduce noise from irrelevant devices.
m.monitorStop = m.netlinkConn.Monitor(m.netlinkEvents, m.netlinkErrors, nil)
var filter netlink.Matcher
// TODO: extend with other criteria based on the hotplug interfaces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly unrelated to this PR it seem(?). Its ok but I couldn't help noticing. I guess because this goes directly to netlinkConn.Monitor() we cannot add a test that the subsystem matching does really work(?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this should be part of another PR, removed.

@@ -60,6 +61,15 @@ func (s *udevMonitorSuite) TestDiscovery(c *C) {
remInfo = inf
callbackChannel <- struct{}{}
}
done := func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nitpick) would be nice to call this "enumerateDone" - when I looked at this I though we could change the code here to only look at done but this test also simulates hotplugs after the enumeration was done. So a slightly different name would probably have given me the extra context :)

@stolowski stolowski merged commit ef5171b into snapcore:master Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
5 participants