Merge lp:~sergiusens/snappy/trunkBackport470 into lp:~snappy-dev/snappy/15.04-deprecated

[Sergio Schvezov]

Backport revno 470 from trunk: Avoid writing to snappy-system.txt if not required

bzr merge -c 470 lp:snappy

[Sergio Schvezov (sergiusens)]

All good, what could possibly go wrong

[Michael Vogt (mvo)]

Looks good, thanks!

review: Approve

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-04-30 11:28:39 +0000
+++ partition/bootloader_uboot.go	2015-06-02 20:27:20 +0000
@@ -51,6 +51,7 @@
 	bootloaderUbootConfigFile = bootloaderUbootConfigFileReal
 	bootloaderUbootStampFile  = bootloaderUbootStampFileReal
 	bootloaderUbootEnvFile    = bootloaderUbootEnvFileReal
+	atomicFileUpdate          = atomicFileUpdateImpl
 )
 
 const bootloaderNameUboot bootloaderName = "u-boot"
@@ -64,6 +65,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
@@ -320,7 +322,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
@@ -349,6 +351,7 @@
 // appended to the file.
 //
 // FIXME: put into utils package
+// FIXME: improve logic
 func modifyNameValueFile(file string, changes []configFileChange) (err error) {
 	var updated []configFileChange
 
@@ -358,12 +361,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)
@@ -381,6 +392,8 @@
 		}
 
 		if !got {
+			updateNeeded = true
+
 			// name/value pair did not exist in original
 			// file, so append
 			lines = append(lines, fmt.Sprintf("%s=%s",
@@ -388,7 +401,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-04-30 11:28:39 +0000
+++ partition/bootloader_uboot_test.go	2015-06-02 20:27:20 +0000
@@ -27,6 +27,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
@@ -289,3 +291,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)
+}