partition,snap: add support for android boot #3324

alfonsosanchezbeato · 2017-05-16T07:06:27Z

Support for android style bootloading

Rename fastboot bootloader to android-boot, as this is a more accurate description (fastboot is a protocol). android-boot is chosen because what we support is booting from devices that are configured in the usual way for Android devices, with recovery and boot partitions and boot images with format as defined in https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h

zyga

A few comments / early feedback.

zyga · 2017-05-16T07:09:38Z

partition/androidboot.go

+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2014-2017 Canonical Ltd


Nitpick: was this written in 2014?

zyga · 2017-05-16T07:10:30Z

partition/androidboot.go

+}
+
+func (a *androidboot) GetBootVars(names ...string) (map[string]string, error) {
+	out := make(map[string]string)


nitpick: move this closer to the loop that populates it.

furthermore: you know how big it needs to be (len(names)), so make it with that size already

zyga · 2017-05-16T07:12:20Z

partition/androidboot_test.go

+	. "gopkg.in/check.v1"
+)
+
+func mockAndroidbootFile(c *C, newPath string, mode os.FileMode) {


Should we not have a SetUpTest function that calls dirs.SetRootDir(c.MkDir())? This should be paired with TearDownTest that calls dirs.SetRootDir("")

zyga · 2017-05-16T07:12:31Z

partition/androidboot_test.go

+func (s *PartitionTestSuite) TestNewAndroidbootNoAndroidbootReturnsNil(c *C) {
+	s.makeFakeAndroidbootConfig(c)
+
+	dirs.GlobalRootDir = "/something/not/there"


You don't want this, see my comment above

zyga · 2017-05-16T07:13:29Z

partition/androidboot_test.go

+	c.Assert(err, IsNil)
+}
+
+func (s *PartitionTestSuite) makeFakeAndroidbootConfig(c *C) {


If this happens to be done in each test feel free to do it in SetUpTest

zyga · 2017-05-16T07:15:19Z

partition/androidbootenv/androidbootenv.go

+		return err
+	}
+
+	rawEnv := bytes.Split(buf, []byte("\n"))


I think this would be nicer with a bufio.Scanner

yeah, this whole method needs restructuring around that. Please.

zyga · 2017-05-16T07:15:50Z

partition/androidbootenv/androidbootenv.go

+		l := bytes.SplitN(env, []byte("="), 2)
+		// be liberal in what you accept
+		if len(l) < 2 {
+			continue


Nitpick: please log weird stuff with logger.Noticef

(or logger.Debugf if it's weird but harmless)

zyga · 2017-05-16T07:16:17Z

partition/androidbootenv/androidbootenv.go

+			return err
+		}
+	}
+


Is this the place to use our atomic write helpers?

also, one nice thing about bytes.Buffer is that you can just declare it and start using it; NewBuffer is only needed when you want to preload the buffer with something. Otherwise, just

var w bytes.Buffer

and you're done (although some things will need you to pass a pointer to this, e.g. things expecting an io.Writer).

chipaca

Very nice, thank you!

A bunch of small things to fix, mostly idiomatic and not conceptual.

chipaca · 2017-05-16T11:27:13Z

partition/androidboot.go

+}
+
+func (a *androidboot) GetBootVars(names ...string) (map[string]string, error) {
+	out := make(map[string]string)


furthermore: you know how big it needs to be (len(names)), so make it with that size already

chipaca · 2017-05-16T11:30:33Z

partition/androidbootenv/androidbootenv.go

+		l := bytes.SplitN(env, []byte("="), 2)
+		// be liberal in what you accept
+		if len(l) < 2 {
+			continue


(or logger.Debugf if it's weird but harmless)

chipaca · 2017-05-16T11:31:22Z

partition/androidbootenv/androidbootenv.go

+		return err
+	}
+
+	rawEnv := bytes.Split(buf, []byte("\n"))


yeah, this whole method needs restructuring around that. Please.

chipaca · 2017-05-16T11:31:46Z

partition/androidbootenv/androidbootenv.go

+			return err
+		}
+	}
+


chipaca · 2017-05-16T11:33:56Z

partition/androidboot_test.go

+ *
+ */
+
+package partition


can you make this package partition_test, please? (maybe use export_test.go to get to the private bits you need for testing)

chipaca · 2017-05-16T11:38:44Z

partition/androidboot_test.go

+
+	a := newAndroidboot()
+	bootVars := map[string]string{}
+	bootVars["snap_mode"] = "try"


you could directly

bootVars := map[string]string{"snap_mode": "try"}

chipaca · 2017-05-16T11:41:49Z

partition/androidbootenv/androidbootenv.go

+			return err
+		}
+	}
+


also, one nice thing about bytes.Buffer is that you can just declare it and start using it; NewBuffer is only needed when you want to preload the buffer with something. Otherwise, just

var w bytes.Buffer

and you're done (although some things will need you to pass a pointer to this, e.g. things expecting an io.Writer).

chipaca · 2017-05-16T11:43:22Z

partition/androidbootenv/androidbootenv_test.go

