Merge lp:~sergiusens/goget-ubuntu-touch/beBetter into lp:goget-ubuntu-touch

[Sergio Schvezov]

Remove code duplication and manage mounting/unmounting better

[lp:~sergiusens/goget-ubuntu-touch/beBetter]

187. By Sergio Schvezov on 2015-06-12

Revert uneeded func name change

[Michael Vogt (mvo)]

Woah, very nice consolidation. I have a lot of questions in the begining probably because its very hot here. Feel free to ignore, this is great and fine to land - I like how it kills duplication.

review: Approve

[lp:~sergiusens/goget-ubuntu-touch/beBetter]

188. By Sergio Schvezov on 2015-06-15

Try to cleanly unmount on mount errors

189. By Sergio Schvezov on 2015-06-15

Message about not being able to remove basemount

190. By Sergio Schvezov on 2015-06-15

Use syscall sync

191. By Sergio Schvezov on 2015-06-15

Nicer error for unmounting

192. By Sergio Schvezov on 2015-06-15

Adding errors.go

193. By Sergio Schvezov on 2015-06-15

Mapping errors

194. By Sergio Schvezov on 2015-06-15

removing duplicate code for paths to mounts

195. By Sergio Schvezov on 2015-06-15

Cleanup moved

[Michael Vogt (mvo)]

Nice!

review: Approve

[John Lenton (chipaca)]

LGTM.

review: Approve

=== modified file 'diskimage/common.go'
--- diskimage/common.go	2015-04-21 19:35:46 +0000
+++ diskimage/common.go	2015-06-15 15:32:50 +0000
@@ -8,6 +8,7 @@
 package diskimage
 
 import (
+	"bufio"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -15,6 +16,7 @@
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"syscall"
 )
 
 // This program is free software: you can redistribute it and/or modify it
@@ -37,6 +39,10 @@
 	}
 }
 
+var (
+	syscallSync = syscall.Sync
+)
+
 type Image interface {
 	Mount() error
 	Unmount() error
@@ -149,40 +155,247 @@
 	return strings.TrimSpace(string(out)), err
 }
 
