[Merge] lp:~sergiusens/goget-ubuntu-touch/efi into lp:goget-ubuntu-touch
Colin Watson
cjwatson at canonical.com
Fri Dec 5 14:14:40 UTC 2014
Review: Needs Fixing
Diff comments:
> === modified file 'diskimage/image.go'
> --- diskimage/image.go 2014-11-28 12:28:57 +0000
> +++ diskimage/image.go 2014-12-05 13:58:58 +0000
> @@ -37,12 +37,14 @@
> type directory string
>
> const (
> + bootLabel imageLabel = "boot"
> systemALabel imageLabel = "system-a"
> systemBLabel imageLabel = "system-b"
> writableLabel imageLabel = "writable"
> )
>
> const (
> + bootDir directory = "system/boot/efi"
> systemADir directory = "system"
> systemBDir directory = "system-b"
> writableDir directory = "writable"
> @@ -169,14 +171,27 @@
>
> func (img *DiskImage) coreMount() (err error) {
> for _, part := range img.parts {
> - mountpoint := filepath.Join(img.Mountpoint, string(part.dir))
> - if err := os.MkdirAll(mountpoint, 0755); err != nil {
> + if part.label != bootLabel {
> + mountpoint := filepath.Join(img.Mountpoint, 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)
> + }
> + }
> + }
> +
> + /*
> + efiMountpoint := filepath.Join(img.Mountpoint, string(img.parts[0].dir))
> + if err := os.MkdirAll(efiMountpoint, 0755); err != nil {
> return err
> }
> - if out, err := exec.Command("mount", filepath.Join("/dev/mapper", part.loop), mountpoint).CombinedOutput(); err != nil {
> + if out, err := exec.Command("mount", filepath.Join("/dev/mapper", img.parts[0].loop), efiMountpoint).CombinedOutput(); err != nil {
> return fmt.Errorf("unable to mount dir to create system image: %s", out)
> }
> - }
> + */
> +
> return nil
> }
>
> @@ -265,13 +280,15 @@
> }
>
> stdin.Write([]byte("mklabel msdos\n"))
> - stdin.Write([]byte("mkpart primary ext4 2048s 3905535s\n"))
> + stdin.Write([]byte("mkpart primary fat32 2048s 1050624s\n"))
> + stdin.Write([]byte("mkpart primary ext4 1050625s 5244929s\n"))
I think you have an off-by-one error here resulting in the second partition being misaligned. Shouldn't this be:
mkpart primary fat32 2048s 1050623s
mkpart primary ext4 1050624s 5244929s
> if dual {
> - stdin.Write([]byte("mkpart primary ext4 3905536s 7809023s\n"))
> - stdin.Write([]byte("mkpart primary ext4 7809024s -1s\n"))
> + stdin.Write([]byte("mkpart primary ext4 5244930s 9439234s\n"))
> + stdin.Write([]byte("mkpart primary ext4 9439235s -1s\n"))
Similarly here. The starting sector of each partition should be divisible by 2048 for proper alignment.
> } else {
> - stdin.Write([]byte("mkpart primary ext4 3905536s -1s\n"))
> + stdin.Write([]byte("mkpart primary ext4 5244930s -1s\n"))
> }
> + stdin.Write([]byte("set 1 esp on\n"))
> stdin.Write([]byte("set 1 boot on\n"))
> stdin.Write([]byte("unit s print\n"))
> stdin.Write([]byte("quit\n"))
> @@ -291,7 +308,7 @@
> return err
> }
>
> - loops := make([]string, 0, 3)
> + loops := make([]string, 0, 4)
> scanner := bufio.NewScanner(stdout)
> for scanner.Scan() {
> fields := strings.Fields(scanner.Text())
> @@ -306,9 +323,9 @@
> return err
> }
>
> - expectedLoops := 2
> + expectedLoops := 3
> if dual {
> - expectedLoops = 3
> + expectedLoops = 4
> }
>
> if len(loops) != expectedLoops {
> @@ -317,15 +334,17 @@
>
> if dual {
> img.parts = []partition{
> - partition{label: systemALabel, dir: systemADir, loop: loops[0]},
> - partition{label: systemBLabel, dir: systemBDir, loop: loops[1]},
> + partition{label: bootLabel, dir: bootDir, loop: loops[0]},
> + partition{label: systemALabel, dir: systemADir, loop: loops[1]},
> + partition{label: systemBLabel, dir: systemBDir, loop: loops[2]},
> + partition{label: writableLabel, dir: writableDir, loop: loops[3]},
> + }
> + } else {
> + img.parts = []partition{
> + partition{label: bootLabel, dir: bootDir, loop: loops[0]},
> + partition{label: systemALabel, dir: systemADir, loop: loops[1]},
> partition{label: writableLabel, dir: writableDir, loop: loops[2]},
> }
> - } else {
> - img.parts = []partition{
> - partition{label: systemALabel, dir: systemADir, loop: loops[0]},
> - partition{label: writableLabel, dir: writableDir, loop: loops[1]},
> - }
> }
>
> if err := kpartxCmd.Wait(); err != nil {
> @@ -346,22 +365,35 @@
> }
>
> //CreateExt4 returns a ext4 partition for a given file
> +func (img DiskImage) Format() error {
> + for _, part := range img.parts {
> + dev := filepath.Join("/dev/mapper", part.loop)
> +
> + if part.label == bootLabel {
> + if out, err := exec.Command("mkfs.vfat", "-n", string(part.label), dev).CombinedOutput(); err != nil {
Based on hard-won experience in partman-efi, I think you want this to end up as the equivalent of this (hopefully you can follow my shell-style pseudocode):
log_sector_size="$(blockdev --getss "$dev")"
if [ "$log_sector_size" = 512 ]; then
opts=
else
opts='-s 1'
fi
mkfs.vfat -F 32 -S "$log_sector_size" $opts -n "$label" "$dev"
> + 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
> +}
> +
> +//CreateExt4 returns a ext4 partition for a given file
> func (img DiskImage) CreateExt4() error {
> - if img.parts == nil {
> - if err := sysutils.CreateEmptyFile(img.path, img.size); err != nil {
> - return err
> - }
> - return exec.Command("mkfs.ext4", "-F", "-L", img.label, img.path).Run()
> - }
> -
> - for _, part := range img.parts {
> - dev := filepath.Join("/dev/mapper", part.loop)
> - 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
> + if img.parts != nil {
> + panic("creating ext4 on images with multiple parts not supported")
> + }
> +
> + if err := sysutils.CreateEmptyFile(img.path, img.size); err != nil {
> + return err
> + }
> +
> + return exec.Command("mkfs.ext4", "-F", "-L", img.label, img.path).Run()
> }
>
> //CreateVFat returns a vfat partition for a given file
>
> === modified file 'ubuntu-device-flash/core.go'
> --- ubuntu-device-flash/core.go 2014-12-04 20:56:49 +0000
> +++ ubuntu-device-flash/core.go 2014-12-05 13:58:58 +0000
> @@ -192,7 +192,7 @@
> }
> defer img.UnMapPartitions()
>
> - return img.CreateExt4()
> + return img.Format()
> }
>
> func (coreCmd *CoreCmd) setup(img *diskimage.DiskImage, filePathChan <-chan string) error {
>
--
https://code.launchpad.net/~sergiusens/goget-ubuntu-touch/efi/+merge/243682
Your team Ubuntu Phablet Team is subscribed to branch lp:goget-ubuntu-touch.
More information about the Ubuntu-reviews
mailing list