osutil: add KernelCommandLineKeyValue; boot: refactor ModeAnd...FromK…

…ernelCommandLine (canonical#9659)

* boot/cmdline.go: add TODO about using strutil.KernelCommandLineSplit

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* strutil/cmdline.go: add GetKernelCommandLineKeyValue

This complements KernelCommandLineSplit, but goes further, checking for a
specific key-value pair in the kernel command line parameters, and returning the
value if found. This will be useful across the codebase for places where we want
to check one specific kernel command line parameter key-value pair.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* logger/logger.go: use GetKernelCommandLineKeyValue directly

This is a bit more straight forward to read IMHO and potentially reduces some
looping over the parameters as we break as soon as we find snapd.debug in the
positive case where it is set.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* strutil,osutil: move kernel commandline helpers to osutil

* boot: tweak TODO now that we use osutil.KernelCommandLineSplit

* many: refactor ModeAnd...FromKernelCommandLine and KernelCommandLineKeyValue

Refactor GetKernelCommandLineKeyValue to KernelCommandLineKeyValue which
returns a map of the specified keys that were found on the kernel command line
that have values. It also no longer takes the command line string as an
argument and instead parses the command line itself.

The above necessitates moving the mocking function for where to find
/proc/cmdline to osutil as well and adjusting many tests for this.

Finally, with all of this in place we can refactor
boot.ModeAndRecoverySystemFromKernelCommandLine to use the osutil helpers
and not duplicate parsing logic in the boot package. This does result in
a slight change in behavior where now duplicated kernel command line
parameters are not a fatal condition, but instead the last definition is
used.

Also adjust some tests to mock an empty proc/cmdline to avoid using the
host's version when running tests.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

* osutil, many: rename to KernelCommandLineKeyValues

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

* boot: disallow non empty system label without a mode

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

* osutil: tweak handling of cmdline keys

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

* osutil: mv cmdline to kcmdline

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

* osutil: comment tweak

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

* logger: further tweaks

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Co-authored-by: Michael Vogt <mvo@ubuntu.com>
Co-authored-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

boot/cmdline.go

@@ -20,16 +20,13 @@
 package boot
 
 import (
-	"bufio"
-	"bytes"
 	"errors"
 	"fmt"
-	"io/ioutil"
-	"strings"
 
 	"github.com/snapcore/snapd/asserts"
 	"github.com/snapcore/snapd/bootloader"
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/strutil"
 )
 
@@ -45,43 +42,36 @@ const (
 )
 
 var (
-	// the kernel commandline - can be overridden in tests
-	procCmdline = "/proc/cmdline"
-
 	validModes = []string{ModeInstall, ModeRecover, ModeRun}
 )
 
-func whichModeAndRecoverySystem(cmdline []byte) (mode string, sysLabel string, err error) {
-	scanner := bufio.NewScanner(bytes.NewBuffer(cmdline))
-	scanner.Split(bufio.ScanWords)
-
-	for scanner.Scan() {
-		w := scanner.Text()
-		if strings.HasPrefix(w, "snapd_recovery_mode=") {
-			if mode != "" {
-				return "", "", fmt.Errorf("cannot specify mode more than once")
-			}
-			mode = strings.SplitN(w, "=", 2)[1]
-			if mode == "" {
-				mode = ModeInstall
-			}
-			if !strutil.ListContains(validModes, mode) {
-				return "", "", fmt.Errorf("cannot use unknown mode %q", mode)
-			}
-		}
-		if strings.HasPrefix(w, "snapd_recovery_system=") {
-			if sysLabel != "" {
-				return "", "", fmt.Errorf("cannot specify recovery system label more than once")
-			}
-			sysLabel = strings.SplitN(w, "=", 2)[1]
-		}
-	}
-	if err := scanner.Err(); err != nil {
+// ModeAndRecoverySystemFromKernelCommandLine returns the current system mode
+// and the recovery system label as passed in the kernel command line by the
+// bootloader.
+func ModeAndRecoverySystemFromKernelCommandLine() (mode, sysLabel string, err error) {
+	m, err := osutil.KernelCommandLineKeyValues("snapd_recovery_mode", "snapd_recovery_system")
+	if err != nil {
 		return "", "", err
 	}
+	var modeOk bool
+	mode, modeOk = m["snapd_recovery_mode"]
+
+	// no mode specified gets interpreted as install
+	if modeOk {
+		if mode == "" {
+			mode = ModeInstall
+		} else if !strutil.ListContains(validModes, mode) {
+			return "", "", fmt.Errorf("cannot use unknown mode %q", mode)
+		}
+	}
+
+	sysLabel = m["snapd_recovery_system"]
+
 	switch {
 	case mode == "" && sysLabel == "":
 		return "", "", fmt.Errorf("cannot detect mode nor recovery system to use")
+	case mode == "" && sysLabel != "":
+		return "", "", fmt.Errorf("cannot specify system label without a mode")
 	case mode == ModeInstall && sysLabel == "":
 		return "", "", fmt.Errorf("cannot specify install mode without system label")
 	case mode == ModeRun && sysLabel != "":
@@ -92,26 +82,6 @@ func whichModeAndRecoverySystem(cmdline []byte) (mode string, sysLabel string, e
 	return mode, sysLabel, nil
 }
 
-// ModeAndRecoverySystemFromKernelCommandLine returns the current system mode
-// and the recovery system label as passed in the kernel command line by the
-// bootloader.
-func ModeAndRecoverySystemFromKernelCommandLine() (mode, sysLabel string, err error) {
-	cmdline, err := ioutil.ReadFile(procCmdline)
-	if err != nil {
-		return "", "", err
-	}
-	return whichModeAndRecoverySystem(cmdline)
-}
-
-// MockProcCmdline overrides the path to /proc/cmdline. For use in tests.
-func MockProcCmdline(newPath string) (restore func()) {
-	oldProcCmdline := procCmdline
-	procCmdline = newPath
-	return func() {
-		procCmdline = oldProcCmdline
-	}
-}
-
 var errBootConfigNotManaged = errors.New("boot config is not managed")
 
 func getBootloaderManagingItsAssets(where string, opts *bootloader.Options) (bootloader.TrustedAssetsBootloader, error) {

boot/cmdline_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/snapcore/snapd/boot/boottest"
 	"github.com/snapcore/snapd/bootloader"
 	"github.com/snapcore/snapd/bootloader/bootloadertest"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -47,7 +48,7 @@ func (s *kernelCommandLineSuite) SetUpTest(c *C) {
 
 	err := os.MkdirAll(filepath.Join(s.rootDir, "proc"), 0755)
 	c.Assert(err, IsNil)
-	restore := boot.MockProcCmdline(filepath.Join(s.rootDir, "proc/cmdline"))
+	restore := osutil.MockProcCmdline(filepath.Join(s.rootDir, "proc/cmdline"))
 	s.AddCleanup(restore)
 }
 
@@ -85,13 +86,19 @@ func (s *kernelCommandLineSuite) TestModeAndLabel(c *C) {
 		cmd: "snapd_recovery_mode=install foo=bar",
 		err: `cannot specify install mode without system label`,
 	}, {
-		// boot scripts couldn't decide on mode
-		cmd: "snapd_recovery_mode=install snapd_recovery_system=1234 snapd_recovery_mode=run",
-		err: "cannot specify mode more than once",
+		cmd: "snapd_recovery_system=1234",
+		err: `cannot specify system label without a mode`,
 	}, {
-		// boot scripts couldn't decide which system to use
-		cmd: "snapd_recovery_system=not-this-one snapd_recovery_mode=install snapd_recovery_system=1234",
-		err: "cannot specify recovery system label more than once",
+		// multiple kernel command line params end up using the last one - this
+		// effectively matches the kernel handling too
+		cmd:  "snapd_recovery_mode=install snapd_recovery_system=1234 snapd_recovery_mode=run",
+		mode: "run",
+		// label gets unset because it's not used for run mode
+		label: "",
+	}, {
+		cmd:   "snapd_recovery_system=not-this-one snapd_recovery_mode=install snapd_recovery_system=1234",
+		mode:  "install",
+		label: "1234",
 	}} {
 		c.Logf("tc: %q", tc)
 		s.mockProcCmdlineContent(c, tc.cmd)

bootloader/grub.go

@@ -29,7 +29,6 @@ import (
 	"github.com/snapcore/snapd/bootloader/grubenv"
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
-	"github.com/snapcore/snapd/strutil"
 )
 
 // sanity - grub implements the required interfaces
@@ -372,7 +371,7 @@ func (g *grub) commandLineForEdition(edition uint, modeArg, systemArg, extraArgs
 		assetName = "grub-recovery.cfg"
 	}
 	staticCmdline := staticCommandLineForGrubAssetEdition(assetName, edition)
-	args, err := strutil.KernelCommandLineSplit(staticCmdline + " " + extraArgs)
+	args, err := osutil.KernelCommandLineSplit(staticCmdline + " " + extraArgs)
 	if err != nil {
 		return "", fmt.Errorf("cannot use badly formatted kernel command line: %v", err)
 	}

cmd/snap-bootstrap/cmd_initramfs_mounts_test.go

@@ -273,7 +273,7 @@ func (s *initramfsMountsSuite) mockProcCmdlineContent(c *C, newContent string) {
 	mockProcCmdline := filepath.Join(c.MkDir(), "proc-cmdline")
 	err := ioutil.WriteFile(mockProcCmdline, []byte(newContent), 0644)
 	c.Assert(err, IsNil)
-	restore := boot.MockProcCmdline(mockProcCmdline)
+	restore := osutil.MockProcCmdline(mockProcCmdline)
 	s.AddCleanup(restore)
 }
 

cmd/snap/cmd_auto_import_test.go

@@ -28,7 +28,6 @@ import (
 
 	. "gopkg.in/check.v1"
 
-	"github.com/snapcore/snapd/boot"
 	snap "github.com/snapcore/snapd/cmd/snap"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/logger"
@@ -315,7 +314,7 @@ func (s *SnapSuite) TestAutoImportUnhappyInInstallMode(c *C) {
 	err := ioutil.WriteFile(mockProcCmdlinePath, []byte("foo=bar snapd_recovery_mode=install snapd_recovery_system=20191118"), 0644)
 	c.Assert(err, IsNil)
 
-	restore = boot.MockProcCmdline(mockProcCmdlinePath)
+	restore = osutil.MockProcCmdline(mockProcCmdlinePath)
 	defer restore()
 
 	_, err = snap.Parser(snap.Client()).ParseArgs([]string{"auto-import"})

cmd/snap/main_test.go

@@ -98,6 +98,10 @@ func (s *BaseSnapSuite) SetUpTest(c *C) {
 	s.AddCleanup(snap.MockIsStdinTTY(false))
 
 	s.AddCleanup(snap.MockSELinuxIsEnabled(func() (bool, error) { return false, nil }))
+
+	// mock an empty cmdline since we check the cmdline to check whether we are
+	// in install mode or not and we don't want to use the host's proc/cmdline
+	s.AddCleanup(osutil.MockProcCmdline(filepath.Join(c.MkDir(), "proc/cmdline")))
 }
 
 func (s *BaseSnapSuite) TearDownTest(c *C) {

logger/export_test.go

@@ -35,10 +35,10 @@ func GetLoggerFlags() int {
 	return log.log.Flags()
 }
 
-func MockProcCmdline(new string) (restore func()) {
-	old := procCmdline
-	procCmdline = new
+func ProcCmdlineMustMock(new bool) (restore func()) {
+	old := procCmdlineUseDefaultMockInTests
+	procCmdlineUseDefaultMockInTests = new
 	return func() {
-		procCmdline = old
+		procCmdlineUseDefaultMockInTests = old
 	}
 }

logger/logger.go

@@ -23,14 +23,11 @@ import (
 	"bytes"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"log"
 	"os"
-	"strings"
 	"sync"
 
 	"github.com/snapcore/snapd/osutil"
-	"github.com/snapcore/snapd/strutil"
 )
 
 // A Logger is a fairly minimal logging tool.
@@ -163,15 +160,17 @@ func SimpleSetup() error {
 	return err
 }
 
-var procCmdline = "/proc/cmdline"
+// used to force testing of the kernel command line parsing
+var procCmdlineUseDefaultMockInTests = true
 
 // TODO: consider generalizing this to snapdenv and having it used by
 // other places that consider SNAPD_DEBUG
 func debugEnabledOnKernelCmdline() bool {
-	buf, err := ioutil.ReadFile(procCmdline)
-	if err != nil {
+	// if this is called during tests, always ignore it so we don't have to mock
+	// the /proc/cmdline for every test that ends up using a logger
+	if osutil.IsTestBinary() && procCmdlineUseDefaultMockInTests {
 		return false
 	}
-	l, _ := strutil.KernelCommandLineSplit(strings.TrimSpace(string(buf)))
-	return strutil.ListContains(l, "snapd.debug=1")
+	m, _ := osutil.KernelCommandLineKeyValues("snapd.debug")
+	return m["snapd.debug"] == "1"
 }

logger/logger_test.go

@@ -31,6 +31,7 @@ import (
 	. "gopkg.in/check.v1"
 
 	"github.com/snapcore/snapd/logger"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -126,10 +127,17 @@ func (s *LogSuite) TestWithLoggerLock(c *C) {
 }
 
 func (s *LogSuite) TestIntegrationDebugFromKernelCmdline(c *C) {
+	// must enable actually checking the command line, because by default the
+	// logger package will skip checking for the kernel command line parameter
+	// if it detects it is in a test because otherwise we would have to mock the
+	// cmdline in many many many more tests that end up using a logger
+	restore := logger.ProcCmdlineMustMock(false)
+	defer restore()
+
 	mockProcCmdline := filepath.Join(c.MkDir(), "proc-cmdline")
 	err := ioutil.WriteFile(mockProcCmdline, []byte("console=tty panic=-1 snapd.debug=1\n"), 0644)
 	c.Assert(err, IsNil)
-	restore := logger.MockProcCmdline(mockProcCmdline)
+	restore = osutil.MockProcCmdline(mockProcCmdline)
 	defer restore()
 
 	var buf bytes.Buffer

strutil/cmdline.go → osutil/kcmdline.go

@@ -17,13 +17,29 @@
  *
  */
 
-package strutil
+package osutil
 
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
+	"strings"
 )
 
+var (
+	procCmdline = "/proc/cmdline"
+)
+
+// MockProcCmdline overrides the path to /proc/cmdline. For use in tests.
+func MockProcCmdline(newPath string) (restore func()) {
+	MustBeTestBinary("mocking can only be done from tests")
+	oldProcCmdline := procCmdline
+	procCmdline = newPath
+	return func() {
+		procCmdline = oldProcCmdline
+	}
+}
+
 // KernelCommandLineSplit tries to split the string comprising full or a part
 // of a kernel command line into a list of individual arguments. Returns an
 // error when the input string is incorrectly formatted.
@@ -157,3 +173,33 @@ func KernelCommandLineSplit(s string) (out []string, err error) {
 	}
 	return out, nil
 }
+
+// KernelCommandLineKeyValues returns a map of the specified keys to the values
+// set for them in the kernel command line (eg. panic=-1). If the value is
+// missing from the kernel command line or it has no value (eg. quiet), the key
+// is omitted from the returned map.
+func KernelCommandLineKeyValues(keys ...string) (map[string]string, error) {
+	buf, err := ioutil.ReadFile(procCmdline)
+	if err != nil {
+		return nil, err
+	}
+	params, err := KernelCommandLineSplit(strings.TrimSpace(string(buf)))
+	if err != nil {
+		return nil, err
+	}
+
+	m := make(map[string]string, len(keys))
+
+	for _, param := range params {
+		for _, key := range keys {
+			if strings.HasPrefix(param, fmt.Sprintf("%s=", key)) {
+				res := strings.SplitN(param, "=", 2)
+				// we have confirmed key= prefix, thus len(res)
+				// is always 2
+				m[key] = res[1]
+				break
+			}
+		}
+	}
+	return m, nil
+}