Merge lp:~jamesodhunt/snappy/move-utility-functions into lp:~snappy-dev/snappy/snappy-moved-to-github

[James Hunt]

* partition/bootloader_uboot.go: Move utility functions to utils.go.
* partition.utils.go: runCommandWithStdout(): Fix bug where an
  additional empty line is included

[Michael Vogt (mvo)]

Thanks for this branch. Some comments inline (I copied them from https://code.launchpad.net/~jamesodhunt/snappy/add-lsblk-cache/+merge/248245 I hope I did not forgot a relevant one).

I also feel that the new functions in utils.go needs tests. It does not have to be done in this MP and I'm happy to merge this once my questions below are answered but I think we should add them soon.

[Sergio Schvezov (sergiusens)]

I added a couple of inline comments and have additional comments here:

* it doesn't seem hard to add tests, can you add them?
* it seems the original intention was to move this to a package itself, not a compilation unit, what made you make your mind up on putting it into the same package?

Thanks.

review: Needs Fixing

[Sergio Schvezov (sergiusens)]

One more inline comment about the output last line trim.

review: Needs Fixing

[Sergio Schvezov (sergiusens)]

I set this to work in progress to clear out the review queue

Unmerged revisions

135. By James Hunt on 2015-02-03

* partition/bootloader_uboot.go: Move utility functions to utils.go.
* partition.utils.go: runCommandWithStdout(): Fix bug where an
  additional empty line is included in the output.

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-01-27 08:25:19 +0000
+++ partition/bootloader_uboot.go	2015-02-03 09:40:51 +0000
@@ -5,7 +5,6 @@
 package partition
 
 import (
-	"bufio"
 	"fmt"
 	"os"
 	"path"
@@ -172,46 +171,6 @@
 	return u.otherRootfs
 }
 
-// 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 file.Close()
-
-	writer := bufio.NewWriter(file)
-
-	for _, line := range lines {
-		if _, err = fmt.Fprintln(writer, line); err != nil {
-			return err
-		}
-	}
-	return writer.Flush()
-}
-
 // Returns name=value entries from the specified file, removing all
 // blank lines and comments.
 func getNameValuePairs(file string) (vars []string, err error) {
@@ -379,68 +338,3 @@
 	return err
 }
 
-// Write lines to file atomically. File does not have to preexist.
-// FIXME: put into utils package
-func atomicFileUpdate(file string, lines []string) (err error) {
-	tmpFile := fmt.Sprintf("%s.NEW", file)
-
-	if err := writeLines(lines, tmpFile); err != nil {
-		return err
-	}
-
-	// atomic update
-	if err = os.Rename(tmpFile, file); err != nil {
-		return err
-	}
-
-	return err
-}
-
-// 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
-// the corresponding ConfigFileChange objects), those entries are
-// appended to the file.
-//
-// FIXME: put into utils package
-func modifyNameValueFile(file string, changes []ConfigFileChange) (err error) {
-	var lines []string
-	var updated []ConfigFileChange
-
-	if lines, err = readLines(file); err != nil {
-		return err
-	}
-
-	var new []string
-
-	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)
-				updated = append(updated, change)
-			}
-		}
-		new = append(new, line)
-	}
-
-	lines = new
-
-	for _, change := range changes {
-		var got bool = false
-		for _, update := range updated {
-			if update.Name == change.Name {
-				got = true
-				break
-			}
-		}
-
-		if got == false {
-			// name/value pair did not exist in original
-			// file, so append
-			lines = append(lines, fmt.Sprintf("%s=%s",
-				change.Name, change.Value))
-		}
-	}
-
-	return atomicFileUpdate(file, lines)
-}

=== modified file 'partition/utils.go'
--- partition/utils.go	2015-01-27 07:59:45 +0000
+++ partition/utils.go	2015-02-03 09:40:51 +0000
@@ -6,6 +6,7 @@
 	"os"
 	"os/exec"
 	"strings"
+	"bufio"
 )
 
 // Return true if given path exists.
@@ -75,5 +76,126 @@
 
 	output = strings.Split(string(bytes), "\n")
 
+	// remove last element if it's empty
+	if len(output) > 1 {
+		last := output[len(output)-1]
+		if last == "" {
+			output = output[:len(output)-1]
+		}
+	}
+
 	return output, err
 }
+
+// Return a string slice of all lines in file specified by path 
+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()
+}
+
+// Write lines slice to file specified by path
+func writeLines(lines []string, path string) (err error) {
+
+	file, err := os.Create(path)
+
+	if err != nil {
+		return err
+	}
+
+	defer file.Close()
+
+	writer := bufio.NewWriter(file)
+
+	for _, line := range lines {
+		if _, err = fmt.Fprintln(writer, line); err != nil {
+			return err
+		}
+	}
+	return writer.Flush()
+}
+
+// Write lines to file atomically. File does not have to preexist.
+func atomicFileUpdate(file string, lines []string) (err error) {
+	tmpFile := fmt.Sprintf("%s.NEW", file)
+
+	if err := writeLines(lines, tmpFile); err != nil {
+		return err
+	}
+
+	// atomic update
+	if err = os.Rename(tmpFile, file); err != nil {
+		return err
+	}
+
+	return err
+}
+
+// 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
+// the corresponding ConfigFileChange objects), those entries are
+// appended to the file.
+//
+func modifyNameValueFile(file string, changes []ConfigFileChange) (err error) {
+	var lines []string
+	var updated []ConfigFileChange
+
+	if lines, err = readLines(file); err != nil {
+		return err
+	}
+
+	var new []string
+
+	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)
+				updated = append(updated, change)
+			}
+		}
+		new = append(new, line)
+	}
+
+	lines = new
+
+	for _, change := range changes {
+		var got bool = false
+		for _, update := range updated {
+			if update.Name == change.Name {
+				got = true
+				break
+			}
+		}
+
+		if got == false {
+			// name/value pair did not exist in original
+			// file, so append
+			lines = append(lines, fmt.Sprintf("%s=%s",
+				change.Name, change.Value))
+		}
+	}
+
+	return atomicFileUpdate(file, lines)
+}
+
+func getLsblkOutput() ([]string, error) {
+	return runCommandWithStdout(
+		"/bin/lsblk",
+		"--ascii",
+		"--output=NAME,LABEL,PKNAME,MOUNTPOINT",
+		"--pairs")
+}
+