On Oct 22, 2015 4:28 AM, "Michael Vogt" <email address hidden> wrote:
>
> Just added some comments so that we don't forget them :)
>
> Diff comments:
>
> > === modified file 'diskimage/common.go'
> > --- diskimage/common.go       2015-09-09 14:36:46 +0000
> > +++ diskimage/common.go       2015-10-22 07:16:55 +0000
> > @@ -190,16 +190,19 @@
> >
> >  // 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
> > -     rootSize  int
> > +     baseMount  string
> > +     bindMounts []string
> > +     hardware   HardwareDescription
> > +     location   string
> > +     oem        OemDescription
> > +     parts      []partition
> > +     partCount  int
> > +     size       int64
> > +     rootSize   int
> >  }
> >
> > +var bindMounts = []string{"dev", "sys", "proc", filepath.Join("sys",
"firmware")}
>
> It looks like we are doing this twice (once here, once in snappy).

You will see a bunch of these little things which could go away when
migrating the provisioner into snappy itself.
>
> > +
> >  // Mount mounts the image. This also maps the loop device.
> >  func (img *BaseImage) Mount() error {
> >       if err := img.doMap(); err != nil {
> >
> > === modified file 'ubuntu-device-flash/snappy.go'
> > --- ubuntu-device-flash/snappy.go     2015-09-16 11:13:20 +0000
> > +++ ubuntu-device-flash/snappy.go     2015-10-22 07:16:55 +0000
> > @@ -339,7 +343,21 @@
> >       return hw, err
> >  }
> >
> > -func (s *Snapper) setup(filePathChan <-chan string, fileCount int)
error {
> > +func (s *Snapper) bindMount(d string) (string, error) {
>
> I gues the name could be better, something like bindMountInWritable or so
>
> > +     src := filepath.Join(s.img.Writable(), "system-data", d)
> > +     dst := filepath.Join(s.img.System(), d)
> > +
> > +     if err := os.MkdirAll(src, 0755); err != nil {
> > +             return "", err
> > +     }
> > +     cmd := exec.Command("mount", "--bind", src, dst)
> > +     if o, err := cmd.CombinedOutput(); err != nil {
> > +             return "", fmt.Errorf("os snap mount failed with: %s %v
", err, string(o))
> > +     }
> > +     return dst, nil
> > +}
> > +
> > +func (s *Snapper) setup(systemImageFiles []Files) error {
> >       printOut("Mounting...")
> >       if err := s.img.Mount(); err != nil {
> >               return err
> > @@ -362,14 +379,82 @@
> >
> >       systemPath := s.img.System()
> >
> > +     // setup a fake system
> > +     if s.oem.PartitionLayout() == "minimal" {
> > +             if err := os.MkdirAll(systemPath, 0755); err != nil {
> > +                     return err
> > +             }
> > +             // mount os snap
> > +             cmd := exec.Command("mount", s.OS, systemPath)
> > +             if o, err := cmd.CombinedOutput(); err != nil {
> > +                     return fmt.Errorf("os snap mount failed with: %s
%v ", err, string(o))
> > +             }
> > +             defer exec.Command("umount", systemPath).Run()
> > +
> > +             // we need to do what "writable-paths" normally does on
> > +             // boot for etc/systemd/system, i.e. copy all the stuff
> > +             // from the os into the writable partition. normally
> > +             // this is the job of the initrd, however it won't touch
> > +             // the dir if there are files in there already. and a
> > +             // kernel/os install will create auto-mount units in there
> > +             src := filepath.Join(systemPath, "etc", "systemd",
"system")
> > +             dst := filepath.Join(s.img.Writable(), "system-data",
"etc", "systemd")
> > +             if err := os.MkdirAll(dst, 0755); err != nil {
> > +                     return err
> > +             }
> > +             cmd = exec.Command("cp", "-a", src, dst)
> > +             if o, err := cmd.CombinedOutput(); err != nil {
> > +                     return fmt.Errorf("copy failed: %s %s", err, o)
> > +             }
> > +
> > +             // bind mount all relevant dirs
> > +             for _, d := range []string{"apps", "oem",
"var/lib/snappy", "var/lib/apps", "etc/systemd/system/", "tmp"} {
> > +                     dst, err := s.bindMount(d)
> > +                     if err != nil {
> > +                             return err
> > +                     }
> > +                     defer exec.Command("umount", dst).Run()
> > +             }
> > +
> > +             // bind mount /boot/efi
> > +             dst = filepath.Join(systemPath, "/boot/efi")
> > +             cmd = exec.Command("mount", "--bind", s.img.Boot(), dst)
> > +             if o, err := cmd.CombinedOutput(); err != nil {
> > +                     return fmt.Errorf("boot bind mount failed with:
%s %v ", err, string(o))
> > +             }
> > +             defer exec.Command("umount", dst).Run()
> > +
> > +             // grub needs this
> > +             grubUbuntu := filepath.Join(s.img.Boot(),
"EFI/ubuntu/grub")
> > +             os.MkdirAll(grubUbuntu, 0755)
> > +
> > +             // and /boot/grub
> > +             src = grubUbuntu
> > +             dst = filepath.Join(systemPath, "/boot/grub")
> > +             cmd = exec.Command("mount", "--bind", src, dst)
> > +             if o, err := cmd.CombinedOutput(); err != nil {
> > +                     return fmt.Errorf("boot/ubuntu bind mount failed
with: %s %v ", err, string(o))
> > +             }
> > +             defer exec.Command("umount", dst).Run()
> > +
> > +             // TERRIBLE but we need a /boot/grub/grub.cfg so that
>
> It looks like this is no longer needed, I removed it and its still fine
it seems.

Right. GenericBootSetup should be taking care of this.

>
> > +             //          the kernel and os snap can be installed
> > +             oemGrubCfg := filepath.Join(s.stagingRootPath, "oem",
"generic-amd64", "current", "grub.cfg")
> > +             cmd = exec.Command("cp", oemGrubCfg, grubUbuntu)
> > +             o, err := cmd.CombinedOutput()
> > +             if err != nil {
> > +                     return fmt.Errorf("failed to copy %s %s", err, o)
> > +             }
> > +     }
> > +
> > +     if err := s.img.SetupBoot(); err != nil {
> > +             return err
> > +     }
> > +
> >       if err := s.install(systemPath); err != nil {
> >               return err
> >       }
> >
> > -     if err := s.img.SetupBoot(); err != nil {
> > -             return err
> > -     }
> > -
> >       for i := range s.customizationFunc {
> >               if err := s.customizationFunc[i](); err != nil {
> >                       return err
>
>
> --
>
https://code.launchpad.net/~snappy-dev/goget-ubuntu-touch/all-snaps/+merge/275273
> Your team Snappy Developers is subscribed to branch
lp:~snappy-dev/goget-ubuntu-touch/all-snaps.
>
> Launchpad-Message-Rationale: Subscriber @snappy-dev
> Launchpad-Message-For: snappy-dev
> Launchpad-Notification-Type: code-review
> Launchpad-Branch: ~snappy-dev/goget-ubuntu-touch/all-snaps
> Launchpad-Project: goget-ubuntu-touch