-func mount(partitions []partition) (baseMount string, err error) {
-	baseMount, err = ioutil.TempDir(os.TempDir(), "diskimage")
+// BaseImage implements the basic primitives to manage images.
+type BaseImage struct {
+	baseMount string
+	hardware  HardwareDescription
+	location  string
+	oem       OemDescription
+	parts     []partition
+	partCount int
+	size      int64
+}
+
+// Mount mounts the image. This also maps the loop device.
+func (img *BaseImage) Mount() error {
+	if err := img.Map(); err != nil {
+		return err
+	}
+
+	baseMount, err := ioutil.TempDir(os.TempDir(), "diskimage")
 	if err != nil {
-		return "", err
-	}
-
-	// We change the mode so snappy can unpack as non root
-	if err := os.Chmod(baseMount, 0755); err != nil {
-		return "", err
+		return err
 	}
 
 	//Remove Mountpoint if we fail along the way
 	defer func() {
 		if err != nil {
-			os.Remove(baseMount)
+			if err := os.Remove(baseMount); err != nil {
+				fmt.Println("WARNING: cannot remove", baseMount, "due to", err)
+			}
 		}
 	}()
 
-	for _, part := range partitions {
+	// We change the mode so snappy can unpack as non root
+	if err := os.Chmod(baseMount, 0755); err != nil {
+		return err
+	}
+
+	for _, part := range img.parts {
 		if part.fs == fsNone {
 			continue
 		}
 
 		mountpoint := filepath.Join(baseMount, string(part.dir))
 		if err := os.MkdirAll(mountpoint, 0755); err != nil {
-			return "", err
-		}
-		if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
-			return "", fmt.Errorf("unable to mount dir to create system image: %s", out)
-		}
-	}
-
-	return baseMount, nil
-
+			return err
+		}
+
+		dev := filepath.Join("/dev/mapper", part.loop)
+		printOut("Mounting", dev, part.fs, "to", mountpoint)
+		if out, errMount := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); errMount != nil {
+			return ErrMount{dev: dev, mountpoint: mountpoint, fs: part.fs, out: out}
+		}
+		// this is cleanup in case one of the mounts fail
+		defer func() {
+			if err != nil {
+				if err := exec.Command("umount", mountpoint).Run(); err != nil {
+					fmt.Println("WARNING:", mountpoint, "could not be unmounted")
+					return
+				}
+
+				if err := os.Remove(mountpoint); err != nil {
+					fmt.Println("WARNING: could not remove ", mountpoint)
+				}
+			}
+		}()
+	}
+	img.baseMount = baseMount
+
+	return nil
+
+}
+
+// Unmount unmounts the image. This also unmaps the loop device.
+func (img *BaseImage) Unmount() error {
+	if img.baseMount == "" {
+		panic("No base mountpoint set")
+	}
+
+	syscallSync()
+
+	for _, part := range img.parts {
+		if part.fs == fsNone {
+			continue
+		}
+
+		mountpoint := filepath.Join(img.baseMount, string(part.dir))
+		if out, err := exec.Command("umount", mountpoint).CombinedOutput(); err != nil {
+			lsof, _ := exec.Command("lsof", "-w", mountpoint).CombinedOutput()
+			printOut(string(lsof))
+			dev := filepath.Join("/dev/mapper", part.loop)
+			return ErrMount{dev: dev, mountpoint: mountpoint, fs: part.fs, out: out}
+		}
+	}
+
+	if err := os.RemoveAll(img.baseMount); err != nil {
+		return err
+	}
+	img.baseMount = ""
+
+	return img.Unmap()
+}
+
+// Map maps the image to loop devices
+func (img *BaseImage) Map() error {
+	if isMapped(img.parts) {
+		panic("cannot double map partitions")
+	}
+
+	kpartxCmd := exec.Command("kpartx", "-avs", img.location)
+	stdout, err := kpartxCmd.StdoutPipe()
+	if err != nil {
+		return err
+	}
+
+	if err := kpartxCmd.Start(); err != nil {
+		return err
+	}
+
+	loops := make([]string, 0, img.partCount)
+	scanner := bufio.NewScanner(stdout)
+	for scanner.Scan() {
+		fields := strings.Fields(scanner.Text())
+
+		if len(fields) > 2 {
+			loops = append(loops, fields[2])
+		} else {
+			return fmt.Errorf("issues while determining drive mappings (%q)", fields)
+		}
+	}
+	if err := scanner.Err(); err != nil {
+		return err
+	}
+
+	if len(loops) != img.partCount {
+		return ErrMapCount{expectedParts: img.partCount, foundParts: len(loops)}
+	}
+
+	mapPartitions(img.parts, loops)
+
+	if err := kpartxCmd.Wait(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+//Unmap destroys loop devices for the partitions
+func (img *BaseImage) Unmap() error {
+	if img.baseMount != "" {
+		panic("cannot unmap mounted partitions")
+	}
+
+	for _, part := range img.parts {
+		if err := exec.Command("dmsetup", "clear", part.loop).Run(); err != nil {
+			return err
+		}
+	}
+
+	if err := exec.Command("kpartx", "-ds", img.location).Run(); err != nil {
+		return err
+	}
+
+	unmapPartitions(img.parts)
+
+	return nil
+}
+
+// Format formats the image following the partition types and labels them
+// accordingly.
+func (img BaseImage) Format() error {
+	for _, part := range img.parts {
+		dev := filepath.Join("/dev/mapper", part.loop)
+
+		if part.fs == fsFat32 {
+			cmd := []string{"-F", "32", "-n", string(part.label)}
+
+			size, err := sectorSize(dev)
+			if err != nil {
+				return err
+			}
+
+			if size != "512" {
+				cmd = append(cmd, "-s", "1")
+			}
+
+			cmd = append(cmd, "-S", size, dev)
+
+			if out, err := exec.Command("mkfs.vfat", cmd...).CombinedOutput(); err != nil {
+				return fmt.Errorf("unable to create filesystem: %s", out)
+			}
+		} else {
+			if out, err := exec.Command("mkfs.ext4", "-F", "-L", string(part.label), dev).CombinedOutput(); err != nil {
+				return fmt.Errorf("unable to create filesystem: %s", out)
+			}
+		}
+	}
+
+	return nil
+}
+
+// User returns the writable path
+func (img BaseImage) Writable() string {
+	if img.parts == nil {
+		panic("img is not setup with partitions")
+	}
+
+	if img.baseMount == "" {
+		panic("img not mounted")
+	}
+
+	return filepath.Join(img.baseMount, string(writableDir))
+}
+
+func (img BaseImage) pathToMount(dir directory) string {
+	if img.parts == nil {
+		panic("img is not setup with partitions")
+	}
+
+	if img.baseMount == "" {
+		panic("img not mounted")
+	}
+
+	return filepath.Join(img.baseMount, string(dir))
+}
+
+//System returns the system path
+func (img BaseImage) System() string {
+	return img.pathToMount(systemADir)
+}
+
+// Boot returns the system-boot path
+func (img BaseImage) Boot() string {
+	return img.pathToMount(bootDir)
+}
+
+// BaseMount returns the base directory used to mount the image partitions.
+func (img BaseImage) BaseMount() string {
+	if img.baseMount == "" {
+		panic("image needs to be mounted")
+	}
+
+	return img.baseMount
 }
 
 func printOut(args ...interface{}) {

=== modified file 'diskimage/core_grub.go'
--- diskimage/core_grub.go	2015-06-11 03:13:19 +0000
+++ diskimage/core_grub.go	2015-06-15 15:32:50 +0000
@@ -8,14 +8,12 @@
 package diskimage
 
 import (
-	"bufio"
 	"errors"
 	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"launchpad.net/goget-ubuntu-touch/sysutils"
@@ -34,21 +32,18 @@
 // with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 type CoreGrubImage struct {
-	CoreImage
-	hardware  HardwareDescription
-	oem       OemDescription
-	location  string
-	size      int64
-	baseMount string
-	parts     []partition
+	BaseImage
 }
 
 func NewCoreGrubImage(location string, size int64, hw HardwareDescription, oem OemDescription) *CoreGrubImage {
 	return &CoreGrubImage{
-		location: location,
-		size:     size,
-		hardware: hw,
-		oem:      oem,
+		BaseImage{
+			location:  location,
+			size:      size,
+			hardware:  hw,
+			oem:       oem,
+			partCount: 5,
+		},
 	}
 }
 
@@ -63,43 +58,6 @@
 configfile $prefix/grub.cfg
 `
 
-func (img *CoreGrubImage) Mount() error {
-	baseMount, err := mount(img.parts)
-	if err != nil {
-		return err
-	}
-	img.baseMount = baseMount
-
-	return nil
-}
-
-func (img *CoreGrubImage) Unmount() (err error) {
-	if img.baseMount == "" {
-		panic("No base mountpoint set")
-	}
-	defer os.Remove(img.baseMount)
-
-	if out, err := exec.Command("sync").CombinedOutput(); err != nil {
-		return fmt.Errorf("Failed to sync filesystems before unmounting: %s", out)
-	}
-
-	for _, part := range img.parts {
-		if part.fs == fsNone {
-			continue
-		}
-
-		mountpoint := filepath.Join(img.baseMount, string(part.dir))
-		if out, err := exec.Command("umount", "-l", mountpoint).CombinedOutput(); err != nil {
-			return fmt.Errorf("unable to unmount dir for image: %s", out)
-		} else {
-		}
-	}
-
-	img.baseMount = ""
-
-	return nil
-}
-
 //Partition creates a partitioned image from an img
 func (img *CoreGrubImage) Partition() error {
 	if err := sysutils.CreateEmptyFile(img.location, img.size, sysutils.GB); err != nil {
@@ -125,150 +83,6 @@
 	return parted.create(img.location)
 }
 
-//Map creates loop devices for the partitions
-func (img *CoreGrubImage) Map() error {
-	if isMapped(img.parts) {
-		panic("cannot double map partitions")
-	}
-
-	kpartxCmd := exec.Command("kpartx", "-avs", img.location)
-	stdout, err := kpartxCmd.StdoutPipe()
-	if err != nil {
-		return err
-	}
-
-	if err := kpartxCmd.Start(); err != nil {
-		return err
-	}
-
-	loops := make([]string, 0, 4)
-	scanner := bufio.NewScanner(stdout)
-	for scanner.Scan() {
-		fields := strings.Fields(scanner.Text())
-
-		if len(fields) > 2 {
-			loops = append(loops, fields[2])
-		} else {
-			return errors.New("issues while determining drive mappings")
-		}
-	}
-	if err := scanner.Err(); err != nil {
-		return err
-	}
-
-	// there are 5 partitions, so there should be five loop mounts
-	if len(loops) != 5 {
-		return errors.New("more partitions then expected while creating loop mapping")
-	}
-
-	mapPartitions(img.parts, loops)
-
-	if err := kpartxCmd.Wait(); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-//Unmap destroys loop devices for the partitions
-func (img *CoreGrubImage) Unmap() error {
-	if img.baseMount != "" {
-		panic("cannot unmap mounted partitions")
-	}
-
-	for _, part := range img.parts {
-		if err := exec.Command("dmsetup", "clear", part.loop).Run(); err != nil {
-			return err
-		}
-	}
-
-	if err := exec.Command("kpartx", "-d", img.location).Run(); err != nil {
-		return err
-	}
-
-	unmapPartitions(img.parts)
-
-	return nil
-}
-
-func (img CoreGrubImage) Format() error {
-	for _, part := range img.parts {
-		dev := filepath.Join("/dev/mapper", part.loop)
-
-		if part.fs == fsFat32 {
-			cmd := []string{"-F", "32", "-n", string(part.label)}
-
-			size, err := sectorSize(dev)
-			if err != nil {
-				return err
-			}
-
-			if size != "512" {
-				cmd = append(cmd, "-s", "1")
-			}
-
-			cmd = append(cmd, "-S", size, dev)
-
-			if out, err := exec.Command("mkfs.vfat", cmd...).CombinedOutput(); err != nil {
-				return fmt.Errorf("unable to create filesystem: %s", out)
-			}
-		} else if part.fs == fsExt4 {
-			if out, err := exec.Command("mkfs.ext4", "-F", "-L", string(part.label), dev).CombinedOutput(); err != nil {
-				return fmt.Errorf("unable to create filesystem: %s", out)
-			}
-		}
-	}
-
-	return nil
-}
-
-// User returns the writable path
-func (img CoreGrubImage) Writable() string {
-	if img.parts == nil {
-		panic("img is not setup with partitions")
-	}
-
-	if img.baseMount == "" {
-		panic("img not mounted")
-	}
-
-	return filepath.Join(img.baseMount, string(writableDir))
-}
-
-// Boot returns the system-boot path
-func (img CoreGrubImage) Boot() string {
-	if img.parts == nil {
-		panic("img is not setup with partitions")
-	}
-
-	if img.baseMount == "" {
-		panic("img not mounted")
-	}
-
-	return filepath.Join(img.baseMount, string(bootDir))
-}
-
-//System returns the system path
-func (img CoreGrubImage) System() string {
-	if img.parts == nil {
-		panic("img is not setup with partitions")
-	}
-
-	if img.baseMount == "" {
-		panic("img not mounted")
-	}
-
-	return filepath.Join(img.baseMount, string(systemADir))
-}
-
-func (img CoreGrubImage) BaseMount() string {
-	if img.baseMount == "" {
-		panic("image needs to be mounted")
-	}
-
-	return img.baseMount
-}
-
 func (img *CoreGrubImage) SetupBoot(oemRootPath string) error {
 	for _, dev := range []string{"dev", "proc", "sys"} {
 		src := filepath.Join("/", dev)

=== modified file 'diskimage/core_uboot.go'
--- diskimage/core_uboot.go	2015-06-11 03:13:19 +0000
+++ diskimage/core_uboot.go	2015-06-15 15:32:50 +0000
@@ -8,14 +8,10 @@
 package diskimage
 
 import (
-	"bufio"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
-	"strings"
 	"text/template"
 
 	"launchpad.net/goget-ubuntu-touch/sysutils"
@@ -34,14 +30,7 @@
 // with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 type CoreUBootImage struct {
-	CoreImage
-	SystemImage
-	hardware  HardwareDescription
-	oem       OemDescription
-	location  string
-	size      int64
-	baseMount string
-	parts     []partition
+	BaseImage
 }
 
 const snappySystemTemplate = `# This is a snappy variables and boot logic file and is entirely generated and
@@ -79,44 +68,14 @@
 
 func NewCoreUBootImage(location string, size int64, hw HardwareDescription, oem OemDescription) *CoreUBootImage {
 	return &CoreUBootImage{
-		hardware: hw,
-		oem:      oem,
-		location: location,
-		size:     size,
-	}
-}
-
-func (img *CoreUBootImage) Mount() error {
-	baseMount, err := mount(img.parts)
-	if err != nil {
-		return err
-	}
-	img.baseMount = baseMount
-
-	return nil
-}
-
-func (img *CoreUBootImage) Unmount() (err error) {
-	if img.baseMount == "" {
-		panic("No base mountpoint set")
-	}
-	defer func() {
-		os.Remove(img.baseMount)
-		img.baseMount = ""
-	}()
-
-	if out, err := exec.Command("sync").CombinedOutput(); err != nil {
-		return fmt.Errorf("Failed to sync filesystems before unmounting: %s", out)
-	}
-
-	for _, part := range img.parts {
-		mountpoint := filepath.Join(img.baseMount, string(part.dir))
-		if out, err := exec.Command("umount", "-l", mountpoint).CombinedOutput(); err != nil {
-			panic(fmt.Sprintf("unable to unmount dir for image: %s", out))
-		}
-	}
-
-	return nil
+		BaseImage{
+			hardware:  hw,
+			oem:       oem,
+			location:  location,
+			size:      size,
+			partCount: 4,
+		},
+	}
 }
 
 //Partition creates a partitioned image from an img
@@ -142,137 +101,6 @@
 	return parted.create(img.location)
 }
 
-//Map creates loop devices for the partitions
-func (img *CoreUBootImage) Map() error {
-	if isMapped(img.parts) {
-		panic("cannot double map partitions")
-	}
-
-	kpartxCmd := exec.Command("kpartx", "-avs", img.location)
-	stdout, err := kpartxCmd.StdoutPipe()
-	if err != nil {
-		return err
-	}
-
-	if err := kpartxCmd.Start(); err != nil {
-		return err
-	}
-
-	loops := make([]string, 0, 4)
-	scanner := bufio.NewScanner(stdout)
-	for scanner.Scan() {
-		fields := strings.Fields(scanner.Text())
-
-		if len(fields) > 2 {
-			loops = append(loops, fields[2])
-		} else {
-			return errors.New("issues while determining drive mappings")
-		}
-	}
-	if err := scanner.Err(); err != nil {
-		return err
-	}
-
-	// there are 5 partitions, so there should be five loop mounts
-	if len(loops) != 4 {
-		return errors.New("more partitions then expected while creating loop mapping")
-	}
-
-	mapPartitions(img.parts, loops)
-
-	if err := kpartxCmd.Wait(); err != nil {
-		return err
-	}
-
-	return nil
-}
-
-//Unmap destroys loop devices for the partitions
-func (img *CoreUBootImage) Unmap() error {
-	if img.baseMount != "" {
-		panic("cannot unmap mounted partitions")
-	}
-
-	for _, part := range img.parts {
-		if err := exec.Command("dmsetup", "clear", part.loop).Run(); err != nil {
-			return err
-		}
-	}
-
-	if err := exec.Command("kpartx", "-d", img.location).Run(); err != nil {
-		return err
-	}
-
-	unmapPartitions(img.parts)
-
-	return nil
-}
-
-func (img CoreUBootImage) Format() error {
-	for _, part := range img.parts {
-		dev := filepath.Join("/dev/mapper", part.loop)
-
-		if part.fs == fsFat32 {
-			cmd := []string{"-F", "32", "-n", string(part.label)}
-
-			size, err := sectorSize(dev)
-			if err != nil {
-				return err
-			}
-
-			if size != "512" {
-				cmd = append(cmd, "-s", "1")
-			}
-
-			cmd = append(cmd, "-S", size, dev)
-
-			if out, err := exec.Command("mkfs.vfat", cmd...).CombinedOutput(); err != nil {
-				return fmt.Errorf("unable to create filesystem: %s", out)
-			}
-		} else {
-			if out, err := exec.Command("mkfs.ext4", "-F", "-L", string(part.label), dev).CombinedOutput(); err != nil {
-				return fmt.Errorf("unable to create filesystem: %s", out)
-			}
-		}
-	}
-
-	return nil
-}
-
-// User returns the writable path
-func (img *CoreUBootImage) Writable() string {
-	if img.parts == nil {
-		panic("img is not setup with partitions")
-	}
-
-	if img.baseMount == "" {
-		panic("img not mounted")
-	}
-
-	return filepath.Join(img.baseMount, string(writableDir))
-}
-
-//System returns the system path
-func (img *CoreUBootImage) System() string {
-	if img.parts == nil {
-		panic("img is not setup with partitions")
-	}
-
-	if img.baseMount == "" {
-		panic("img not mounted")
-	}
-
-	return filepath.Join(img.baseMount, string(systemADir))
-}
-
-func (img CoreUBootImage) BaseMount() string {
-	if img.baseMount == "" {
-		panic("image needs to be mounted")
-	}
-
-	return img.baseMount
-}
-
 func (img CoreUBootImage) SetupBoot(oemRootPath string) error {
 	// destinations
 	bootPath := filepath.Join(img.baseMount, string(bootDir))

=== added file 'diskimage/errors.go'
--- diskimage/errors.go	1970-01-01 00:00:00 +0000
+++ diskimage/errors.go	2015-06-15 15:32:50 +0000
@@ -0,0 +1,32 @@
+//
+// diskimage - handles ubuntu disk images
+//
+// Copyright (c) 2015 Canonical Ltd.
+//
+// Written by Sergio Schvezov <[e-mail]>
+//
+package diskimage
+
+import "fmt"
+
+// ErrMount represents a mount error
+type ErrMount struct {
+	dev        string
+	mountpoint string
+	fs         fsType
+	out        []byte
+}
+
+func (e ErrMount) Error() string {
+	return fmt.Sprintf("cannot mount %s(%s) on %s: %s", e.dev, e.fs, e.mountpoint, e.out)
+}
+
+// ErrMapCount represents an error on the expected amount of partitions
+type ErrMapCount struct {
+	foundParts    int
+	expectedParts int
+}
+
+func (e ErrMapCount) Error() string {
+	return fmt.Sprintf("expected %d partitons but found %d", e.expectedParts, e.foundParts)
+}

=== modified file 'ubuntu-device-flash/core.go'
--- ubuntu-device-flash/core.go	2015-06-09 13:57:34 +0000
+++ ubuntu-device-flash/core.go	2015-06-15 15:32:50 +0000
@@ -322,24 +322,14 @@
 }
 
 func (coreCmd *CoreCmd) setup(img diskimage.CoreImage, filePathChan <-chan string, fileCount int) error {
-	printOut("Mapping...")
-	if err := img.Map(); err != nil {
-		return err
-	}
-	defer func() {
-		printOut("Unmapping...")
-		defer img.Unmap()
-	}()
-
 	printOut("Mounting...")
 	if err := img.Mount(); err != nil {
-		fmt.Println(err)
 		return err
 	}
 	defer func() {
 		printOut("Unmounting...")
 		if err := img.Unmount(); err != nil {
-			fmt.Println(err)
+			fmt.Println("WARNING: unexpected issue:", err)
 		}
 	}()