Merge lp:~c-lobrano/snappy/hw-assign-symlink into lp:~snappy-dev/snappy/snappy-moved-to-github

[Carlo Lobrano]

Proposal fix for Bug #1496319

This branch extends hw-assign in order to have also a symlink to a "hw-assigned" device node.

The new signature of the command is:

   snappy hw-assign <device> <symlink>

the <symlink> parameter is optional, this way the new hw-assign is backward compatible.

Best regards

[Michael Vogt (mvo)]

Hey Carlo!

Thanks for the branch and sorry for the slow reply. We moved the code to github, but no worries, I imported your branch and pushed it github and created a pull request at https://github.com/ubuntu-core/snappy/pull/26

Lets continue in there. Feel free to branch from this above branch with your own, there are some merge conflicts right now, I can help resolve them if you want, just let me know.

Thanks again for your work on this and sorry for the trouble that this move might cause :)

Unmerged revisions

724. By Carlo Lobrano on 2015-10-19

Some fixes

1. add missing includes and calls due to previous merge
2. avoid raising errors when adding symlink to a device already in write path
3. avoid raising "file not found" error when the last UDEV rule has been removed
4. add new error when creating multiple symlinks to the same device
5. add test for the new error at point 4

723. By Carlo Lobrano on 2015-10-19

Merge master branch

722. By Carlo Lobrano on 2015-10-19

Revert add deviceProperties for hw-assign

Better to apply this features to a different snappy command (UDEV related maybe)

721. By Carlo Lobrano on 2015-10-01

Update hwaccess_test.go

updated tests that make use of AddSymlinkToHWDevice to manage the new parameter DeviceProperties

720. By Carlo Lobrano on 2015-09-30

Add deviceProperties to hw-assign

used to define the characteristics of the device to trigger the Udev symlink rule

719. By Carlo Lobrano on 2015-09-30

Rework of hw-unassign for symlinks and other fixes

cmd_hwassign.go:
 - Inverted hw assignement and symlink creation. The device is supposed to be already assigned to the snap, before creating the symlink.

errors.go:
 - added custom errors for symlinks.go

hwaccess.go:
 - reworked RemoveHWAccess to manage symlinks
 - add some wrapper functions to make things simpler

hwaccess_test.go:
 - add tests for new functions

718. By Carlo Lobrano on 2015-09-29

- Updated snappy.pot
- Add wrapper for checking whether a Snap has an apparmor json file
- Add wrapper for adding a new writepath for a Snap
- Added tests of new wrapper functions

717. By Carlo Lobrano on 2015-09-28

Udev rule to add symlink to HW device with hw-assign command

- added tests to verify the presence of such udev rule when the command is issued
- added third optional argument "symlinkpath" to hw-assign command
- added new method AddSymlinkToHWDevice to hwaccess module
- added new error for invalid symlink
- wrapped function to strip snapname from a full appname_binary-or-service_version string

=== modified file 'cmd/snappy/cmd_hwassign.go'
--- cmd/snappy/cmd_hwassign.go	2015-07-09 06:05:53 +0000
+++ cmd/snappy/cmd_hwassign.go	2015-10-19 15:22:22 +0000
@@ -29,14 +29,15 @@
 
 type cmdHWAssign struct {
 	Positional struct {
-		PackageName string `positional-arg-name:"package name"`
-		DevicePath  string `positional-arg-name:"device path"`
-	} `required:"true" positional-args:"yes"`
+		PackageName string `positional-arg-name:"package_name" required:"true"`
+		DevicePath  string `positional-arg-name:"device_path" required:"true"`
+		SymlinkPath string `positional-arg-name:"symlink_path" required:"false"`
+	} `positional-args:"yes"`
 }
 
 var shortHWAssignHelp = i18n.G("Assign a hardware device to a package")
 
