interfaces: fix udev tagging for hooks #4144

zyga · Nov 3, 2017

This branch aims to fix udev tagging to support hooks in addition to apps that
were added earlier.

There are three main patches here:

add spec.TagDevice method to udev.Specification, this automatically tags
both apps and hooks
switch non-common interfaces to TagDevice, with exception of some special
uses of AddSnippet
switch common interfaces to TagDevice, with some trivial changes to how
that is defined.

The only remaining uses of spec.AddSnippet are special-cases that add large
chunk of arbitrary udev code that is not specifically aiming to tag a device.

This passes local unit testing, no testing was done on any hardware.

Base on initial feedback I also included changes to format udev rules
differently. They are now preceded by the snapd interface name (as a comment).
The order of entries is also different, now being sorted by TAG += if one
exists (this allows us to see everything affecting particular command easily)

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

codecov-io · Nov 3, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@7273bc2). Click here to learn what that means.
The diff coverage is 92.64%.

@@            Coverage Diff            @@
##             master    #4144   +/-   ##
=========================================
  Coverage          ?   75.71%           
=========================================
  Files             ?      437           
  Lines             ?    37856           
  Branches          ?        0           
=========================================
  Hits              ?    28663           
  Misses            ?     7192           
  Partials          ?     2001

Impacted Files	Coverage Δ
interfaces/builtin/utils.go	`100% <ø> (ø)`
interfaces/builtin/ofono.go	`75.51% <ø> (ø)`
interfaces/udev/spec.go	`95.18% <100%> (ø)`
interfaces/builtin/serial_port.go	`67.17% <80%> (ø)`
interfaces/builtin/hidraw.go	`65.21% <87.5%> (ø)`

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 7273bc2...f714b95. Read the comment docs.

stolowski · Nov 6, 2017

stolowski reviewed Nov 6, 2017

View changes

Looks good, much better than the old code. Just two minor comments.

Nice.

I'm not totally sure it's more readable than calling udevUsbDeviceSnippet helper here to be honest. Anyway, not biggie but if you decide not to use udevUsbDeviceSnippet, then perhaps udevUsbDeviceSnippet can be removed from utils?

I agree, either use udevUsbDeviceSnippet where you can or remove it from utils.go.

I dropped it entirely now. I think the result is much more readable.

Could you please extend this test to have call spec.TagDevice twice?

jdstrand · Nov 7, 2017

jdstrand requested changes Nov 7, 2017

View changes

I blackbox tested this by connecting all the (non-gadget) interfaces and found that the udev tagging rules are fine. On the one hand, the sorting of the rules is nice since the left hand side (non-TAG) part is sorted, but on the other hand, the TAGs are not grouped by command. I think it would be good to group by snap command and then by rule (eg, each command is in a stanza with stanzas separated by new lines). While this sounds cosmetic, it helps with debugging and for making policy audits easier.

I'm not totally sure it's more readable than calling udevUsbDeviceSnippet helper here to be honest. Anyway, not biggie but if you decide not to use udevUsbDeviceSnippet, then perhaps udevUsbDeviceSnippet can be removed from utils?

I agree, either use udevUsbDeviceSnippet where you can or remove it from utils.go.

I dropped it entirely now. I think the result is much more readable.

Oh, this tun[0-9]* tag is actually an error. I realize this PR didn't introduce this, but we should fix it. See #4031

Dropped

zyga · Nov 8, 2017

The udev rules cannot contain trailing comments:

foo # comment

This is not allowed. I'll update the PR after lunch.

jdstrand · Nov 8, 2017

jdstrand approved these changes Nov 8, 2017

View changes

Since you fixed the ofono issue and added the comment commit, I'm going to mark this as approved. I do think that it would be nice to group by command, but we didn't have that before so I don't think it should be a blocker for this PR.

stolowski · Nov 9, 2017

stolowski approved these changes Nov 9, 2017

View changes

Looking good.

zyga · Nov 10, 2017

Just for the record, this does contain the formatting changes that were requested. Jamie just didn't see it when he made that comment above. We discussed this on IRC yesterday.

pedronis · Nov 10, 2017

@zyga are the comments in #4146 (review) addressed here?

zyga · Nov 10, 2017

@pedronis yes (now)

This was referenced Nov 3, 2017

Closed

interfaces: fix udev tagging for hooks (2.29) #4145

Merged

interfaces: fix udev tagging for hooks (2.29) #4146

zyga requested review from pedronis, stolowski and mvo5 Nov 3, 2017

zyga and others added some commits Nov 10, 2017

Merge branch 'master' of github.com:snapcore/snapd into fix/udev-hooks

4aa5220

Merge remote-tracking branch 'upstream/master' into HEAD

9922217

zyga merged commit 0dbad11 into snapcore:master Nov 10, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

zyga deleted the zyga:fix/udev-hooks branch Nov 10, 2017

