Merge lp:~sergiusens/snappy/ubootNoUnnecessaryRewrites into lp:~snappy-dev/snappy/snappy-moved-to-github

[Sergio Schvezov]

Avoid writing to snappy-system.txt if not required

This code base needs some love and I would of done it here if we didn't need to backport this commit to the 15.04 tree.

[John Lenton (chipaca)]

review: Approve

[Michael Vogt (mvo)]

Looks good, some comments inline, feel free to ignore.

[Sergio Schvezov (sergiusens)]

(BeagleBoneBlack)ubuntu@localhost:~$ ls /boot/uboot/ -l
total 4
drwxr-xr-x 3 root root 512 May 21 08:47 a
drwxr-xr-x 3 root root 512 May 21 08:47 b
-rwxr-xr-x 1 root root 1585 May 21 08:47 snappy-system.txt
-rwxr-xr-x 1 root root 237 May 21 08:47 uEnv.txt
(BeagleBoneBlack)ubuntu@localhost:~$ sudo snappy booted
(BeagleBoneBlack)ubuntu@localhost:~$ ls /boot/uboot/ -l
total 4
drwxr-xr-x 3 root root 512 May 21 08:47 a
drwxr-xr-x 3 root root 512 May 21 08:47 b
-rwxr-xr-x 1 root root 1585 May 21 08:47 snappy-system.txt
-rwxr-xr-x 1 root root 237 May 21 08:47 uEnv.txt
(BeagleBoneBlack)ubuntu@localhost:~$ sudo snappy list --verbose
Name Date Version Developer
ubuntu-core 2015-05-21 2 ubuntu*
ubuntu-core 2015-05-21 2 ubuntu
webdm 2015-05-21 0.6.1 *
beagleblack 2015-05-21 1.7.1 *
(BeagleBoneBlack)ubuntu@localhost:~$ sudo snappy update
Installing ubuntu-core (60)
Starting download of ubuntu-core
138.23 MB / 138.23 MB [=============================] 100.00 % 241.93 KB/s 9m45s
Done
Name Date Version Developer
ubuntu-core 2015-05-20 60 ubuntu!
Reboot to use the new ubuntu-core.

# timestamp is updated (regular->try a->b)
(BeagleBoneBlack)ubuntu@localhost:~$ ls /boot/uboot/ -l
total 4
drwxr-xr-x 3 root root 512 May 21 08:47 a
drwxr-xr-x 3 root root 512 May 21 08:47 b
-rwxr-xr-x 1 root root 1581 May 21 12:46 snappy-system.txt
-rwxr-xr-x 1 root root 237 May 21 08:47 uEnv.txt
(BeagleBoneBlack)ubuntu@localhost:~$ sudo reboot
# ...
# timestamp updated (try->regular)
(BeagleBoneBlack)ubuntu@localhost:~$ ls /boot/uboot/ -l
total 4
drwxr-xr-x 3 root root 512 May 21 08:47 a
drwxr-xr-x 3 root root 512 May 21 08:47 b
-rwxr-xr-x 1 root root 1585 May 21 12:54 snappy-system.txt
-rwxr-xr-x 1 root root 237 May 21 08:47 uEnv.txt

I'll do another test later

[Sergio Schvezov (sergiusens)]

thanks for the review, will update in a bit

[lp:~sergiusens/snappy/ubootNoUnnecessaryRewrites]

469. By Sergio Schvezov on 2015-05-21

Remove unnecessary check if atomicWrite was called.

470. By Sergio Schvezov on 2015-05-21

Don't make atomicFileUpdateImpl a var

471. By Sergio Schvezov on 2015-05-21

Taking into consideration the almost impossible case of snappy-system.txt being incomplete.

[Michael Vogt (mvo)]

Thanks a lot for this update Sergio! This is good to go now :) In a better world we would also have a unittest for atomicFileUpdate() but I think the tests you added are just fine for now (as this needs a 15.04 backport). And we can improve further later. In any case, thanks a lot for working on this!

review: Approve

[Michael Vogt (mvo)]

What should we do about the cherry pick to 15.04? Will there be a separate branch?

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-05-15 13:33:27 +0000
+++ partition/bootloader_uboot.go	2015-05-21 17:13:00 +0000
@@ -53,6 +53,7 @@
 	bootloaderUbootConfigFile = bootloaderUbootConfigFileReal
 	bootloaderUbootStampFile  = bootloaderUbootStampFileReal
 	bootloaderUbootEnvFile    = bootloaderUbootEnvFileReal
