interfaces/hotplug: add hotplug Specification and HotplugDeviceInfo #5416

stolowski · 2018-06-27T11:36:12Z

Introduce two structures related to hotplug: Specification will be passed to interfaces that implement HotplugDeviceDetected(..) method, it's a way for interfaces to add slot definitions on hot plug add event. HotplugDeviceInfo wraps some uevent data, it will be passed from udev monitor to interface manager callback on hotplug add/remove events.

zyga · 2018-06-27T12:45:10Z

This is missing some stuff

src/github.com/snapcore/snapd/interfaces/hotplug/spec.go:25:2: cannot find package "github.com/snapcore/snapd/interfaces/utils" in any of:
	/home/gopath/src/github.com/snapcore/snapd/_build/src/github.com/snapcore/snapd/vendor/github.com/snapcore/snapd/interfaces/utils (vendor tree)

zyga

First pass, -1 because of the issue with make, please ping me for 2nd pass.

zyga · 2018-06-27T12:50:52Z

dirs/dirs.go

@@ -106,6 +106,7 @@ var (
 	SnapshotsDir     string

 	ErrtrackerDbDir string
+	SysDir          string


Nitpick, to avoid any shade of confusion could you please call this SysfsDir instead?

zyga · 2018-06-27T12:52:09Z

interfaces/hotplug/deviceinfo.go

+	"github.com/snapcore/snapd/dirs"
+)
+
+type HotplugDeviceInfo struct {


Please document this somehow.

HotplugDeviceInfo stores information about devices added dynamically at runtime.

zyga · 2018-06-27T12:52:32Z

interfaces/hotplug/deviceinfo.go

+type HotplugDeviceInfo struct {
+	object       string
+	Data         map[string]string
+	idVendor     string


I'm somewhat -0.5 on keeping those explicit. Instead we could have methods that fish them out of Data

Note, these attributes are not present in Data, they were read lazily on first access from the filesystem. However, removed all the idVendor/idProduct/serial aspects for now per our discussion.

zyga · 2018-06-27T12:52:53Z

interfaces/hotplug/deviceinfo.go

+)
+
+type HotplugDeviceInfo struct {
+	object       string


Please document what you mean by object here. Is this is a sysfs path?

Added a comment to Object() getter.

zyga · 2018-06-27T12:53:05Z

interfaces/hotplug/deviceinfo.go

+	serial       string
+}
+
+func NewHotplugDeviceInfo(obj string, env map[string]string) *HotplugDeviceInfo {


Please add docs for everything.

zyga · 2018-06-27T12:57:21Z

interfaces/hotplug/deviceinfo.go

+	HotplugDeviceDetected(di *HotplugDeviceInfo, spec *Specification) error
+}
+
+// HotplugDeviceInfo can be implemented by interfaces that need to provide a non-standard device key for hotplug devices


Can you add a chunk of text right at the top of the file that documents the key concepts of hot plug in snapd. Please specifically document the notion of device key.

zyga · 2018-06-27T12:58:20Z

interfaces/hotplug/spec.go

+	}
+}
+
+func (h *Specification) AddSlot(name, label string, attrs map[string]interface{}) error {


Can this take a SlotInfo instead (and ensure some things are not wired, i.e. no snap.Info). This is more future proof than a function with three arguments.

It could, my only concern is there is actually more stuff in SlotInfo that should not be touched/initialized by the interface implementation. We can of course make sure it's not wired (error out or ignore/reset snap.Info, hooks, apps etc.) but it feels like somewhat confusing API. How about something like:
AddSlot(slot SlotSpec)
so this is stable and future-proof, SlotSpec has only the attributes/setters that are relevant and we have clear way of mediating.

zyga · 2018-06-27T12:58:59Z

interfaces/hotplug/spec.go

+	"github.com/snapcore/snapd/interfaces/utils"
+)
+
+type SlotSpec struct {


Document public things please. This helps you and your reviewers alike.

zyga · 2018-06-27T12:59:51Z

interfaces/hotplug/spec.go

+}
+
+type Specification struct {
+	slots map[string]SlotSpec


I would make this map[string]*SlotSpec as there are maps inside SlotSpec and making all of this by-value feels bad.

zyga · 2018-06-27T13:00:29Z

interfaces/hotplug/spec.go

+}
+
+func (h *Specification) Slots() []SlotSpec {
+	slots := make([]SlotSpec, len(h.slots))


This is wrong, you meant make([]SlotSpec, 0, len(h.slots))

Probably implies this is not tested.

bboozzoo

@zyga covered the rest

bboozzoo · 2018-06-28T09:45:35Z

interfaces/hotplug/deviceinfo.go

+
+func (h *HotplugDeviceInfo) readOnceMaybe(fileName string, out *string) string {
+	if *out == "" {
+		data, err := ioutil.ReadFile(filepath.Join(h.Path(), fileName))


This could be converted to a public ReadAttribute() (string, error) helper

codecov-io · 2018-06-29T09:27:50Z

Codecov Report

Merging #5416 into master will increase coverage by 0.08%.
The diff coverage is 74.64%.

@@            Coverage Diff             @@
##           master    #5416      +/-   ##
==========================================
+ Coverage   78.92%   79.01%   +0.08%     
==========================================
  Files         509      512       +3     
  Lines       38445    38672     +227     
==========================================
+ Hits        30343    30555     +212     
- Misses       5655     5664       +9     
- Partials     2447     2453       +6

Impacted Files	Coverage Δ
interfaces/utils/utils.go	`0% <0%> (ø)`	⬆️
dirs/dirs.go	`97.8% <100%> (+0.02%)`	⬆️
interfaces/connection.go	`82.52% <100%> (-2.23%)`	⬇️
interfaces/hotplug/deviceinfo.go	`100% <100%> (ø)`
interfaces/hotplug/spec.go	`88.88% <88.88%> (ø)`
interfaces/policy/policy.go	`93.6% <0%> (-3.21%)`	⬇️
snap/snaptest/snaptest.go	`66.11% <0%> (-2.85%)`	⬇️
store/errors.go	`77.17% <0%> (-1.31%)`	⬇️
overlord/hookstate/hookmgr.go	`74.27% <0%> (-0.98%)`	⬇️
client/client.go	`80% <0%> (ø)`	⬆️
... and 9 more

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 34562bc...237c82b. Read the comment docs.

zyga

One more pass with some comments. I think this needs some more tweaks before landing (at least in my eyes)

zyga · 2018-06-29T12:41:45Z

interfaces/hotplug/deviceinfo.go

+
+// HotplugDeviceInfo carries information about added/removed device detected at runtime.
+type HotplugDeviceInfo struct {
+	object string


Can you just call this devpath please, here and everywhere in the hot plug code. My desire here is to limit the vocabulary used so that we don't invent names for existing kernel constructs.

Hmmm, I could, however this corresponds to KObj (kernel object?) value associated with the udev event reported by go-udev (what I get from go-udev is kobj string and map of extra data such as DEVPATH etc). While there seems to be direct relation between kobj and DEVPATH, I'm not 100% sure it's always the case, so perhaps it's ok to leave the doors open in case we find both devpath and object values useful?

Either devpath and kobjsomething are better than object which is as generic as one can get. Whatever you do please explain what the value is in a comment.

zyga · 2018-06-29T12:48:42Z

interfaces/hotplug/deviceinfo.go

+}
+
+// Returns the value of "SUBSYSTEM" attribute of the udev event associated with the device, e.g. "usb".
+func (h *HotplugDeviceInfo) Subsystem() string {


I think it is worth to indicate that SUBSYSTEM is always present. while pretty much everything else is entirely optional.

Plugging a device in results in multiple add events with generic events first, followed by more concrete ones where you actually got more interesting attributes present. I expect interfaces to filter out (ignore) some events and only react on those that actually cary all important attributes.
Point taken thouhg, I added comments to all attributes.

zyga · 2018-06-29T12:49:49Z

interfaces/hotplug/deviceinfo.go

+	return h.Data["SUBSYSTEM"]
+}
+
+// Returns the value of "MINOR" attribute of the udev event associated with the device.


Please indicate that this and everything below is optional and may return an empty string.
I also have a gut feeling Data should be private but we can see how this works.

EDIT: I have a feeling we should just expose an attribute getter and not have APIs for things that are optional (and by looking at udev database, mostly absent).

Added comments. Made data private and added a generic attribute getter. I'm not sure about removing getters for optional attributes, I think these are actually the attributes that are the most interesting for interfaces (even if optional), so having exposed helpers for them feels handy and I'd personally prefer to read di.Minor() than di.Attribute("MINOR").

zyga · 2018-06-29T12:50:09Z

interfaces/hotplug/deviceinfo_test.go

+	}
+
+	di := NewHotplugDeviceInfo("/devices/pci0000:00/0000:00:14.0/usb2/2-3", env)
+


Please add a test with all of the optional things absent.

zyga · 2018-06-29T12:51:14Z

interfaces/hotplug/spec.go

+}
+
+type Specification struct {
+	slots map[string]*SlotSpec


Please document what the key in the map is. Is it DEVPATH?

Documented.

zyga · 2018-06-29T12:52:03Z

interfaces/hotplug/spec.go

+	}
+}
+
+func (h *Specification) AddSlot(slotSpec *SlotSpec) error {


This must absolutely associate with a sysfs device. How is this being ensured?

This is going to happen outside in the hotplug event processing loop. Hotplug event handler will iterate over all hotplug interfaces, determine device key, create a Specification per-device. Interface fills in the Specification by adding slot(s), the outher code knows device key for given set of slots.

Can you add this response as a comment please.

bboozzoo · 2018-07-04T15:14:47Z

interfaces/hotplug/spec_test.go

+func (s *hotplugSpecSuite) TestAddSlot(c *C) {
+	spec := NewSpecification()
+	c.Assert(spec.AddSlot(&SlotSpec{Name: "slot1", Label: "A slot", Attrs: map[string]interface{}{"foo": "bar"}}), IsNil)
+	c.Assert(spec.AddSlot(&SlotSpec{Name: "slot2", Label: "A slot", Attrs: map[string]interface{}{"baz": "booze"}}), IsNil)


foo bar froz bot AFAIK ;-)

bboozzoo · 2018-07-04T15:15:07Z

interfaces/hotplug/spec.go

+	for k := range h.slots {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)


Do these need to be sorted or is it just to get a stable output for the tests?

It's about making it test friendly only.

bboozzoo · 2018-07-04T15:16:01Z

interfaces/hotplug/deviceinfo.go

+// Returns full device path under /sysfs, e.g /sys/devices/pci0000:00/0000:00:14.0/usb1/1-2.
+// The path is derived from DEVPATH attribute of the udev event.
+func (h *HotplugDeviceInfo) Path() string {
+	path, ok := h.Attribute("DEVPATH")


The check doesn't do any harm, but Isn't DEVPATH one of the attributes that's supposed to be there always?

Why do we have Path and Object as separate functions if they must always return the exact same value?

zyga

One more pass, let's chat on IRC

zyga · 2018-07-05T07:44:15Z

interfaces/hotplug/deviceinfo.go

+
+// Returns object path, i.e. the sysfs path of the device, e.g. /devices/pci0000:00/0000:00:14.0/usb1/1-2.
+// It is expected to be equal to DEVPATH attribute.
+func (h *HotplugDeviceInfo) Object() string {


I really think this should be just DevPath or Devpath and that we should not use the un-decorated word object.

Ok, removed object (and Object getter) alltogether for now, per IRC discussion.

zyga · 2018-07-05T07:48:09Z

interfaces/hotplug/deviceinfo.go

+// Returns full device path under /sysfs, e.g /sys/devices/pci0000:00/0000:00:14.0/usb1/1-2.
+// The path is derived from DEVPATH attribute of the udev event.
+func (h *HotplugDeviceInfo) Path() string {
+	path, ok := h.Attribute("DEVPATH")


Why do we have Path and Object as separate functions if they must always return the exact same value?

zyga · 2018-07-05T07:49:04Z

interfaces/utils/utils.go

+var validName = regexp.MustCompile("^[a-z](?:-?[a-z0-9])*$")
+
+// ValidateName checks if a string can be used as a plug or slot name.
+func ValidateName(name string) error {


Please drop this and look at snap.Validate{Plug,Slot}Name instead.

Great idea, thanks. Done.

zyga

LGTM with one last comment

zyga · 2018-07-05T10:21:02Z

interfaces/hotplug/deviceinfo.go

+
+// NewHotplugDeviceInfo creates HotplugDeviceInfo structure related to udev add or remove event.
+func NewHotplugDeviceInfo(env map[string]string) *HotplugDeviceInfo {
+	return &HotplugDeviceInfo{


As a sanity check this should ensure that env contains DEVPATH.

Added hotplug Specification and HotplugDeviceInfo.

275f7ad

zyga requested changes Jun 27, 2018

View reviewed changes

Merge branch 'master' into hotplug-device-info

280ed48

bboozzoo reviewed Jun 28, 2018

View reviewed changes

stolowski added 7 commits June 28, 2018 12:22

Merge branch 'master' into hotplug-device-info

8163720

Review feedback.

1edbf70

Removed type HotplugDeviceHandler type definition for now.

9a5f9da

Updated HotPlugSpec.

63b33d0

Merge branch 'master' into hotplug-device-info

07adf32

Merge branch 'master' into hotplug-device-info

7dd7efe

Merge branch 'master' into hotplug-device-info

e408116

Merge branch 'master' into hotplug-device-info

e3adc77

stolowski mentioned this pull request Jun 29, 2018

interfaces/repo: added AllHotplugInterfaces helper #5433

Merged

stolowski added 2 commits June 29, 2018 14:19

Merge branch 'master' into hotplug-device-info

23e8ec6

Sort slots.

06eb95c

zyga reviewed Jun 29, 2018

View reviewed changes

stolowski added 7 commits June 29, 2018 16:03

Review comments.

4c637dd

More comments.

4c23113

Made data private, added generic Attribute getter.

ee9c8b6

Added Definer type.

dc6a005

Merge branch 'master' into hotplug-device-info

4bff39a

Merge branch 'master' into hotplug-device-info

7ceb603

Validate interface name in the hotplug spec.

9c027cf

bboozzoo approved these changes Jul 4, 2018

View reviewed changes

zyga reviewed Jul 5, 2018

View reviewed changes

stolowski added 2 commits July 5, 2018 12:12

Drop Object attribute from HotplugDeviceInfo. Use snap.ValidateSlotName.

8d1b22d

Reverted the addition of ValidateName.

00276aa

zyga approved these changes Jul 5, 2018

View reviewed changes

Check that DEVPATH exists.

237c82b

stolowski merged commit 7982e02 into canonical:master Jul 5, 2018

		}

		di := NewHotplugDeviceInfo("/devices/pci0000:00/0000:00:14.0/usb2/2-3", env)

interfaces/hotplug: add hotplug Specification and HotplugDeviceInfo #5416

interfaces/hotplug: add hotplug Specification and HotplugDeviceInfo #5416

Conversation

stolowski commented Jun 27, 2018

zyga commented Jun 27, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 29, 2018 • edited Loading

Codecov Report

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski Jun 28, 2018 •

edited

Loading

codecov-io commented Jun 29, 2018 •

edited

Loading