[Merge] lp:~sergiusens/goget-ubuntu-touch/beBetter into lp:goget-ubuntu-touch
Michael Vogt
michael.vogt at canonical.com
Fri Jun 12 13:01:18 UTC 2015
Review: Approve
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.
Diff comments:
> === modified file 'diskimage/common.go'
> --- diskimage/common.go 2015-04-21 19:35:46 +0000
> +++ diskimage/common.go 2015-06-12 05:18:02 +0000
> @@ -8,6 +8,7 @@
> package diskimage
>
> import (
> + "bufio"
> "errors"
> "fmt"
> "io/ioutil"
> @@ -149,40 +150,234 @@
> 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
> + return err
> }
>
> // We change the mode so snappy can unpack as non root
> if err := os.Chmod(baseMount, 0755); err != nil {
If I read this right, then if this fails, the tempdir is not cleaned up?
> - return "", err
> + return err
> }
>
> //Remove Mountpoint if we fail along the way
> defer func() {
> if err != nil {
Why is it removed on non-err? What cleans it up in this case? Maybe a small comment for poor souls like me? Or in the comment above. Something like " //Remove Mountpoint if we fail along the way otherwise FuncFoobar will use it and cleanup afterwards"
> - os.Remove(baseMount)
> + os.RemoveAll(baseMount)
Is this dangerous? Suppose the mountpoint is not unmounted due to $badstars it will remove the sd card content? I guess that does not matter because its just incomplete stuff anway(?).
> }
> }()
>
> - for _, part := range partitions {
> + for _, part := range img.parts {
> if part.fs == fsNone {
> continue
> }
>
> mountpoint := filepath.Join(baseMount, string(part.dir))
Just curious, but why does "part.dir" needs a conversion to string()?
> if err := os.MkdirAll(mountpoint, 0755); err != nil {
> - return "", err
> + return err
> }
> if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
You could use "syscall.Mount()" here (but I guess that (much?) more work.
> - return "", fmt.Errorf("unable to mount dir to create system image: %s", out)
> - }
> - }
> -
> - return baseMount, nil
> -
> + return fmt.Errorf("unable to mount dir to create system image: %s", out)
> + }
> + }
> + 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")
> + }
> +
> + if out, err := exec.Command("sync").CombinedOutput(); err != nil {
You could use "syscall.Sync()" instead (if you want).
> + 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", mountpoint).CombinedOutput(); err != nil {
You could use syscall.Unmount() here
> + lsof, _ := exec.Command("lsof", "-w", mountpoint).CombinedOutput()
> + printOut(string(lsof))
> + return fmt.Errorf("unable to unmount dir for image: %s", 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)
It is a bit sad that this is the "interface" to talk to kpartx :/ Oh well (and not your fault of course).
> + stdout, err := kpartxCmd.StdoutPipe()
> + if err != nil {
> + return err
> + }
> +
> + if err := kpartxCmd.Start(); err != nil {
> + return err
> + }
> +
> + loops := make([]string, 0, img.partCount)
nice
> + 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")
Maybe print the problematic line here in the error message? Like "issues while determining drive mappings, can not parse %q" or something?
> + }
> + }
> + if err := scanner.Err(); err != nil {
> + return err
> + }
> +
> + if len(loops) != img.partCount {
> + return errors.New("more partitions then expected while creating loop mapping")
I ran into this today, not sure how useful it is, but maybe output the size and the expected size here in the error too? It might give hints, e.g. if len(loops) is double (or triple) the amount of img.partCount that gives advanced users a hint that something went wrong with the unmapping (or maybe not, I don't really know what I'm talking about here). I also wonder if it should try a Unmap() in this case, but again I may be toally off here.
> + }
> +
> + 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))
What is the type of "writableDir"? Why does it need a string cast?
> +}
> +
> +//System returns the system path
> +func (img BaseImage) 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))
> +}
> +
> +// Boot returns the system-boot path
> +func (img BaseImage) 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))
Boot/System/Writable look very similar. Not sure if its worth the effort but I wonder if it could be consolidated somehow? But maybe I'm overthinking this.
> +}
> +
> +// 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-12 05:18:02 +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
Very NICE
> }
>
> 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-12 05:18:02 +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
nice
> }
>
> 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))
>
> === 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-12 05:18:02 +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)
> }
> }()
>
>
--
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/beBetter/+merge/261804
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list