Merge lp:~mvo/snappy/snappy-fix-bbb-crash into lp:~snappy-dev/snappy/snappy-moved-to-github

[Michael Vogt]

Fix crash on the BBB when it tries to call BootloaderDir.

I'm not overly happy with the fix it would be better to have a single
bootloader dir. I hope we get there eventually.

[Sergio Schvezov (sergiusens)]

please wait for this one, I want to land update-grub today and not deal with another merge

[Sergio Schvezov (sergiusens)]

it also fixes this issue in some way

Unmerged revisions

547. By Michael Vogt on 2015-07-01

only set currentBootPath, otherBootPath if we have that

546. By Michael Vogt on 2015-07-01

no mountinfo anymore in the tests

545. By Michael Vogt on 2015-07-01

decouple partition/bootloader further

=== modified file 'partition/bootloader.go'
--- partition/bootloader.go	2015-06-11 09:38:18 +0000
+++ partition/bootloader.go	2015-07-01 14:16:22 +0000
@@ -19,6 +19,8 @@
 
 package partition
 
+import "path/filepath"
+
 const (
 	// bootloader variable used to denote which rootfs to boot from
 	bootloaderRootfsVar = "snappy_ab"
@@ -84,13 +86,24 @@
 var bootloader = bootloaderImpl
 
 func bootloaderImpl(p *Partition) (bootLoader, error) {
+	var currentBootPath, otherBootPath string
+
+	// FIXME: ugly
+	if p != nil {
+		currentRootfs := p.rootPartition().shortName
+		currentBootPath = filepath.Join(bootloaderUbootDir, currentRootfs)
+
+		otherRootfs := p.otherRootPartition().shortName
+		otherBootPath = filepath.Join(bootloaderUbootDir, otherRootfs)
+	}
+
 	// try uboot
-	if uboot := newUboot(p); uboot != nil {
+	if uboot := newUboot(currentBootPath, otherBootPath); uboot != nil {
 		return uboot, nil
 	}
 
 	// no, try grub
-	if grub := newGrub(p); grub != nil {
+	if grub := newGrub(currentBootPath, otherBootPath); grub != nil {
 		return grub, nil
 	}
 

=== modified file 'partition/bootloader_grub.go'
--- partition/bootloader_grub.go	2015-06-11 09:16:26 +0000
+++ partition/bootloader_grub.go	2015-07-01 14:16:22 +0000
@@ -54,7 +54,7 @@
 const bootloaderNameGrub bootloaderName = "grub"
 
 // newGrub create a new Grub bootloader object
-func newGrub(partition *Partition) bootLoader {
+func newGrub(currentBootPath, otherBootPath string) bootLoader {
 	if !helpers.FileExists(bootloaderGrubConfigFile) || !helpers.FileExists(bootloaderGrubUpdateCmd) {
 		return nil
 	}

=== modified file 'partition/bootloader_grub_test.go'
--- partition/bootloader_grub_test.go	2015-06-11 09:16:26 +0000
+++ partition/bootloader_grub_test.go	2015-07-01 14:16:22 +0000
@@ -49,16 +49,14 @@
 func (s *PartitionTestSuite) TestNewGrubNoGrubReturnsNil(c *C) {
 	bootloaderGrubConfigFile = "no-such-dir"
 
-	partition := New()
-	g := newGrub(partition)
+	g := newGrub("a", "b")
 	c.Assert(g, IsNil)
 }
 
 func (s *PartitionTestSuite) TestNewGrub(c *C) {
 	s.makeFakeGrubEnv(c)
 
-	partition := New()
-	g := newGrub(partition)
+	g := newGrub("a", "b")
 	c.Assert(g, NotNil)
 	c.Assert(g.Name(), Equals, bootloaderNameGrub)
 }
@@ -76,28 +74,23 @@
 	s.makeFakeGrubEnv(c)
 	allCommands = []singleCommand{}
 
-	partition := New()
-	g := newGrub(partition)
+	g := newGrub("a", "b")
 	c.Assert(g, NotNil)
 	err := g.ToggleRootFS("b")
 	c.Assert(err, IsNil)
 
-	// this is always called
-	mp := singleCommand{"/bin/mountpoint", mountTarget}
-	c.Assert(allCommands[0], DeepEquals, mp)
-
 	expectedGrubUpdate := singleCommand{"/usr/sbin/chroot", mountTarget, bootloaderGrubUpdateCmd}
-	c.Assert(allCommands[1], DeepEquals, expectedGrubUpdate)
+	c.Assert(allCommands[0], DeepEquals, expectedGrubUpdate)
 
 	expectedGrubSet := singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_mode=try"}
-	c.Assert(allCommands[2], DeepEquals, expectedGrubSet)
+	c.Assert(allCommands[1], DeepEquals, expectedGrubSet)
 
 	// the https://developer.ubuntu.com/en/snappy/porting guide says
 	// we always use the short names
 	expectedGrubSet = singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_ab=b"}
-	c.Assert(allCommands[3], DeepEquals, expectedGrubSet)
+	c.Assert(allCommands[2], DeepEquals, expectedGrubSet)
 
-	c.Assert(len(allCommands), Equals, 4)
+	c.Assert(len(allCommands), Equals, 3)
 }
 
 func mockGrubEditenvList(cmd ...string) (string, error) {
@@ -109,8 +102,7 @@
 	s.makeFakeGrubEnv(c)
 	runCommandWithStdout = mockGrubEditenvList
 
-	partition := New()
-	g := newGrub(partition)
+	g := newGrub("a", "b")
 
 	v, err := g.GetBootVar(bootloaderBootmodeVar)
 	c.Assert(err, IsNil)
@@ -129,26 +121,19 @@
 	s.makeFakeGrubEnv(c)
 	allCommands = []singleCommand{}
 
-	partition := New()
-	g := newGrub(partition)
+	g := newGrub("a", "b")
 	c.Assert(g, NotNil)
 	err := g.MarkCurrentBootSuccessful("a")
 	c.Assert(err, IsNil)
 
-	// this is always called
-	mp := singleCommand{"/bin/mountpoint", mountTarget}
-	c.Assert(allCommands[0], DeepEquals, mp)
-
 	expectedGrubSet := singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "unset", "snappy_trial_boot"}
-
-	c.Assert(allCommands[1], DeepEquals, expectedGrubSet)
+	c.Assert(allCommands[0], DeepEquals, expectedGrubSet)
 
 	expectedGrubSet2 := singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_ab=a"}
-
-	c.Assert(allCommands[2], DeepEquals, expectedGrubSet2)
+	c.Assert(allCommands[1], DeepEquals, expectedGrubSet2)
 
 	expectedGrubSet3 := singleCommand{bootloaderGrubEnvCmd, bootloaderGrubEnvFile, "set", "snappy_mode=regular"}
-
-	c.Assert(allCommands[3], DeepEquals, expectedGrubSet3)
-
+	c.Assert(allCommands[2], DeepEquals, expectedGrubSet3)
+
+	c.Assert(allCommands, HasLen, 3)
 }

=== modified file 'partition/bootloader_uboot.go'
--- partition/bootloader_uboot.go	2015-06-11 09:16:26 +0000
+++ partition/bootloader_uboot.go	2015-07-01 14:16:22 +0000
@@ -71,20 +71,17 @@
 }
 
 // newUboot create a new Grub bootloader object
-func newUboot(partition *Partition) bootLoader {
+func newUboot(currentBootPath, otherBootPath string) bootLoader {
 	if !helpers.FileExists(bootloaderUbootConfigFile) {
 		return nil
 	}
 
-	u := uboot{}
-
-	currentRootfs := partition.rootPartition().shortName
-	u.currentBootPath = filepath.Join(bootloaderUbootDir, currentRootfs)
-
-	otherRootfs := partition.otherRootPartition().shortName
-	u.otherBootPath = filepath.Join(bootloaderUbootDir, otherRootfs)
-
-	return &u
+	u := &uboot{
+		currentBootPath: currentBootPath,
+		otherBootPath:   otherBootPath,
+	}
+
+	return u
 }
 
 func (u *uboot) Name() bootloaderName {

=== modified file 'partition/bootloader_uboot_test.go'
--- partition/bootloader_uboot_test.go	2015-06-29 23:15:00 +0000
+++ partition/bootloader_uboot_test.go	2015-07-01 14:16:22 +0000
@@ -74,26 +74,31 @@
 	c.Assert(err, IsNil)
 }
 
+func (s *PartitionTestSuite) TestNewUbootBootDir(c *C) {
+	s.makeFakeUbootEnv(c)
+
+	u := newUboot("a", "b")
+	c.Assert(u.BootDir(), Matches, ".*/boot/uboot")
+}
+
 func (s *PartitionTestSuite) TestNewUbootNoUbootReturnsNil(c *C) {
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, IsNil)
 }
 
 func (s *PartitionTestSuite) TestNewUboot(c *C) {
 	s.makeFakeUbootEnv(c)
 
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 	c.Assert(u.Name(), Equals, bootloaderNameUboot)
 }
 
 func (s *PartitionTestSuite) TestUbootGetBootVar(c *C) {
 	s.makeFakeUbootEnv(c)
-
 	partition := New()
-	u := newUboot(partition)
+
+	u := newUboot("a", "b")
 
 	nextBoot, err := u.GetBootVar(bootloaderRootfsVar)
 	c.Assert(err, IsNil)
@@ -107,9 +112,9 @@
 
 func (s *PartitionTestSuite) TestUbootToggleRootFS(c *C) {
 	s.makeFakeUbootEnv(c)
-
 	partition := New()
-	u := newUboot(partition)
+
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 
 	err := u.ToggleRootFS("b")
@@ -126,8 +131,7 @@
 func (s *PartitionTestSuite) TestUbootGetEnvVar(c *C) {
 	s.makeFakeUbootEnv(c)
 
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 
 	v, err := u.GetBootVar(bootloaderBootmodeVar)
@@ -252,8 +256,7 @@
 	c.Assert(err, IsNil)
 	c.Assert(helpers.FileExists(bootloaderUbootStampFile), Equals, true)
 
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 
 	// enter "try" mode so that we check to ensure that snappy
@@ -288,8 +291,7 @@
 	atomiCall := false
 	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
 
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 
 	c.Check(u.MarkCurrentBootSuccessful("a"), IsNil)
@@ -305,8 +307,7 @@
 	atomiCall := false
 	atomicFileUpdate = func(a string, b []string) error { atomiCall = true; return atomicFileUpdateImpl(a, b) }
 
-	partition := New()
-	u := newUboot(partition)
+	u := newUboot("a", "b")
 	c.Assert(u, NotNil)
 
 	c.Check(u.MarkCurrentBootSuccessful("a"), IsNil)