-var longHWAssignHelp = i18n.G("This command adds access to a specific hardware device (e.g. /dev/ttyUSB0) for an installed package.")
+var longHWAssignHelp = i18n.G("This command adds access to a specific hardware device (e.g. /dev/ttyUSB0) for an installed package, possibly through symlink if provided.")
 
 func init() {
 	arg, err := parser.AddCommand("hw-assign",
@@ -46,8 +47,9 @@
 	if err != nil {
 		logger.Panicf("Unable to hwassign: %v", err)
 	}
-	addOptionDescription(arg, "package name", i18n.G("Assign hardware to a specific installed package"))
-	addOptionDescription(arg, "device path", i18n.G("The hardware device path (e.g. /dev/ttyUSB0)"))
+	addOptionDescription(arg, "package_name", i18n.G("Assign hardware to a specific installed package"))
+	addOptionDescription(arg, "device_path", i18n.G("The hardware device path (e.g. /dev/ttyUSB0)"))
+	addOptionDescription(arg, "symlink_path", i18n.G("The optional symlink to device path (e.g. /dev/symlink)"))
 }
 
 func (x *cmdHWAssign) Execute(args []string) error {
@@ -57,15 +59,29 @@
 func (x *cmdHWAssign) doHWAssign() error {
 	if err := snappy.AddHWAccess(x.Positional.PackageName, x.Positional.DevicePath); err != nil {
 		if err == snappy.ErrHWAccessAlreadyAdded {
-			// TRANSLATORS: the first %s is a pkgname, the second %s is a path
-			fmt.Printf(i18n.G("'%s' previously allowed access to '%s'. Skipping\n"), x.Positional.PackageName, x.Positional.DevicePath)
-			return nil
-		}
-
-		return err
-	}
-
-	// TRANSLATORS: the first %s is a pkgname, the second %s is a path
-	fmt.Printf(i18n.G("'%s' is now allowed to access '%s'\n"), x.Positional.PackageName, x.Positional.DevicePath)
+			if "" == x.Positional.SymlinkPath {
+				// TRANSLATORS: the first %s is a pkgname, the second %s is a path
+				fmt.Printf(i18n.G("'%s' previously allowed access to '%s'. Skipping\n"), x.Positional.PackageName, x.Positional.DevicePath)
+				return nil
+			}
+		} else {
+			return err
+		}
+	}
+
+	if "" != x.Positional.SymlinkPath {
+		if err := snappy.AddSymlinkToHWDevice(
+			x.Positional.PackageName,
+			x.Positional.DevicePath,
+			x.Positional.SymlinkPath); err != nil {
+			return err
+		}
+		// TRANSLATORS: the first %s is a pkgname, the second %s is a path
+		fmt.Printf(i18n.G("'%s' is allowed to access '%s' through symlink '%s' \n"), x.Positional.PackageName, x.Positional.DevicePath, x.Positional.SymlinkPath)
+	} else {
+		// TRANSLATORS: the first %s is a pkgname, the second %s is a path
+		fmt.Printf(i18n.G("'%s' is allowed to access '%s'\n"), x.Positional.PackageName, x.Positional.DevicePath)
+	}
+
 	return nil
 }

=== modified file 'po/snappy.pot'
--- po/snappy.pot	2015-09-29 09:45:15 +0000
+++ po/snappy.pot	2015-10-19 15:22:22 +0000
@@ -7,7 +7,7 @@
 msgid   ""
 msgstr  "Project-Id-Version: snappy\n"
         "Report-Msgid-Bugs-To: [e-mail]\n"
-        "POT-Creation-Date: 2015-09-29 10:18+0100\n"
+        "POT-Creation-Date: 2015-10-19 17:05+0200\n"
         "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
         "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
         "Language-Team: LANGUAGE <[e-mail]>\n"
@@ -28,16 +28,21 @@
 
 #. TRANSLATORS: the first %s is a pkgname, the second %s is a path
 #, c-format