+	atomicFileUpdate          = atomicFileUpdateImpl
 )
 
 const bootloaderNameUboot bootloaderName = "u-boot"
@@ -66,6 +67,7 @@
 }
 
 // Stores a Name and a Value to be added as a name=value pair in a file.
+// TODO convert to map
 type configFileChange struct {
 	Name  string
 	Value string
@@ -322,7 +324,7 @@
 
 // Write lines to file atomically. File does not have to preexist.
 // FIXME: put into utils package
-func atomicFileUpdate(file string, lines []string) (err error) {
+func atomicFileUpdateImpl(file string, lines []string) (err error) {
 	tmpFile := fmt.Sprintf("%s.NEW", file)
 
 	// XXX: if go switches to use aio_fsync, we need to open the dir for writing
@@ -351,6 +353,7 @@
 // appended to the file.
 //
 // FIXME: put into utils package
+// FIXME: improve logic
 func modifyNameValueFile(file string, changes []configFileChange) (err error) {
 	var updated []configFileChange
 
@@ -360,12 +363,20 @@
 	}
 
 	var new []string
+	// we won't write to a file if we don't need to.
+	updateNeeded := false
 
 	for _, line := range lines {
 		for _, change := range changes {
 			if strings.HasPrefix(line, fmt.Sprintf("%s=", change.Name)) {
-				line = fmt.Sprintf("%s=%s", change.Name, change.Value)
+				value := strings.SplitN(line, "=", 2)[1]
+				// updated is used later to see if you had the originally requested
+				// value.
 				updated = append(updated, change)
+				if value != change.Value {
+					line = fmt.Sprintf("%s=%s", change.Name, change.Value)
+					updateNeeded = true
+				}
 			}
 		}
 		new = append(new, line)
@@ -383,6 +394,8 @@
 		}
 
 		if !got {
+			updateNeeded = true
+
 			// name/value pair did not exist in original
 			// file, so append
 			lines = append(lines, fmt.Sprintf("%s=%s",
@@ -390,7 +403,11 @@
 		}
 	}
 
-	return atomicFileUpdate(file, lines)
+	if updateNeeded {
+		return atomicFileUpdate(file, lines)
+	}
+
+	return nil
 }
 
 func (u *uboot) AdditionalBindMounts() []string {

=== modified file 'partition/bootloader_uboot_test.go'
--- partition/bootloader_uboot_test.go	2015-05-15 13:33:27 +0000
+++ partition/bootloader_uboot_test.go	2015-05-21 17:13:00 +0000
@@ -29,6 +29,8 @@
 	"launchpad.net/snappy/helpers"
 )
 
+// TODO move to uboot specific test suite.
+
 const fakeUbootEnvData = `
 # This is a snappy variables and boot logic file and is entirely generated and
 # managed by Snappy. Modifications may break boot
@@ -291,3 +293,40 @@
 	c.Assert(strings.Contains(string(bytes), "snappy_mode=regular"), Equals, true)
 	c.Assert(strings.Contains(string(bytes), "snappy_ab=a"), Equals, true)
 }
+
+func (s *PartitionTestSuite) TestNoWriteNotNeeded(c *C) {
+	s.makeFakeUbootEnv(c)
+
+	atomiCall := false
+	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
+
+	partition := New()
+	u := newUboot(partition)
+	c.Assert(u, NotNil)
+
+	c.Check(u.MarkCurrentBootSuccessful(), IsNil)
+	c.Assert(atomiCall, Equals, false)
+}
+
+func (s *PartitionTestSuite) TestWriteDueToMissingValues(c *C) {
+	s.makeFakeUbootEnv(c)
+
+	// this file needs specific data
+	c.Assert(ioutil.WriteFile(bootloaderUbootEnvFile, []byte(""), 0644), IsNil)
+
+	atomiCall := false
+	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
+
+	partition := New()
+	u := newUboot(partition)
+	c.Assert(u, NotNil)
+
+	c.Check(u.MarkCurrentBootSuccessful(), IsNil)
+	c.Assert(atomiCall, Equals, true)
+
+	bytes, err := ioutil.ReadFile(bootloaderUbootEnvFile)
+	c.Assert(err, IsNil)
+	c.Check(strings.Contains(string(bytes), "snappy_mode=try"), Equals, false)
+	c.Check(strings.Contains(string(bytes), "snappy_mode=regular"), Equals, true)
+	c.Check(strings.Contains(string(bytes), "snappy_ab=a"), Equals, true)
+}