+
+func (a *androidbootenvTestSuite) TestSaveAndLoad(c *C) {
+	env := androidbootenv.NewEnv(a.envPath)
+	c.Check(env, NotNil)


here this should probably be an Assert (otherwise all the other tests will panic)

chipaca · 2017-05-16T11:43:36Z

partition/androidbootenv/androidbootenv_test.go

+
+func (a *androidbootenvTestSuite) TestSet(c *C) {
+	env := androidbootenv.NewEnv(a.envPath)
+	c.Check(env, NotNil)


... Assert :-)

chipaca · 2017-05-16T11:44:14Z

snap/gadget.go

 			foundBootloader = true
 		default:
-			return nil, fmt.Errorf(errorFormat, "bootloader must be either grub or u-boot")
+			return nil, fmt.Errorf(errorFormat, "bootloader must be either grub, u-boot or android-boot")


s/either/one of/

zyga

Let us know if there's anything we can do to help.

alfonsosanchezbeato · 2017-05-17T13:02:15Z

PR resubmitted after addressing the requested changes. @zyga and @chipaca thanks for your thorough review and for being patient with a golang newbie like me :)

mvo5

Looks very nice, thanks for doing this work. I have some comments inside that might be worth considering. But nothing major, it looks pretty nice.

mvo5 · 2017-05-17T13:31:10Z

partition/export_test.go