+msgid   "'%s' is allowed to access '%s' through symlink '%s' \n"
+msgstr  ""
+
+#. TRANSLATORS: the first %s is a pkgname, the second %s is a path
+#, c-format
+msgid   "'%s' is allowed to access '%s'\n"
+msgstr  ""
+
+#. TRANSLATORS: the first %s is a pkgname, the second %s is a path
+#, c-format
 msgid   "'%s' is no longer allowed to access '%s'\n"
 msgstr  ""
 
 #. TRANSLATORS: the first %s is a pkgname, the second %s is a path
 #, c-format
-msgid   "'%s' is now allowed to access '%s'\n"
-msgstr  ""
-
-#. TRANSLATORS: the first %s is a pkgname, the second %s is a path
-#, c-format
 msgid   "'%s' previously allowed access to '%s'. Skipping\n"
 msgstr  ""
 
@@ -287,13 +292,16 @@
 msgid   "The hardware device path (e.g. /dev/ttyUSB0)"
 msgstr  ""
 
+msgid   "The optional symlink to device path (e.g. /dev/symlink)"
+msgstr  ""
+
 msgid   "The package to rollback "
 msgstr  ""
 
 msgid   "The version to rollback to"
 msgstr  ""
 
-msgid   "This command adds access to a specific hardware device (e.g. /dev/ttyUSB0) for an installed package."
+msgid   "This command adds access to a specific hardware device (e.g. /dev/ttyUSB0) for an installed package, possibly through symlink if provided."
 msgstr  ""
 
 msgid   "This command is no longer available, please use the \"list\" command"

=== modified file 'snappy/errors.go'
--- snappy/errors.go	2015-09-15 12:55:09 +0000
+++ snappy/errors.go	2015-10-19 15:22:22 +0000
@@ -51,6 +51,14 @@
 	// is given in the hw-assign command
 	ErrInvalidHWDevice = errors.New("invalid hardware device")
 
+	// ErrInvalidSymlinkToHWDevice is returned when a invalid symlink to hardware device
+	// is given in the hw-assign command
+	ErrInvalidSymlinkToHWDevice = errors.New("invalid symlink to hardware device")
+
+	// ErrSymlinkToHWNameCollision is returned when a symlink with the same name of a
+	// device in the write path is requested
+	ErrSymlinkToHWNameCollision = errors.New("symlink's name collides with on of the snap's write paths")
+
 	// ErrHWAccessRemoveNotFound is returned if the user tries to
 	// remove a device that does not exist
 	ErrHWAccessRemoveNotFound = errors.New("can not find device in hw-access list")
@@ -59,6 +67,14 @@
 	// that is already in the hwaccess list
 	ErrHWAccessAlreadyAdded = errors.New("device is already in hw-access list")
 
+	// ErrHwDeviceAlreadySymlinked is returned when the device has already a symlink
+	// with a different name
+	ErrHwDeviceAlreadySymlinked = errors.New("device has a symlink already")
+
+	// ErrSymlinkToHWAlreadyAdded is returned if you try to add a symlink
+	// that is already in the hwaccess list
+	ErrSymlinkToHWAlreadyAdded = errors.New("symlink is already in hw-access list")
+
 	// ErrReadmeInvalid is returned if the package contains a invalid
 	// meta/readme.md
 	ErrReadmeInvalid = errors.New("meta/readme.md invalid")

=== modified file 'snappy/hwaccess.go'
--- snappy/hwaccess.go	2015-09-29 07:07:13 +0000
+++ snappy/hwaccess.go	2015-10-19 15:22:22 +0000
@@ -38,8 +38,9 @@
 var aaClickHookCmd = "aa-clickhook"
 
 type appArmorAdditionalJSON struct {
-	WritePath []string `json:"write_path,omitempty"`
-	ReadPath  []string `json:"read_path,omitempty"`
+	WritePath   []string          `json:"write_path,omitempty"`
+	ReadPath    []string          `json:"read_path,omitempty"`
+	SymlinkPath map[string]string `json:"symlink_path,omitempty"`
 }
 
 // return the json filename to add to the security json
