bootloader: use ForGadget when installing boot config

boot/makebootable.go

@@ -356,13 +356,9 @@ func makeBootable20RunMode(model *asserts.Model, rootdir string, bootWith *Boota
 
 		// installing boot config must be performed after the boot
 		// partition has been populated with gadget data
-		ok, err := bl.InstallBootConfig(bootWith.UnpackedGadgetDir, opts)
-		if err != nil {
+		if err := bl.InstallBootConfig(bootWith.UnpackedGadgetDir, opts); err != nil {
 			return fmt.Errorf("cannot install managed bootloader assets: %v", err)
 		}
-		if !ok {
-			return fmt.Errorf("cannot install boot config with a mismatched gadget")
-		}
 	}
 
 	if sealer != nil {

boot/makebootable_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/snapcore/snapd/bootloader"
 	"github.com/snapcore/snapd/bootloader/assets"
 	"github.com/snapcore/snapd/bootloader/bootloadertest"
+	"github.com/snapcore/snapd/bootloader/grubenv"
 	"github.com/snapcore/snapd/bootloader/ubootenv"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/gadget"
@@ -79,6 +80,7 @@ func makeSnapWithFiles(c *C, name, yaml string, revno snap.Revision, files [][]s
 }
 
 func (s *makeBootableSuite) TestMakeBootable(c *C) {
+	bootloader.Force(nil)
 	model := boottest.MakeMockModel()
 
 	grubCfg := []byte("#grub cfg")
@@ -117,14 +119,11 @@ version: 4.0
 	c.Assert(err, IsNil)
 
 	// check the bootloader config
-	m, err := s.bootloader.GetBootVars("snap_kernel", "snap_core", "snap_menuentry")
-	c.Assert(err, IsNil)
-	c.Check(m["snap_kernel"], Equals, "pc-kernel_5.snap")
-	c.Check(m["snap_core"], Equals, "core18_3.snap")
-	c.Check(m["snap_menuentry"], Equals, "My Model")
-
-	// kernel was extracted as needed
-	c.Check(s.bootloader.ExtractKernelAssetsCalls, DeepEquals, []snap.PlaceInfo{kernelInfo})
+	seedGenv := grubenv.NewEnv(filepath.Join(s.rootdir, "boot/grub/grubenv"))
+	c.Assert(seedGenv.Load(), IsNil)
+	c.Check(seedGenv.Get("snap_kernel"), Equals, "pc-kernel_5.snap")
+	c.Check(seedGenv.Get("snap_core"), Equals, "core18_3.snap")
+	c.Check(seedGenv.Get("snap_menuentry"), Equals, "My Model")
 
 	// check symlinks from snap blob dir
 	kernelBlob := filepath.Join(dirs.SnapBlobDirUnder(s.rootdir), kernelInfo.Filename())
@@ -173,6 +172,7 @@ func (s *makeBootable20UbootSuite) SetUpTest(c *C) {
 }
 
 func (s *makeBootable20Suite) TestMakeBootable20(c *C) {
+	bootloader.Force(nil)
 	model := boottest.MakeMockUC20Model()
 
 	unpackedGadgetDir := c.MkDir()
@@ -228,7 +228,8 @@ version: 5.0
 	// ensure only a single file got copied (the grub.cfg)
 	files, err := filepath.Glob(filepath.Join(s.rootdir, "EFI/ubuntu/*"))
 	c.Assert(err, IsNil)
-	c.Check(files, HasLen, 1)
+	// grub.cfg and grubenv
+	c.Check(files, HasLen, 2)
 	// check that the recovery bootloader configuration was installed with
 	// the correct content
 	c.Check(filepath.Join(s.rootdir, "EFI/ubuntu/grub.cfg"), testutil.FileEquals, grubRecoveryCfgAsset)
@@ -237,13 +238,13 @@ version: 5.0
 	c.Check(filepath.Join(s.rootdir, "boot"), testutil.FileAbsent)
 
 	// ensure the correct recovery system configuration was set
-	c.Check(s.bootloader.RecoverySystemDir, Equals, recoverySystemDir)
-	c.Check(s.bootloader.RecoverySystemBootVars, DeepEquals, map[string]string{
-		"snapd_recovery_kernel": "/snaps/pc-kernel_5.snap",
-	})
-	c.Check(s.bootloader.BootVars, DeepEquals, map[string]string{
-		"snapd_recovery_system": label,
-	})
+	seedGenv := grubenv.NewEnv(filepath.Join(s.rootdir, "EFI/ubuntu/grubenv"))
+	c.Assert(seedGenv.Load(), IsNil)
+	c.Check(seedGenv.Get("snapd_recovery_system"), Equals, label)
+
+	systemGenv := grubenv.NewEnv(filepath.Join(s.rootdir, recoverySystemDir, "grubenv"))
+	c.Assert(systemGenv.Load(), IsNil)
+	c.Check(systemGenv.Get("snapd_recovery_kernel"), Equals, "/snaps/pc-kernel_5.snap")
 }
 
 func (s *makeBootable20Suite) TestMakeBootable20UnsetRecoverySystemLabelError(c *C) {
@@ -802,6 +803,7 @@ version: 5.0
 }
 
 func (s *makeBootable20UbootSuite) TestUbootMakeBootable20TraditionalUbootenvFails(c *C) {
+	bootloader.Force(nil)
 	model := boottest.MakeMockUC20Model()
 
 	unpackedGadgetDir := c.MkDir()
@@ -849,7 +851,7 @@ version: 5.0
 
 	// TODO:UC20: enable this use case
 	err = boot.MakeBootable(model, s.rootdir, bootWith, nil)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot find boot config in %q", unpackedGadgetDir))
+	c.Assert(err, ErrorMatches, "non-empty uboot.env not supported on UC20 yet")
 }
 
 func (s *makeBootable20UbootSuite) TestUbootMakeBootable20BootScr(c *C) {

bootloader/androidboot.go

@@ -52,7 +52,7 @@ func (a *androidboot) dir() string {
 	return filepath.Join(a.rootdir, "/boot/androidboot")
 }
 
-func (a *androidboot) InstallBootConfig(gadgetDir string, opts *Options) (bool, error) {
+func (a *androidboot) InstallBootConfig(gadgetDir string, opts *Options) error {
 	gadgetFile := filepath.Join(gadgetDir, a.Name()+".conf")
 	systemFile := a.ConfigFile()
 	return genericInstallBootConfig(gadgetFile, systemFile)

bootloader/bootloader.go

@@ -96,9 +96,8 @@ type Bootloader interface {
 	ConfigFile() string
 
 	// InstallBootConfig will try to install the boot config in the
-	// given gadgetDir to rootdir. If no boot config for this bootloader
-	// is found ok is false.
-	InstallBootConfig(gadgetDir string, opts *Options) (ok bool, err error)
+	// given gadgetDir to rootdir.
+	InstallBootConfig(gadgetDir string, opts *Options) error
 
 	// ExtractKernelAssets extracts kernel assets from the given kernel snap.
 	ExtractKernelAssets(s snap.PlaceInfo, snapf snap.Container) error
@@ -198,25 +197,22 @@ type TrustedAssetsBootloader interface {
 	BootChain(runBl Bootloader, kernelPath string) ([]BootFile, error)
 }
 
-func genericInstallBootConfig(gadgetFile, systemFile string) (bool, error) {
-	if !osutil.FileExists(gadgetFile) {
-		return false, nil
-	}
+func genericInstallBootConfig(gadgetFile, systemFile string) error {
 	if err := os.MkdirAll(filepath.Dir(systemFile), 0755); err != nil {
-		return true, err
+		return err
 	}
-	return true, osutil.CopyFile(gadgetFile, systemFile, osutil.CopyFlagOverwrite)
+	return osutil.CopyFile(gadgetFile, systemFile, osutil.CopyFlagOverwrite)
 }
 
-func genericSetBootConfigFromAsset(systemFile, assetName string) (bool, error) {
+func genericSetBootConfigFromAsset(systemFile, assetName string) error {
 	bootConfig := assets.Internal(assetName)
 	if bootConfig == nil {
-		return true, fmt.Errorf("internal error: no boot asset for %q", assetName)
+		return fmt.Errorf("internal error: no boot asset for %q", assetName)
 	}
 	if err := os.MkdirAll(filepath.Dir(systemFile), 0755); err != nil {
-		return true, err
+		return err
 	}
-	return true, osutil.AtomicWriteFile(systemFile, bootConfig, 0644, 0)
+	return osutil.AtomicWriteFile(systemFile, bootConfig, 0644, 0)
 }
 
 func genericUpdateBootConfigFromAssets(systemFile string, assetName string) error {
@@ -249,16 +245,11 @@ func InstallBootConfig(gadgetDir, rootDir string, opts *Options) error {
 	if err := opts.validate(); err != nil {
 		return err
 	}
-	// TODO:UC20 use ForGadget() to obtain the right bootloader
-	for _, bl := range []installableBootloader{&grub{}, &uboot{}, &androidboot{}, &lk{}} {
-		bl.setRootDir(rootDir)
-		ok, err := bl.InstallBootConfig(gadgetDir, opts)
-		if ok {
-			return err
-		}
+	bl, err := ForGadget(gadgetDir, rootDir, opts)
+	if err != nil {
+		return fmt.Errorf("cannot find boot config in %q", gadgetDir)
 	}
-
-	return fmt.Errorf("cannot find boot config in %q", gadgetDir)
+	return bl.InstallBootConfig(gadgetDir, opts)
 }
 
 type bootloaderNewFunc func(rootdir string, opts *Options) Bootloader
@@ -368,8 +359,9 @@ func ForGadget(gadgetDir, rootDir string, opts *Options) (Bootloader, error) {
 	}
 	for _, blNew := range bootloaders {
 		bl := blNew(rootDir, opts)
+		markerConf := filepath.Join(gadgetDir, bl.Name()+".conf")
 		// do we have a marker file?
-		if osutil.FileExists(filepath.Join(gadgetDir, bl.Name()+".conf")) {
+		if osutil.FileExists(markerConf) {
 			return bl, nil
 		}
 	}

bootloader/bootloadertest/bootloadertest.go

@@ -43,7 +43,6 @@ type MockBootloader struct {
 	RemoveKernelAssetsCalls  []snap.PlaceInfo
 
 	InstallBootConfigCalled []string
-	InstallBootConfigResult bool
 	InstallBootConfigErr    error
 
 	enabledKernel    snap.PlaceInfo
@@ -138,9 +137,9 @@ func (b *MockBootloader) SetEnabledTryKernel(s snap.PlaceInfo) (restore func())
 
 // InstallBootConfig installs the boot config in the gadget directory to the
 // mock bootloader's root directory.
-func (b *MockBootloader) InstallBootConfig(gadgetDir string, opts *bootloader.Options) (bool, error) {
+func (b *MockBootloader) InstallBootConfig(gadgetDir string, opts *bootloader.Options) error {
 	b.InstallBootConfigCalled = append(b.InstallBootConfigCalled, gadgetDir)
-	return b.InstallBootConfigResult, b.InstallBootConfigErr
+	return b.InstallBootConfigErr
 }
 
 // MockRecoveryAwareBootloader mocks a bootloader implementing the

bootloader/grub.go

@@ -87,29 +87,19 @@ func (g *grub) dir() string {
 	return filepath.Join(g.rootdir, g.basedir)
 }
 
-func (g *grub) installManagedRecoveryBootConfig(gadgetDir string) (bool, error) {
-	gadgetGrubCfg := filepath.Join(gadgetDir, g.Name()+".conf")
-	if !osutil.FileExists(gadgetGrubCfg) {
-		// gadget does not use grub bootloader
-		return false, nil
-	}
+func (g *grub) installManagedRecoveryBootConfig(gadgetDir string) error {
 	assetName := g.Name() + "-recovery.cfg"
 	systemFile := filepath.Join(g.rootdir, "/EFI/ubuntu/grub.cfg")
 	return genericSetBootConfigFromAsset(systemFile, assetName)
 }
 
-func (g *grub) installManagedBootConfig(gadgetDir string) (bool, error) {
-	gadgetGrubCfg := filepath.Join(gadgetDir, g.Name()+".conf")
-	if !osutil.FileExists(gadgetGrubCfg) {
-		// gadget does not use grub bootloader
-		return false, nil
-	}
+func (g *grub) installManagedBootConfig(gadgetDir string) error {
 	assetName := g.Name() + ".cfg"
 	systemFile := filepath.Join(g.rootdir, "/EFI/ubuntu/grub.cfg")
 	return genericSetBootConfigFromAsset(systemFile, assetName)
 }
 
-func (g *grub) InstallBootConfig(gadgetDir string, opts *Options) (bool, error) {
+func (g *grub) InstallBootConfig(gadgetDir string, opts *Options) error {
 	if opts != nil && opts.Role == RoleRecovery {
 		// install managed config for the recovery partition
 		return g.installManagedRecoveryBootConfig(gadgetDir)

bootloader/lk.go

@@ -72,7 +72,7 @@ func (l *lk) dir() string {
 	return filepath.Join(l.rootdir, "/boot/lk/")
 }
 
-func (l *lk) InstallBootConfig(gadgetDir string, opts *Options) (bool, error) {
+func (l *lk) InstallBootConfig(gadgetDir string, opts *Options) error {
 	gadgetFile := filepath.Join(gadgetDir, l.Name()+".conf")
 	systemFile := l.ConfigFile()
 	return genericInstallBootConfig(gadgetFile, systemFile)

bootloader/uboot.go

@@ -84,7 +84,7 @@ func (u *uboot) dir() string {
 	return filepath.Join(u.rootdir, u.basedir)
 }
 
-func (u *uboot) InstallBootConfig(gadgetDir string, blOpts *Options) (bool, error) {
+func (u *uboot) InstallBootConfig(gadgetDir string, blOpts *Options) error {
 	gadgetFile := filepath.Join(gadgetDir, u.Name()+".conf")
 	// if the gadget file is empty, then we don't install anything
 	// this is because there are some gadgets, namely the 20 pi gadget right
@@ -95,7 +95,7 @@ func (u *uboot) InstallBootConfig(gadgetDir string, blOpts *Options) (bool, erro
 	//            actual format?
 	st, err := os.Stat(gadgetFile)
 	if err != nil {
-		return false, err
+		return err
 	}
 	if st.Size() == 0 {
 		// we have an empty uboot.conf, and hence a uboot bootloader in the
@@ -105,20 +105,20 @@ func (u *uboot) InstallBootConfig(gadgetDir string, blOpts *Options) (bool, erro
 
 		err := os.MkdirAll(filepath.Dir(u.envFile()), 0755)
 		if err != nil {
-			return false, err
+			return err
 		}
 
 		// TODO:UC20: what's a reasonable size for this file?
 		env, err := ubootenv.Create(u.envFile(), 4096)
 		if err != nil {
-			return false, err
+			return err
 		}
 
 		if err := env.Save(); err != nil {
-			return false, nil
+			return nil
 		}
 
-		return true, nil
+		return nil
 	}
 
 	// InstallBootConfig gets called on a uboot that does not come from newUboot
@@ -128,7 +128,7 @@ func (u *uboot) InstallBootConfig(gadgetDir string, blOpts *Options) (bool, erro
 	if blOpts != nil && blOpts.Role == RoleRecovery {
 		// not supported yet, this is traditional uboot.env from gadget
 		// TODO:UC20: support this use-case
-		return false, fmt.Errorf("non-empty uboot.env not supported on UC20 yet")
+		return fmt.Errorf("non-empty uboot.env not supported on UC20 yet")
 	}
 
 	systemFile := u.ConfigFile()

image/image_test.go

@@ -38,6 +38,7 @@ import (
 	"github.com/snapcore/snapd/bootloader"
 	"github.com/snapcore/snapd/bootloader/assets"
 	"github.com/snapcore/snapd/bootloader/bootloadertest"
+	"github.com/snapcore/snapd/bootloader/grubenv"
 	"github.com/snapcore/snapd/bootloader/ubootenv"
 	"github.com/snapcore/snapd/image"
 	"github.com/snapcore/snapd/osutil"
@@ -2568,10 +2569,8 @@ func (s *imageSuite) makeUC20Model(extraHeaders map[string]interface{}) *asserts
 	return s.Brands.Model("my-brand", "my-model", headers)
 }
 
-func (s *imageSuite) TestSetupSeedCore20(c *C) {
-	bl := bootloadertest.Mock("grub", c.MkDir()).RecoveryAware()
-	bootloader.Force(bl)
-
+func (s *imageSuite) TestSetupSeedCore20Grub(c *C) {
+	bootloader.Force(nil)
 	restore := image.MockTrusted(s.StoreSigning.Trusted)
 	defer restore()
 
@@ -2642,28 +2641,33 @@ func (s *imageSuite) TestSetupSeedCore20(c *C) {
 
 	// check boot config
 	grubCfg := filepath.Join(prepareDir, "system-seed", "EFI/ubuntu/grub.cfg")
+	seedGrubenv := filepath.Join(prepareDir, "system-seed", "EFI/ubuntu/grubenv")
 	grubRecoveryCfgAsset := assets.Internal("grub-recovery.cfg")
 	c.Assert(grubRecoveryCfgAsset, NotNil)
 	c.Check(grubCfg, testutil.FileEquals, string(grubRecoveryCfgAsset))
-	// make sure that grub.cfg is the only file present inside the directory
+	// make sure that grub.cfg and grubenv are the only files present inside
+	// the directory
 	gl, err := filepath.Glob(filepath.Join(prepareDir, "system-seed/EFI/ubuntu/*"))
 	c.Assert(err, IsNil)
-	c.Check(gl, DeepEquals, []string{grubCfg})
-
-	c.Check(s.stderr.String(), Equals, "")
+	c.Check(gl, DeepEquals, []string{
+		grubCfg,
+		seedGrubenv,
+	})
 
 	// check recovery system specific config
 	systems, err := filepath.Glob(filepath.Join(seeddir, "systems", "*"))
 	c.Assert(err, IsNil)
 	c.Assert(systems, HasLen, 1)
 
-	c.Check(bl.RecoverySystemDir, Equals, fmt.Sprintf("/systems/%s", filepath.Base(systems[0])))
-	c.Check(bl.RecoverySystemBootVars, DeepEquals, map[string]string{
-		"snapd_recovery_kernel": "/snaps/pc-kernel_1.snap",
-	})
-	c.Check(bl.BootVars, DeepEquals, map[string]string{
-		"snapd_recovery_system": filepath.Base(systems[0]),
-	})
+	seedGenv := grubenv.NewEnv(seedGrubenv)
+	c.Assert(seedGenv.Load(), IsNil)
+	c.Check(seedGenv.Get("snapd_recovery_system"), Equals, filepath.Base(systems[0]))
+
+	c.Check(s.stderr.String(), Equals, "")
+
+	systemGenv := grubenv.NewEnv(filepath.Join(systems[0], "grubenv"))
+	c.Assert(systemGenv.Load(), IsNil)
+	c.Check(systemGenv.Get("snapd_recovery_kernel"), Equals, "/snaps/pc-kernel_1.snap")
 
 	// check the downloads
 	c.Check(s.storeActions, HasLen, 5)