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

Proposed by Carlo Lobrano
Status: Merged
Approved by: John Lenton
Approved revision: 719
Merged at revision: 718
Proposed branch: lp:~c-lobrano/snappy/hw-assign-fix-overwrite-udev-rule
Merge into: lp:~snappy-dev/snappy/snappy-moved-to-github
Diff against target: 159 lines (+87/-9)
3 files modified
.bzrignore (+3/-0)
snappy/hwaccess.go (+71/-9)
snappy/hwaccess_test.go (+13/-0)
To merge this branch: bzr merge lp:~c-lobrano/snappy/hw-assign-fix-overwrite-udev-rule
Reviewer Review Type Date Requested Status
John Lenton (community) Approve
Review via email: mp+272000@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
John Lenton (chipaca) wrote :

Thank you for this!

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

review: Needs Fixing
Revision history for this message
Carlo Lobrano (c-lobrano) :
Revision history for this message
John Lenton (chipaca) wrote :

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.

Revision history for this message
Carlo Lobrano (c-lobrano) :
Revision history for this message
John Lenton (chipaca) wrote :

Good. Thank you!

review: Approve
Revision history for this message
John Lenton (chipaca) :
Revision history for this message
Carlo Lobrano (c-lobrano) :
Revision history for this message
Snappy Tarmac (snappydevtarmac) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file '.bzrignore'
--- .bzrignore 2015-08-25 16:36:01 +0000
+++ .bzrignore 2015-09-25 15:00:57 +0000
@@ -1,2 +1,5 @@
1_integration-tests/bin/1_integration-tests/bin/
2_integration-tests/data/output/2_integration-tests/data/output/
3./share
4./snappy/hello_1.0.1_all.snap
5./tags
36
=== 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 @@
20package snappy20package snappy
2121
22import (22import (
23 "bufio"
23 "encoding/json"24 "encoding/json"
24 "fmt"25 "fmt"
25 "io/ioutil"26 "io/ioutil"
@@ -115,6 +116,26 @@
115 return filepath.Join(snapUdevRulesDir, fmt.Sprintf("70-snappy_hwassign_%s.rules", partid))116 return filepath.Join(snapUdevRulesDir, fmt.Sprintf("70-snappy_hwassign_%s.rules", partid))
116}117}
117118
119func addUdevRuleForSnap(snapname, newRule string) error {
120 udevRulesFile := udevRulesPathForPart(snapname)
121
122 rules, err := ioutil.ReadFile(udevRulesFile)
123 if nil != err && !os.IsNotExist(err) {
124 return err
125 }
126
127 // At this point either rules variable contains some rules if the
128 // file exists, or it is nil if the file does not exist yet.
129 // In both cases, updatedRules will have the right content.
130 updatedRules := append(rules, newRule...)
131
132 if err := helpers.AtomicWriteFile(udevRulesFile, updatedRules, 0644); nil != err {
133 return err
134 }
135
136 return nil
137}
138
118func writeUdevRuleForDeviceCgroup(snapname, device string) error {139func writeUdevRuleForDeviceCgroup(snapname, device string) error {
119 os.MkdirAll(snapUdevRulesDir, 0755)140 os.MkdirAll(snapUdevRulesDir, 0755)
120141
@@ -130,7 +151,7 @@
130KERNEL=="%v", TAG:="snappy-assign", ENV{SNAPPY_APP}:="%s"151KERNEL=="%v", TAG:="snappy-assign", ENV{SNAPPY_APP}:="%s"
131`, filepath.Base(device), snapname)152`, filepath.Base(device), snapname)
132153
133 if err := ioutil.WriteFile(udevRulesPathForPart(snapname), []byte(acl), 0644); err != nil {154 if err := addUdevRuleForSnap(snapname, acl); err != nil {
134 return err155 return err
135 }156 }
136157
@@ -197,6 +218,49 @@
197 return appArmorAdditional.WritePath, nil218 return appArmorAdditional.WritePath, nil
198}219}
199220
221func removeUdevRuleForSnap(snapname, device string) error {
222 udevRulesFile := udevRulesPathForPart(snapname)
223
224 file, err := os.Open(udevRulesFile)
225 if nil != err && !os.IsNotExist(err) {
226 return err
227 }
228
229 // Get the full list of rules to keep
230 var rulesToKeep []string
231 scanner := bufio.NewScanner(file)
232 devicePattern := "\"" + filepath.Base(device) + "\""
233
234 for scanner.Scan() {
235 rule := scanner.Text()
236 if "" != rule && !strings.Contains(rule, devicePattern) {
237 rulesToKeep = append(rulesToKeep, rule)
238 }
239 }
240 file.Close()
241
242 // Update the file with the remaining rules or delete it
243 // if there is not any rule left.
244 if 0 < len(rulesToKeep) {
245 // Appending the []string list of rules in a single
246 // string to convert it later in []byte
247 var out string
248 for _, rule := range rulesToKeep {
249 out = out + rule + "\n"
250 }
251
252 if err := helpers.AtomicWriteFile(udevRulesFile, []byte(out), 0644); nil != err {
253 return err
254 }
255 } else {
256 if err := os.Remove(udevRulesFile); nil != err {
257 return err
258 }
259 }
260
261 return nil
262}
263
200// RemoveHWAccess allows the given snap package to access the given hardware264// RemoveHWAccess allows the given snap package to access the given hardware
201// device265// device
202func RemoveHWAccess(snapname, device string) error {266func RemoveHWAccess(snapname, device string) error {
@@ -227,14 +291,12 @@
227 return err291 return err
228 }292 }
229293
230 udevRulesFile := udevRulesPathForPart(snapname)294 if err = removeUdevRuleForSnap(snapname, device); nil != err {
231 if helpers.FileExists(udevRulesFile) {295 return err
232 if err := os.Remove(udevRulesFile); err != nil {296 }
233 return err297
234 }298 if err := activateOemHardwareUdevRules(); err != nil {
235 if err := activateOemHardwareUdevRules(); err != nil {299 return err
236 return err
237 }
238 }300 }
239301
240 // re-generate apparmor rules302 // re-generate apparmor rules
241303
=== 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 @@
209}209}
210`)210`)
211211
212 // check the udev rule file contains all the rules
213 content, err = ioutil.ReadFile(filepath.Join(snapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
214 c.Assert(err, IsNil)
215 c.Assert(string(content), Equals, `
216KERNEL=="bar", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
217
218KERNEL=="bar*", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
219`)
212 // remove220 // remove
213 err = RemoveHWAccess("hello-app", "/dev/bar")221 err = RemoveHWAccess("hello-app", "/dev/bar")
214 c.Assert(err, IsNil)222 c.Assert(err, IsNil)
@@ -229,6 +237,11 @@
229 ]237 ]
230}238}
231`)239`)
240 // check the udevReadGlob Udev rule is still there
241 content, err = ioutil.ReadFile(filepath.Join(snapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
242 c.Assert(err, IsNil)
243 c.Assert(string(content), Equals, `KERNEL=="bar*", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
244`)
232}245}
233246
234func makeRunUdevAdmMock(a *[][]string) func(args ...string) error {247func makeRunUdevAdmMock(a *[][]string) func(args ...string) error {

Subscribers

People subscribed via source and target branches