-        -			spec.AddSnippet(snippet)
-        -		}
+        +	for _, rule := range iface.connectedPlugUDev {
+        +		spec.TagDevice(rule)

+        +		spec.TagDevice(fmt.Sprintf(`SUBSYSTEM=="hidraw", KERNEL=="%s"`, strings.TrimPrefix(path, "/dev/")))
+        +	} else {
+        +		spec.TagDevice(fmt.Sprintf(`IMPORT{builtin}="usb_id"
+        +SUBSYSTEM=="hidraw", SUBSYSTEMS=="usb", ATTRS{idVendor}=="%04x", ATTRS{idProduct}=="%04x"`, usbVendor, usbProduct))

+        +	iface := &ifacetest.TestInterface{
+        +		InterfaceName: "test",
+        +		UDevConnectedPlugCallback: func(spec *udev.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
+        +			spec.TagDevice(`kernel="voodoo"`)

+        +		spec.TagDevice(fmt.Sprintf(`SUBSYSTEM=="hidraw", KERNEL=="%s"`, strings.TrimPrefix(path, "/dev/")))
+        +	} else {
+        +		spec.TagDevice(fmt.Sprintf(`IMPORT{builtin}="usb_id"
+        +SUBSYSTEM=="hidraw", SUBSYSTEMS=="usb", ATTRS{idVendor}=="%04x", ATTRS{idProduct}=="%04x"`, usbVendor, usbProduct))

+        +	*/
+        +	spec.TagDevice(`KERNEL=="tty[A-Z]*[0-9]*|cdc-wdm[0-9]*"`)
+        +	spec.TagDevice(`KERNEL=="tun"`)
+        +	spec.TagDevice(`KERNEL=="tun[0-9]*"`)

snapcore/snapd

interfaces: fix udev tagging for hooks #4144

zyga commented Nov 3, 2017 •

Edited 2 times

zyga
Nov 8, 2017

zyga
Nov 8, 2017

zyga added some commits Nov 3, 2017

This was referenced Nov 3, 2017

interfaces: fix udev tagging for hooks (2.29) #4145

interfaces: fix udev tagging for hooks (2.29) #4146

zyga requested review from pedronis, stolowski and mvo5 Nov 3, 2017

codecov-io commented Nov 3, 2017 •

Edited 2 times

codecov-io
Nov 10, 2017

codecov-io
Nov 7, 2017

stolowski reviewed Nov 6, 2017

View changes

jdstrand requested changes Nov 7, 2017

View changes

zyga added some commits Nov 8, 2017

zyga commented Nov 8, 2017

zyga added some commits Nov 8, 2017

jdstrand approved these changes Nov 8, 2017

View changes

stolowski approved these changes Nov 9, 2017

View changes

zyga commented Nov 10, 2017

zyga and others added some commits Nov 10, 2017

pedronis commented Nov 10, 2017

zyga and others added some commits Nov 10, 2017

zyga commented Nov 10, 2017

zyga merged commit `0dbad11` into snapcore:master Nov 10, 2017
1 check passed

1 check passed

zyga deleted the zyga:fix/udev-hooks branch Nov 10, 2017

snapcore/snapd

Join GitHub today

interfaces: fix udev tagging for hooks #4144

Conversation

zyga commented Nov 3, 2017 • edited Edited 2 times zyga Nov 8, 2017 zyga Nov 8, 2017

zyga added some commits Nov 3, 2017

Some checks were not successful

This was referenced Nov 3, 2017

zyga requested review from pedronis, stolowski and mvo5 Nov 3, 2017

codecov-io commented Nov 3, 2017 • edited Edited 2 times codecov-io Nov 10, 2017 codecov-io Nov 7, 2017

Codecov Report

stolowski reviewed Nov 6, 2017 View changes

Some checks were not successful

jdstrand requested changes Nov 7, 2017 View changes

zyga added some commits Nov 8, 2017

Some checks were not successful

zyga commented Nov 8, 2017

zyga added some commits Nov 8, 2017

jdstrand approved these changes Nov 8, 2017 View changes

stolowski approved these changes Nov 9, 2017 View changes

zyga commented Nov 10, 2017

zyga and others added some commits Nov 10, 2017

pedronis commented Nov 10, 2017

zyga and others added some commits Nov 10, 2017

zyga commented Nov 10, 2017

Hide details View details zyga merged commit 0dbad11 into snapcore:master Nov 10, 2017 1 check passed

1 check passed

zyga deleted the zyga:fix/udev-hooks branch Nov 10, 2017

zyga commented Nov 3, 2017 •

Edited 2 times

zyga
Nov 8, 2017

zyga
Nov 8, 2017

codecov-io commented Nov 3, 2017 •

Edited 2 times

codecov-io
Nov 10, 2017

codecov-io
Nov 7, 2017

stolowski reviewed Nov 6, 2017

View changes

jdstrand requested changes Nov 7, 2017

View changes

jdstrand approved these changes Nov 8, 2017

View changes

stolowski approved these changes Nov 9, 2017

View changes

zyga merged commit `0dbad11` into snapcore:master Nov 10, 2017
1 check passed