+)
+
+// creates a new Androidboot bootloader object
+func MockNewAndroidboot() Bootloader {


(nitpick) this can probably just be NewAndroidboot() nothing is mocked/faked here its just exported for the tests.

mvo5 · 2017-05-17T13:33:25Z

partition/export_test.go

+func MockAndroidbootFile(c *C, mode os.FileMode) {
+	f := &androidboot{}
+	newpath := filepath.Join(dirs.GlobalRootDir, "/boot/androidboot")
+	os.MkdirAll(newpath, os.ModePerm)


(nitpick) I think it makes sense to get the err from os.MkdirAll() and c.Assert(err, IsNil).

Also, os.ModePerm is 0777, even in its a test I would use 0755 here instead (just in case umask is funny etc).

mvo5 · 2017-05-17T13:34:06Z

partition/export_test.go

+
+func MockAndroidbootFile(c *C, mode os.FileMode) {
+	f := &androidboot{}
+	newpath := filepath.Join(dirs.GlobalRootDir, "/boot/androidboot")


(nitpick) might be easier to do: newpath := filepath.Dir(f.ConfigFile()) (or f.Dir()), then you don't need to encode the knowledge of the exact path into this test.

mvo5 · 2017-05-17T13:38:17Z

partition/androidbootenv/androidbootenv.go

+	file, err := os.Open(a.path)
+	if err != nil {
+		return err
+	}


defer file.Close() is missing here.

(fixed, below)

zyga

Much nicer, thank you! A few more suggestions.

zyga · 2017-05-17T13:39:40Z

partition/androidboot.go

+
+	out := make(map[string]string, len(names))
+	for _, name := range names {
+		out[name] = env.Get(name)


Can there be any duplicates in names?

Not really, it would be a bug either in snapd or in the bootloader if that happens.

zyga · 2017-05-17T13:39:58Z

partition/androidboot.go

+	if err := env.Load(); err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	for k, v := range values {


Do we need to worry about overwriting values here?

No, values should not be duplicated

zyga · 2017-05-17T13:40:32Z

partition/androidboot_test.go

+func (g *androidbootTestSuite) SetUpTest(c *C) {
+	dirs.SetRootDir(c.MkDir())
+
+	// the file needs to exist


It would be more useful to explain why the file has to exist.

zyga · 2017-05-17T13:41:35Z

partition/androidboot_test.go

+}
+
+func (s *androidbootTestSuite) TestNewAndroidbootNoAndroidbootReturnsNil(c *C) {
+	dirs.SetRootDir("")


Except when ran on android. I would suggest you to actually keep using the temporary random directory and populate it (or not populate it perhaps) so that it has the right data for the test. This will just run it on the real system which is not what unit tests are about.

zyga · 2017-05-17T13:41:58Z

partition/androidboot_test.go

+}
+
+func (s *androidbootTestSuite) TestSetGetBootVar(c *C) {
+	a := partition.MockNewAndroidboot()


You can move this to the SetUpTest function as it seems to happen in all the tests.

TestNewAndroidbootNoAndroidbootReturnsNil is unfortunately an exception.

zyga · 2017-05-17T13:42:26Z

partition/androidbootenv/androidbootenv.go

+	"github.com/snapcore/snapd/osutil"
+)
+
+type Env struct {


Some documentation (tip: run golint in the directory) would help here.

zyga · 2017-05-17T13:43:18Z

partition/androidbootenv/androidbootenv.go

+		l := strings.SplitN(scanner.Text(), "=", 2)
+		// be liberal in what you accept
+		if len(l) < 2 {
+			logger.Noticef("WARNING: bad value while parsing %v", a.path)


It might be invaluable to actually log scanner.Text() with %q here.

zyga · 2017-05-17T13:44:00Z

partition/androidbootenv/androidbootenv_test.go

+}
+
+func (a *androidbootenvTestSuite) TestSet(c *C) {
+	env := androidbootenv.NewEnv(a.envPath)


env := ... can be moved to SetUpTest

zyga · 2017-05-17T13:44:59Z

partition/export_test.go

+)
+
+// creates a new Androidboot bootloader object
+func MockNewAndroidboot() Bootloader {


This is not a mock, it's doing the real thing.

Instead do this:

var NewAndroidBoot = newAndroidBoot

(Note that I also changed the capitalization of androidboot to AndroidBoot)

Changed but to NewAndroidBoot instead

zyga · 2017-05-17T13:45:32Z

partition/export_test.go

+	return newAndroidboot()
+}
+
+func MockAndroidbootFile(c *C, mode os.FileMode) {


Nitpick: spell it as androidBoot everywhere

chipaca

nearly there! marking approved by me as my concerns are raised by zyga and mvo already :-)

alfonsosanchezbeato · 2017-05-17T14:51:38Z

Thanks again for the reviews, PR refreshed after addressing comments.

zyga

One last set of nitpicks. LGTM then

zyga · 2017-05-18T09:43:03Z

partition/androidbootenv/androidbootenv.go

+		}
+		a.env[l[0]] = l[1]
+	}
+


I think you need to use the scanner.Err here. See https://golang.org/src/bufio/example_test.go for examples.

Good catch, done

zyga · 2017-05-18T09:43:35Z

partition/androidbootenv/androidbootenv.go

+func (a *Env) Save() error {
+	var w bytes.Buffer
+
+	for k, v := range a.env {


Do we care about deterministic output here? If so please sort the keys, and write in order.

Not really, neither the booloader or snapd cares about the order in which the variables are stored.

zyga · 2017-05-18T09:44:14Z

partition/androidbootenv/androidbootenv_test.go

+	a.env.Set("key3", "value3")
+
+	err := a.env.Save()
+	c.Assert(err, IsNil)


Can you add a trivial test that ensures the output is as you expect, as in the actual string is what we expect (deterministic)

See previous comment

alfonsosanchezbeato · 2017-05-18T10:11:54Z

PR repushed after addressing comments

codecov-io · 2017-05-18T10:43:29Z

Codecov Report

Merging #3324 into master will decrease coverage by 0.01%.
The diff coverage is 73.77%.

@@            Coverage Diff             @@
##           master    #3324      +/-   ##
==========================================
- Coverage    77.6%   77.59%   -0.02%     
==========================================
  Files         364      366       +2     
  Lines       24956    25014      +58     
==========================================
+ Hits        19368    19409      +41     
- Misses       3865     3876      +11     
- Partials     1723     1729       +6

Impacted Files	Coverage Δ
snap/gadget.go	`82.35% <100%> (ø)`	⬆️
partition/bootloader.go	`80.55% <33.33%> (-4.74%)`	⬇️
partition/androidbootenv/androidbootenv.go	`66.66% <66.66%> (ø)`
partition/androidboot.go	`84.61% <84.61%> (ø)`
interfaces/sorting.go	`93.33% <0%> (-3.34%)`	⬇️
overlord/ifacestate/helpers.go	`62.86% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee977f...7824302. Read the comment docs.

zyga

LGTM, there are some tweaks we could do but I won't mind doing it in a follow-up.

zyga changed the title ~~Androidboot~~ partition,snap: add support for android boot May 16, 2017

morphis and others added 4 commits May 16, 2017 09:15

Initial support for fastboot as bootloader

bee0f6e

Add android-boot env files handler

cdb856b

Fix android-boot tests

3126b56

zyga reviewed May 16, 2017

View reviewed changes

chipaca suggested changes May 16, 2017

View reviewed changes

zyga requested changes May 17, 2017

View reviewed changes

alfonsosanchezbeato added 2 commits May 17, 2017 14:56

Address review comments

2c02cc1

Add auxiliary file in partition to test private methods

bdbce34

alfonsosanchezbeato force-pushed the androidboot branch from b3d8bc3 to bdbce34 Compare May 17, 2017 12:59

mvo5 reviewed May 17, 2017

View reviewed changes

zyga reviewed May 17, 2017

View reviewed changes

chipaca approved these changes May 17, 2017

View reviewed changes

Address review comments

1a29e05

alfonsosanchezbeato added 2 commits May 17, 2017 17:50

Fix androidbootenv tests

491bd93

Fix gadget test

44c2a53

mvo5 approved these changes May 18, 2017

View reviewed changes

zyga reviewed May 18, 2017

View reviewed changes

Check error while parsing androidboot env file

7824302

Merge remote-tracking branch 'upstream/master' into HEAD

0d78dc0

zyga approved these changes May 19, 2017

View reviewed changes

zyga merged commit 004f307 into canonical:master May 19, 2017

partition,snap: add support for android boot #3324

partition,snap: add support for android boot #3324

Conversation

alfonsosanchezbeato commented May 16, 2017 • edited Loading

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented May 17, 2017

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented May 17, 2017

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented May 18, 2017

codecov-io commented May 18, 2017

Codecov Report

zyga left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented May 16, 2017 •

edited

Loading