Merge lp:~mvo/snappy/snappy-atomic-write-once into lp:~snappy-dev/snappy/snappy-moved-to-github

[Michael Vogt]

Refactor partition code to use helpers.AtomicWrite() instead of duplicating that code.

This branch refactors the partition code a bit to use helpers.AtomicWriteFile() so that we only have a single implementation of this and not duplicate code.

[Sergio Schvezov (sergiusens)]

Looks good, but I didn't do any live testing so not top approving

review: Approve

[Snappy Tarmac (snappydevtarmac)]

Attempt to merge into lp:snappy failed due to conflicts:

text conflict in partition/bootloader_uboot.go

[lp:~mvo/snappy/snappy-atomic-write-once]

573. By Michael Vogt on 2015-07-24

merged lp:snappy and resolved conflicts

574. By Michael Vogt on 2015-07-24

add error checking to all writes

[Michael Vogt (mvo)]

review: Approve

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-07-23 18:10:51 +0000
+++ partition/bootloader_uboot.go	2015-07-24 12:00:08 +0000
@@ -21,9 +21,9 @@
 
 import (
 	"bufio"
+	"bytes"
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
 
 	"launchpad.net/snappy/helpers"
@@ -56,7 +56,8 @@
 	bootloaderUbootStampFile  = bootloaderUbootStampFileReal
 	bootloaderUbootEnvFile    = bootloaderUbootEnvFileReal
 	bootloaderUbootFwEnvFile  = bootloaderUbootFwEnvFileReal
-	atomicFileUpdate          = atomicFileUpdateImpl
+
+	atomicWriteFile = helpers.AtomicWriteFile
 )
 
 const bootloaderNameUboot bootloaderName = "u-boot"
@@ -198,30 +199,6 @@
 	return bootloaderUbootDir
 }
 
-// Write lines to file atomically. File does not have to preexist.
-// FIXME: put into utils package
-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
-	dir, err := os.Open(filepath.Dir(file))
-	if err != nil {
-		return err
-	}
-	defer dir.Close()
-
-	if err := writeLines(lines, tmpFile); err != nil {
-		return err
-	}
-
-	// atomic update
-	if err := os.Rename(tmpFile, file); err != nil {
-		return err
-	}
-
-	return dir.Sync()
-}
-
 // Rewrite the specified file, applying the specified set of changes.
 // Lines not in the changes slice are left alone.
 // If the original file does not contain any of the name entries (from
@@ -230,19 +207,22 @@
 //
 // FIXME: put into utils package
 // FIXME: improve logic
-func modifyNameValueFile(file string, changes []configFileChange) (err error) {
+func modifyNameValueFile(path string, changes []configFileChange) error {
 	var updated []configFileChange
 
-	lines, err := readLines(file)
-	if err != nil {
-		return err
-	}
-
-	var new []string
 	// we won't write to a file if we don't need to.
 	updateNeeded := false
 
-	for _, line := range lines {
+	file, err := os.Open(path)
+	if err != nil {
+		return err
+	}
+	defer file.Close()
+
+	buf := bytes.NewBuffer(nil)
+	scanner := bufio.NewScanner(file)
+	for scanner.Scan() {
+		line := scanner.Text()
 		for _, change := range changes {
 			if strings.HasPrefix(line, fmt.Sprintf("%s=", change.Name)) {
 				value := strings.SplitN(line, "=", 2)[1]
@@ -255,11 +235,11 @@
 				}
 			}
 		}
-		new = append(new, line)
+		if _, err := fmt.Fprintln(buf, line); err != nil {
+			return err
+		}
 	}
 
-	lines = new
-
 	for _, change := range changes {
 		got := false
 		for _, update := range updated {
@@ -274,64 +254,15 @@
 
 			// name/value pair did not exist in original
 			// file, so append
-			lines = append(lines, fmt.Sprintf("%s=%s",
-				change.Name, change.Value))
+			if _, err := fmt.Fprintf(buf, "%s=%s\n", change.Name, change.Value); err != nil {
+				return err
+			}
 		}
 	}
 
 	if updateNeeded {
-		return atomicFileUpdate(file, lines)
+		return atomicWriteFile(path, buf.Bytes(), 0644)
 	}
 
 	return nil
 }
-
-// FIXME: put into utils package
-func readLines(path string) (lines []string, err error) {
-
-	file, err := os.Open(path)
-
-	if err != nil {
-		return nil, err
-	}
-
-	defer file.Close()
-
-	scanner := bufio.NewScanner(file)
-	for scanner.Scan() {
-		lines = append(lines, scanner.Text())
-	}
-
-	return lines, scanner.Err()
-}
-
-// FIXME: put into utils package
-func writeLines(lines []string, path string) (err error) {
-
-	file, err := os.Create(path)
-
-	if err != nil {
-		return err
-	}
-
-	defer func() {
-		e := file.Close()
-		if err == nil {
-			err = e
-		}
-	}()
-
-	writer := bufio.NewWriter(file)
-
-	for _, line := range lines {
-		if _, err := fmt.Fprintln(writer, line); err != nil {
-			return err
-		}
-	}
-
-	if err := writer.Flush(); err != nil {
-		return err
-	}
-
-	return file.Sync()
-}

=== modified file 'partition/bootloader_uboot_test.go'
--- partition/bootloader_uboot_test.go	2015-07-23 18:10:51 +0000
+++ partition/bootloader_uboot_test.go	2015-07-24 12:00:08 +0000
@@ -291,7 +291,10 @@
 	s.makeFakeUbootEnv(c)
 
 	atomiCall := false
-	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
+	atomicWriteFile = func(a string, b []byte, c os.FileMode) error {
+		atomiCall = true
+		return helpers.AtomicWriteFile(a, b, c)
+	}
 
 	partition := New()
 	u := newUboot(partition)
@@ -308,7 +311,10 @@
 	c.Assert(ioutil.WriteFile(bootloaderUbootEnvFile, []byte(""), 0644), IsNil)
 
 	atomiCall := false
-	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
+	atomicWriteFile = func(a string, b []byte, c os.FileMode) error {
+		atomiCall = true
+		return helpers.AtomicWriteFile(a, b, c)
+	}
 
 	partition := New()
 	u := newUboot(partition)

=== modified file 'partition/utils.go'
--- partition/utils.go	2015-06-11 08:26:37 +0000
+++ partition/utils.go	2015-07-24 12:00:08 +0000
@@ -26,14 +26,6 @@
 	"strings"
 )
 
-// Run the commandline specified by the args array chrooted to the given dir
-var runInChroot = func(chrootDir string, args ...string) (err error) {
-	fullArgs := []string{"/usr/sbin/chroot", chrootDir}
-	fullArgs = append(fullArgs, args...)
-
-	return runCommand(fullArgs...)
-}
-
 // FIXME: would it make sense to differenciate between launch errors and
 //        exit code? (i.e. something like (returnCode, error) ?)
 func runCommandImpl(args ...string) (err error) {