Merge lp:~c-lobrano/snappy/hw-assign-fix-overwrite-udev-rule into lp:~snappy-dev/snappy/snappy-moved-to-github

[Carlo Lobrano]

Description of the changes:

- hw-assign now appends each new udev rule for snap to the existing file, if any, in place of overwriting the same file
- hw-unassign now removes only the single udev rule related to the device unassigned, in place of just deleting the file

Proposal fix for Bug #1497299

Please let me know if this fix can be improved somehow or if I misinterpreted something.

Best regards,
Carlo Lobrano

[John Lenton (chipaca)]

Thank you for this!

There's one thing that needs addressing, inline below. Also some stylistic nits for you to consider.

review: Needs Fixing

[John Lenton (chipaca)]

Better!

Still two problems in the code. One in a comment below. The other is from golint:

snappy/hwaccess.go:119:35: don't use underscores in Go names; func parameter new_rule should be newRule
snappy/hwaccess.go:135:2: don't use underscores in Go names; var updated_rules should be updatedRules
snappy/hwaccess.go:238:3: don't use underscores in Go names; var device_pattern should be devicePattern

look at .precommit in the project root for help in having golint run automatically for you.

[John Lenton (chipaca)]

Good. Thank you!

review: Approve

[Snappy Tarmac (snappydevtarmac)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file '.bzrignore'
--- .bzrignore	2015-08-25 16:36:01 +0000
+++ .bzrignore	2015-09-25 15:00:57 +0000
@@ -1,2 +1,5 @@
 _integration-tests/bin/
 _integration-tests/data/output/
+./share
+./snappy/hello_1.0.1_all.snap
+./tags

=== modified file 'snappy/hwaccess.go'
--- snappy/hwaccess.go	2015-09-15 12:55:09 +0000
+++ snappy/hwaccess.go	2015-09-25 15:00:57 +0000
@@ -20,6 +20,7 @@
 package snappy
 
 import (
+	"bufio"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -115,6 +116,26 @@
 	return filepath.Join(snapUdevRulesDir, fmt.Sprintf("70-snappy_hwassign_%s.rules", partid))
 }
 
+func addUdevRuleForSnap(snapname, newRule string) error {
+	udevRulesFile := udevRulesPathForPart(snapname)
+
+	rules, err := ioutil.ReadFile(udevRulesFile)
+	if nil != err && !os.IsNotExist(err) {
+		return err
+	}
+
+	// At this point either rules variable contains some rules if the
+	// file exists, or it is nil if the file does not exist yet.
+	// In both cases, updatedRules will have the right content.
+	updatedRules := append(rules, newRule...)
+
+	if err := helpers.AtomicWriteFile(udevRulesFile, updatedRules, 0644); nil != err {
+		return err
+	}
+
+	return nil
+}
+
 func writeUdevRuleForDeviceCgroup(snapname, device string) error {
 	os.MkdirAll(snapUdevRulesDir, 0755)
 
@@ -130,7 +151,7 @@
 KERNEL=="%v", TAG:="snappy-assign", ENV{SNAPPY_APP}:="%s"
 `, filepath.Base(device), snapname)
 
-	if err := ioutil.WriteFile(udevRulesPathForPart(snapname), []byte(acl), 0644); err != nil {
+	if err := addUdevRuleForSnap(snapname, acl); err != nil {
 		return err
 	}
 
@@ -197,6 +218,49 @@
 	return appArmorAdditional.WritePath, nil
 }
 
+func removeUdevRuleForSnap(snapname, device string) error {
+	udevRulesFile := udevRulesPathForPart(snapname)
+
+	file, err := os.Open(udevRulesFile)
+	if nil != err && !os.IsNotExist(err) {
+		return err
+	}
+
+	// Get the full list of rules to keep
+	var rulesToKeep []string
+	scanner := bufio.NewScanner(file)
+	devicePattern := "\"" + filepath.Base(device) + "\""
+
+	for scanner.Scan() {
+		rule := scanner.Text()
+		if "" != rule && !strings.Contains(rule, devicePattern) {
+			rulesToKeep = append(rulesToKeep, rule)
+		}
+	}
+	file.Close()
+
+	// Update the file with the remaining rules or delete it
+	// if there is not any rule left.
+	if 0 < len(rulesToKeep) {
+		// Appending the []string list of rules in a single
+		// string to convert it later in []byte
+		var out string
+		for _, rule := range rulesToKeep {
+			out = out + rule + "\n"
+		}
+
+		if err := helpers.AtomicWriteFile(udevRulesFile, []byte(out), 0644); nil != err {
+			return err
+		}
+	} else {
+		if err := os.Remove(udevRulesFile); nil != err {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // RemoveHWAccess allows the given snap package to access the given hardware
 // device
 func RemoveHWAccess(snapname, device string) error {
@@ -227,14 +291,12 @@
 		return err
 	}
 
-	udevRulesFile := udevRulesPathForPart(snapname)
-	if helpers.FileExists(udevRulesFile) {
-		if err := os.Remove(udevRulesFile); err != nil {
-			return err
-		}
-		if err := activateOemHardwareUdevRules(); err != nil {
-			return err
-		}
+	if err = removeUdevRuleForSnap(snapname, device); nil != err {
+		return err
+	}
+
+	if err := activateOemHardwareUdevRules(); err != nil {
+		return err
 	}
 
 	// re-generate apparmor rules

=== modified file 'snappy/hwaccess_test.go'
--- snappy/hwaccess_test.go	2015-09-15 12:55:09 +0000
+++ snappy/hwaccess_test.go	2015-09-25 15:00:57 +0000
@@ -209,6 +209,14 @@
 }
 `)
 
+	// check the udev rule file contains all the rules
+	content, err = ioutil.ReadFile(filepath.Join(snapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `
+KERNEL=="bar", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+
+KERNEL=="bar*", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+`)
 	// remove
 	err = RemoveHWAccess("hello-app", "/dev/bar")
 	c.Assert(err, IsNil)
@@ -229,6 +237,11 @@
   ]
 }
 `)
+	// check the udevReadGlob Udev rule is still there
+	content, err = ioutil.ReadFile(filepath.Join(snapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `KERNEL=="bar*", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+`)
 }
 
 func makeRunUdevAdmMock(a *[][]string) func(args ...string) error {