@@ -137,16 +138,25 @@
 	return nil
 }
 
+// StripSnapName extracts the snapname from a full
+// appname_binary-or-service_version string
+func stripSnapName(snapname string) string {
+	strippedSnapname := snapname
+	if strings.Contains(snapname, "_") {
+		l := strings.Split(snapname, "_")
+		strippedSnapname = l[0]
+	}
+
+	return strippedSnapname
+}
+
 func writeUdevRuleForDeviceCgroup(snapname, device string) error {
 	os.MkdirAll(dirs.SnapUdevRulesDir, 0755)
 
 	// the device cgroup/launcher etc support only the apps level,
 	// not a binary/service or version, so if we get a full
 	// appname_binary-or-service_version string we need to split that
-	if strings.Contains(snapname, "_") {
-		l := strings.Split(snapname, "_")
-		snapname = l[0]
-	}
+	snapname = stripSnapName(snapname)
 
 	acl := fmt.Sprintf(`
 KERNEL=="%v", TAG:="snappy-assign", ENV{SNAPPY_APP}:="%s"
@@ -159,16 +169,27 @@
 	return activateOemHardwareUdevRules()
 }
 
+func writeSymlinkUdevRuleForDeviceCgroup(snapname, device, symlink string) error {
+	os.MkdirAll(dirs.SnapUdevRulesDir, 0755)
+
+	snapname = stripSnapName(snapname)
+
+	acl := fmt.Sprintf(`
+ACTION=="add", KERNEL=="%v", TAG:="snappy-assign", ENV{SNAPPY_APP}:="%s", SYMLINK+="%v"
+`, filepath.Base(device), snapname, filepath.Base(symlink))
+
+	if err := addUdevRuleForSnap(snapname, acl); err != nil {
+		return err
+	}
+
+	return activateOemHardwareUdevRules()
+
+}
+
 var regenerateAppArmorRules = regenerateAppArmorRulesImpl
 
-// AddHWAccess allows the given snap package to access the given hardware
-// device
-func AddHWAccess(snapname, device string) error {
-	if !validDevice(device) {
-		return ErrInvalidHWDevice
-	}
-
-	// check if there is anything apparmor related to add to
+// check if there is anything apparmor related to add to
+func hasSnapApparmorJSON(snapname string) error {
 	globExpr := filepath.Join(dirs.SnapAppArmorDir, fmt.Sprintf("%s_*.json", snapname))
 	matches, err := filepath.Glob(globExpr)
 	if err != nil {
@@ -178,6 +199,10 @@
 		return ErrPackageNotFound
 	}
 
+	return nil
+}
+
+func addNewWritePathForSnap(snapname, device string) error {
 	// read .additional file, its ok if the file does not exist (yet)
 	appArmorAdditional, err := readHWAccessJSONFile(snapname)
 	if err != nil && !os.IsNotExist(err) {
@@ -199,6 +224,75 @@
 		return err
 	}
 
+	return nil
+}
+
+func isStringInSlice(key string, slice []string) bool {
+	for _, p := range slice {
+		if p == key {
+			return true
+		}
+	}
+	return false
+}
+
+func addNewSymlinkPathForSnap(snapname, symlink, device string) error {
+	// read .additional file, its ok if the file does not exist (yet)
+	appArmorAdditional, err := readHWAccessJSONFile(snapname)
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+
+	// It is expected to have already access to the
+	// real hw device before creating a symlink to it
+	if !isStringInSlice(device, appArmorAdditional.WritePath) {
+		return ErrHWAccessRemoveNotFound
+	}
+
+	if isStringInSlice(symlink, appArmorAdditional.WritePath) {
+		return ErrSymlinkToHWNameCollision
+	}
+
+	if nil != appArmorAdditional.SymlinkPath {
+		for key, item := range appArmorAdditional.SymlinkPath {
+			if key == symlink {
+				return ErrSymlinkToHWAlreadyAdded
+			}
+			if item == device {
+				return ErrHwDeviceAlreadySymlinked
+			}
+		}
+	} else {
+		appArmorAdditional.SymlinkPath = make(map[string]string)
+	}
+
+	// add the new symlink:hw-device pair
+	appArmorAdditional.SymlinkPath[symlink] = device
+
+	// and write the data out
+	err = writeHWAccessJSONFile(snapname, appArmorAdditional)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// AddHWAccess allows the given snap package to access the given hardware
+// device
+func AddHWAccess(snapname, device string) error {
+	if !validDevice(device) {
+		return ErrInvalidHWDevice
+	}
+
+	if err := hasSnapApparmorJSON(snapname); nil != err {
+		return err
+	}
+
+	if err := addNewWritePathForSnap(snapname, device); nil != err {
+		return err
+	}
+
 	// add udev rule for device cgroup
 	if err := writeUdevRuleForDeviceCgroup(snapname, device); err != nil {
 		return err
@@ -208,6 +302,32 @@
 	return regenerateAppArmorRules()
 }
 
+// AddSymlinkToHWDevice writes an Udev rule to create a symlink to the
+// given hardware device
+func AddSymlinkToHWDevice(snapname, device, symlink string) error {
+	if !validDevice(device) {
+		return ErrInvalidHWDevice
+	}
+
+	if !validDevice(symlink) {
+		return ErrInvalidSymlinkToHWDevice
+	}
+
+	if err := hasSnapApparmorJSON(snapname); nil != err {
+		return err
+	}
+
+	if err := addNewSymlinkPathForSnap(snapname, symlink, device); nil != err {
+		return err
+	}
+
+	if err := writeSymlinkUdevRuleForDeviceCgroup(snapname, device, symlink); nil != err {
+		return err
+	}
+
+	return nil
+}
+
 // ListHWAccess returns a list of hardware-device strings that the snap
 // can access
 func ListHWAccess(snapname string) ([]string, error) {
@@ -264,8 +384,8 @@
 
 // RemoveHWAccess allows the given snap package to access the given hardware
 // device
-func RemoveHWAccess(snapname, device string) error {
-	if !validDevice(device) {
+func RemoveHWAccess(snapname, path string) error {
+	if !validDevice(path) {
 		return ErrInvalidHWDevice
 	}
 
@@ -274,26 +394,71 @@
 		return err
 	}
 
-	// remove write path, please golang make this easier!
-	newWritePath := []string{}
-	for _, p := range appArmorAdditional.WritePath {
-		if p != device {
-			newWritePath = append(newWritePath, p)
-		}
-	}
-	if len(newWritePath) == len(appArmorAdditional.WritePath) {
-		return ErrHWAccessRemoveNotFound
-	}
-	appArmorAdditional.WritePath = newWritePath
-
-	// and write it out again
-	err = writeHWAccessJSONFile(snapname, appArmorAdditional)
-	if err != nil {
-		return err
-	}
-
-	if err = removeUdevRuleForSnap(snapname, device); nil != err {
-		return err
+	// Check whether it is a symlink first
+	device, symlink, err := resolveSymlink(snapname, path)
+	if nil != err {
+		return err
+	}
+
+	// When unassigning a symlink, path == symlink and only the
+	// symlink has to be removed.
+	// When unassigning an HW device, path == device (while symlink
+	// can be another path or a null string) and both device
+	// and symlink (if any) have to be removed.
+	if path == device {
+		// remove write path, please golang make this easier!
+		newWritePath := []string{}
+		for _, p := range appArmorAdditional.WritePath {
+			if p != device {
+				newWritePath = append(newWritePath, p)
+			}
+		}
+
+		if len(newWritePath) == len(appArmorAdditional.WritePath) {
+			return ErrHWAccessRemoveNotFound
+		}
+
+		// Update WritePath
+		appArmorAdditional.WritePath = newWritePath
+
+		// and write it out again
+		err = writeHWAccessJSONFile(snapname, appArmorAdditional)
+		if err != nil {
+			return err
+		}
+
+		if err = removeUdevRuleForSnap(snapname, device); nil != err {
+			return err
+		}
+
+	}
+
+	if "" != symlink {
+		newSymlinkPath := make(map[string]string)
+		for key, value := range appArmorAdditional.SymlinkPath {
+			if key != symlink {
+				newSymlinkPath[key] = value
+			}
+		}
+
+		if len(appArmorAdditional.SymlinkPath) == len(newSymlinkPath) {
+			return ErrHWAccessRemoveNotFound
+		}
+
+		appArmorAdditional.SymlinkPath = newSymlinkPath
+
+		err = writeHWAccessJSONFile(snapname, appArmorAdditional)
+		if err != nil {
+			return err
+		}
+
+		if err := removeUdevRuleForSnap(snapname, symlink); nil != err {
+			// When there are only rules for the same HW device, the UDEV
+			// rule file might not exist anymore at this point.
+			if !os.IsNotExist(err) {
+				return err
+			}
+		}
 	}
 
 	if err := activateOemHardwareUdevRules(); err != nil {
@@ -317,3 +482,25 @@
 
 	return regenerateAppArmorRules()
 }
+
+func resolveSymlink(snapname, path string) (device, symlink string, err error) {
+	appArmorAdditional, err := readHWAccessJSONFile(snapname)
+	if nil != err {
+		return "", "", err
+	}
+
+	if nil != appArmorAdditional.SymlinkPath {
+		for symlink, device := range appArmorAdditional.SymlinkPath {
+			if path == symlink || path == device {
+				return device, symlink, nil
+			}
+		}
+	}
+
+	// no symlink found, path must be an HW device
+	if !isStringInSlice(path, appArmorAdditional.WritePath) {
+		return "", "", ErrHWAccessRemoveNotFound
+	}
+
+	return path, "", nil
+}

=== modified file 'snappy/hwaccess_test.go'
--- snappy/hwaccess_test.go	2015-09-29 07:07:13 +0000
+++ snappy/hwaccess_test.go	2015-10-19 15:22:22 +0000
@@ -290,6 +290,24 @@
 	verifyUdevAdmActivateRules(c, runUdevAdmCalls)
 }
 
+func (s *SnapTestSuite) TestWriteSymlinkUdevRuleForDeviceCgroup(c *C) {
+	var runUdevAdmCalls [][]string
+	runUdevAdm = makeRunUdevAdmMock(&runUdevAdmCalls)
+
+	snapapp := "foo-app_meep_1.0"
+
+	err := writeSymlinkUdevRuleForDeviceCgroup(snapapp, "/dev/ttyS0", "/dev/symS0")
+	c.Assert(err, IsNil)
+
+	got, err := ioutil.ReadFile(filepath.Join(dirs.SnapUdevRulesDir, "70-snappy_hwassign_foo-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(got), Equals, `
+ACTION=="add", KERNEL=="ttyS0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="foo-app", SYMLINK+="symS0"
+`)
+
+	verifyUdevAdmActivateRules(c, runUdevAdmCalls)
+}
+
 func (s *SnapTestSuite) TestRemoveAllHWAccess(c *C) {
 	makeInstalledMockSnap(s.tempdir, "")
 
@@ -320,3 +338,203 @@
 		Output:   []byte("meep\n"),
 	})
 }
+
+func (s *SnapTestSuite) TestHasSnapApparmorJSON(c *C) {
+	err := hasSnapApparmorJSON("non-existent-app")
+	c.Assert(err, Equals, ErrPackageNotFound)
+
+	makeInstalledMockSnap(s.tempdir, "")
+	err = AddHWAccess("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+
+	err = hasSnapApparmorJSON("hello-app")
+	c.Assert(err, IsNil)
+}
+
+func (s *SnapTestSuite) TestAddNewWritePathForSnap(c *C) {
+	// try add same path twice
+	makeInstalledMockSnap(s.tempdir, "")
+	err := addNewWritePathForSnap("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+	err = addNewWritePathForSnap("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, Equals, ErrHWAccessAlreadyAdded)
+
+	// check .additional file is written right
+	content, err := ioutil.ReadFile(filepath.Join(dirs.SnapAppArmorDir, "hello-app.json.additional"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `{
+  "write_path": [
+    "/dev/ttyUSB0"
+  ],
+  "read_path": [
+    "/run/udev/data/*"
+  ]
+}
+`)
+}
+
+func (s *SnapTestSuite) TestAddNewSymlinkPathForSnap(c *C) {
+	makeInstalledMockSnap(s.tempdir, "")
+
+	// try add as symlink to an hw device not in write path, yet
+	err := addNewSymlinkPathForSnap("hello-app", "/dev/symtest0", "/dev/ttyUSB0")
+	c.Assert(err, Equals, ErrHWAccessRemoveNotFound)
+
+	// try add the same symlink twice
+	err = addNewWritePathForSnap("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+	err = addNewSymlinkPathForSnap("hello-app", "/dev/symtest0", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+	err = addNewSymlinkPathForSnap("hello-app", "/dev/symtest0", "/dev/ttyUSB0")
+	c.Assert(err, Equals, ErrSymlinkToHWAlreadyAdded)
+
+	// try add a new symlink to a device that already has one
+	err = addNewSymlinkPathForSnap("hello-app", "/dev/symtest1", "/dev/ttyUSB0")
+	c.Assert(err, Equals, ErrHwDeviceAlreadySymlinked)
+
+	// check .additional file is written right
+	content, err := ioutil.ReadFile(filepath.Join(dirs.SnapAppArmorDir, "hello-app.json.additional"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `{
+  "write_path": [
+    "/dev/ttyUSB0"
+  ],
+  "read_path": [
+    "/run/udev/data/*"
+  ],
+  "symlink_path": {
+    "/dev/symtest0": "/dev/ttyUSB0"
+  }
+}
+`)
+
+	// try add a second symlink
+	err = AddHWAccess("hello-app", "/dev/ttyUSB1")
+	c.Assert(err, IsNil)
+
+	err = AddSymlinkToHWDevice("hello-app", "/dev/ttyUSB1", "/dev/symtest1")
+	c.Assert(err, IsNil)
+
+	// check .additional file is written right
+	content, err = ioutil.ReadFile(filepath.Join(dirs.SnapAppArmorDir, "hello-app.json.additional"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `{
+  "write_path": [
+    "/dev/ttyUSB0",
+    "/dev/ttyUSB1"
+  ],
+  "read_path": [
+    "/run/udev/data/*"
+  ],
+  "symlink_path": {
+    "/dev/symtest0": "/dev/ttyUSB0",
+    "/dev/symtest1": "/dev/ttyUSB1"
+  }
+}
+`)
+	// try add symlink with the same name of one of snap's write paths
+	err = addNewSymlinkPathForSnap("hello-app", "/dev/ttyUSB1", "/dev/ttyUSB1")
+	c.Assert(err, Equals, ErrSymlinkToHWNameCollision)
+}
+
+func (s *SnapTestSuite) TestRemoveSymlinkToHWDevice(c *C) {
+	aaClickHookCmd = "true"
+	makeInstalledMockSnap(s.tempdir, "")
+
+	// Add access to 2 devices
+	err := AddHWAccess("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+	err = AddHWAccess("hello-app", "/dev/ttyUSB1")
+	c.Assert(err, IsNil)
+
+	// Adding a symlinks
+	err = AddSymlinkToHWDevice("hello-app", "/dev/ttyUSB0", "/dev/symtest0")
+	c.Assert(err, IsNil)
+
+	err = AddSymlinkToHWDevice("hello-app", "/dev/ttyUSB1", "/dev/symtest1")
+	c.Assert(err, IsNil)
+
+	// Remove hw device with symlink: the symlink is expected to be removed too
+	err = RemoveHWAccess("hello-app", "/dev/ttyUSB1")
+	c.Assert(err, IsNil)
+
+	content, err := ioutil.ReadFile(filepath.Join(dirs.SnapAppArmorDir, "hello-app.json.additional"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `{
+  "write_path": [
+    "/dev/ttyUSB0"
+  ],
+  "read_path": [
+    "/run/udev/data/*"
+  ],
+  "symlink_path": {
+    "/dev/symtest0": "/dev/ttyUSB0"
+  }
+}
+`)
+
+	// Remove only the symlink
+	err = RemoveHWAccess("hello-app", "/dev/symtest0")
+	c.Assert(err, IsNil)
+
+	// having removed the last symlink, SymlinkPath is expected to be nil
+	appArmorAdditional, err := readHWAccessJSONFile("hello-app")
+	c.Assert(err, IsNil)
+	c.Assert(appArmorAdditional.SymlinkPath, IsNil)
+
+	// expecting write path unchanged
+	content, err = ioutil.ReadFile(filepath.Join(dirs.SnapAppArmorDir, "hello-app.json.additional"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `{
+  "write_path": [
+    "/dev/ttyUSB0"
+  ],
+  "read_path": [
+    "/run/udev/data/*"
+  ]
+}
+`)
+}
+
+func (s *SnapTestSuite) TestRemoveUdevRuleForSnap(c *C) {
+	aaClickHookCmd = "true"
+	makeInstalledMockSnap(s.tempdir, "")
+
+	// Add access to 3 devices and 2 symlinks
+	err := AddHWAccess("hello-app", "/dev/ttyUSB0")
+	c.Assert(err, IsNil)
+	err = AddHWAccess("hello-app", "/dev/ttyUSB1")
+	c.Assert(err, IsNil)
+	err = AddHWAccess("hello-app", "/dev/ttyUSB2")
+	c.Assert(err, IsNil)
+	err = AddSymlinkToHWDevice("hello-app", "/dev/ttyUSB0", "/dev/symlink0")
+	c.Assert(err, IsNil)
+	err = AddSymlinkToHWDevice("hello-app", "/dev/ttyUSB1", "/dev/symlink1")
+	c.Assert(err, IsNil)
+
+	// Remove the device without symlink
+	err = RemoveHWAccess("hello-app", "/dev/ttyUSB2")
+	c.Assert(err, IsNil)
+	content, err := ioutil.ReadFile(filepath.Join(dirs.SnapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `KERNEL=="ttyUSB0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+KERNEL=="ttyUSB1", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+ACTION=="add", KERNEL=="ttyUSB0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app", SYMLINK+="symlink0"
+ACTION=="add", KERNEL=="ttyUSB1", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app", SYMLINK+="symlink1"
+`)
+	// Remove a device with a symlink
+	err = RemoveHWAccess("hello-app", "/dev/ttyUSB1")
+	c.Assert(err, IsNil)
+	content, err = ioutil.ReadFile(filepath.Join(dirs.SnapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `KERNEL=="ttyUSB0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+ACTION=="add", KERNEL=="ttyUSB0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app", SYMLINK+="symlink0"
+`)
+	// Remove the symlink
+	err = RemoveHWAccess("hello-app", "/dev/symlink0")
+	c.Assert(err, IsNil)
+	content, err = ioutil.ReadFile(filepath.Join(dirs.SnapUdevRulesDir, "70-snappy_hwassign_hello-app.rules"))
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Equals, `KERNEL=="ttyUSB0", TAG:="snappy-assign", ENV{SNAPPY_APP}:="hello-app"
+`)
+}