gadget: fix discovering of multiple disks when building disk traits

gadget/device_darwin.go

@@ -29,6 +29,6 @@ func FindDeviceForStructure(vs *VolumeStructure) (string, error) {
 	return "", errNotImplemented
 }
 
-func VerifyDeviceForStructure(device string, vs *VolumeStructure) (string, error) {
+func ResolveDeviceForStructure(device string) (string, error) {
 	return "", errNotImplemented
 }

gadget/device_linux.go

@@ -22,7 +22,6 @@ package gadget
 import (
 	"fmt"
 	"path/filepath"
-	"strings"
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/logger"
@@ -32,15 +31,7 @@ import (
 
 var evalSymlinks = filepath.EvalSymlinks
 
-func resolveBaseDevice(baseDevicePath string) (string, error) {
-	if !osutil.IsSymlink(baseDevicePath) {
-		// no need to resolve anything
-		return baseDevicePath, nil
-	}
-	return evalSymlinks(baseDevicePath)
-}
-
-func resolveMaybeDiskPaths(devicePrefix string, diskPaths []string) (string, error) {
+func resolveMaybeDiskPaths(diskPaths []string) (string, error) {
 	var found string
 	var match string
 	for _, candidate := range diskPaths {
@@ -62,11 +53,6 @@ func resolveMaybeDiskPaths(devicePrefix string, diskPaths []string) (string, err
 			return "", fmt.Errorf("conflicting device match, %q points to %q, previous match %q points to %q",
 				candidate, target, match, found)
 		}
-		if !strings.HasPrefix(target, devicePrefix) {
-			// does not match the expected device path, meaning the candidate
-			// will be skipped
-			continue
-		}
 		found = target
 		match = candidate
 	}
@@ -109,26 +95,16 @@ func discoverDeviceDiskCandidatesForStructure(vs *VolumeStructure) (candidates [
 // correctly.
 func FindDeviceForStructure(vs *VolumeStructure) (string, error) {
 	candidates := discoverDeviceDiskCandidatesForStructure(vs)
-	return resolveMaybeDiskPaths("", candidates)
+	return resolveMaybeDiskPaths(candidates)
 }
 
-// VerifyDeviceForStructure is an extension to FindDeviceForStructure that allows
-// supplying a device path to limit match to a specific disk. Calling this without a filter
+// ResolveDeviceForStructure is an opposite to FindDeviceForStructure that allows
+// supplying a device path to resolve a specific disk. Calling this without a filter
 // (i.e device == ""), will return an error
-func VerifyDeviceForStructure(device string, vs *VolumeStructure) (string, error) {
+// The device path must be a path into /dev/disk/**
+func ResolveDeviceForStructure(device string) (string, error) {
 	if device == "" {
 		return "", fmt.Errorf("internal error: device must be supplied")
 	}
-
-	resolved, err := resolveBaseDevice(device)
-	if err != nil {
-		return "", err
-	}
-
-	// Even though the underlying disk path is fixed, there are other
-	// requirements that must be fulfilled in this case, to simplify
-	// logic we re-use the disk candidates code, even though not all
-	// candidates will match.
-	candidates := discoverDeviceDiskCandidatesForStructure(vs)
-	return resolveMaybeDiskPaths(resolved, candidates)
+	return resolveMaybeDiskPaths([]string{device})
 }

gadget/device_test.go

@@ -21,6 +21,7 @@ package gadget_test
 
 import (
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 
@@ -245,38 +246,35 @@ func (d *deviceSuite) TestDeviceFindBadEvalSymlinks(c *C) {
 	c.Check(found, Equals, "")
 }
 
-func (d *deviceSuite) TestVerifyDeviceEmptyBase(c *C) {
-	found, err := gadget.VerifyDeviceForStructure("", &gadget.VolumeStructure{Name: "relative", EnclosingVolume: &gadget.Volume{}})
+func (d *deviceSuite) TestResolveDeviceEmptyBase(c *C) {
+	found, err := gadget.ResolveDeviceForStructure("")
 	c.Check(err, ErrorMatches, `internal error: device must be supplied`)
 	c.Check(found, Equals, "")
 }
 
-func (d *deviceSuite) TestVerifyDeviceMatchesBaseDeviceAsSymlink(c *C) {
+func (d *deviceSuite) TestResolveDeviceBaseDeviceNotSymlink(c *C) {
 	// only the by-filesystem-label symlink
 	fakedevice := filepath.Join(d.dir, "/dev/fakedevice")
 	c.Assert(os.Symlink(fakedevice, filepath.Join(d.dir, "/dev/disk/by-label/foo")), IsNil)
 	c.Assert(os.Symlink("../../fakedevice", filepath.Join(d.dir, "/dev/disk/by-partlabel/relative")), IsNil)
 
-	found, err := gadget.VerifyDeviceForStructure(
-		filepath.Join(d.dir, "/dev/disk/by-label/foo"),
-		&gadget.VolumeStructure{Name: "relative", EnclosingVolume: &gadget.Volume{}})
-	c.Check(err, IsNil)
-	c.Check(found, Equals, filepath.Join(d.dir, "/dev/fakedevice"))
+	found, err := gadget.ResolveDeviceForStructure(fakedevice)
+	c.Check(err, ErrorMatches, fmt.Sprintf(`candidate %s/dev/fakedevice is not a symlink`, d.dir))
+	c.Check(found, Equals, "")
 }
 
-func (d *deviceSuite) TestVerifyDeviceMatchesBaseDevice(c *C) {
+func (d *deviceSuite) TestResolveDeviceMatchesBaseDeviceAsSymlink(c *C) {
 	// only the by-filesystem-label symlink
 	fakedevice := filepath.Join(d.dir, "/dev/fakedevice")
 	c.Assert(os.Symlink(fakedevice, filepath.Join(d.dir, "/dev/disk/by-label/foo")), IsNil)
 	c.Assert(os.Symlink("../../fakedevice", filepath.Join(d.dir, "/dev/disk/by-partlabel/relative")), IsNil)
 
-	found, err := gadget.VerifyDeviceForStructure(fakedevice,
-		&gadget.VolumeStructure{Name: "relative", EnclosingVolume: &gadget.Volume{}})
+	found, err := gadget.ResolveDeviceForStructure(filepath.Join(d.dir, "/dev/disk/by-label/foo"))
 	c.Check(err, IsNil)
 	c.Check(found, Equals, filepath.Join(d.dir, "/dev/fakedevice"))
 }
 
-func (d *deviceSuite) TestVerifyDeviceNoMatchesBaseDevice(c *C) {
+func (d *deviceSuite) TestResolveDeviceNoMatchesBaseDevice(c *C) {
 	// fake two devices
 	// only the by-filesystem-label symlink
 	fakedevice0 := filepath.Join(d.dir, "/dev/fakedevice")
@@ -287,8 +285,7 @@ func (d *deviceSuite) TestVerifyDeviceNoMatchesBaseDevice(c *C) {
 
 	c.Assert(os.Symlink("../../fakedevice", filepath.Join(d.dir, "/dev/disk/by-partlabel/relative")), IsNil)
 
-	found, err := gadget.VerifyDeviceForStructure(fakedevice1,
-		&gadget.VolumeStructure{Name: "relative", EnclosingVolume: &gadget.Volume{}})
+	found, err := gadget.ResolveDeviceForStructure(fakedevice1)
 	c.Check(err, ErrorMatches, `device not found`)
 	c.Check(found, Equals, "")
 }

gadget/gadget.go

@@ -782,23 +782,54 @@ func LoadDiskVolumesDeviceTraits(dir string) (map[string]DiskVolumeDeviceTraits,
 	return mapping, nil
 }
 
+func deviceNodeForStructure(device string, vs *VolumeStructure) (string, error) {
+	if device != "" {
+		disk, err := disks.DiskFromDeviceName(device)
+		if err != nil {
+			return "", err
+		}
+
+		return disk.KernelDeviceNode(), nil
+	}
+
+	structureDevice, err := FindDeviceForStructure(vs)
+	if err != nil && err != ErrDeviceNotFound {
+		return "", err
+	}
+
+	if structureDevice != "" {
+		// we found a device for this structure, get the parent disk
+		// and save that as the device for this volume
+		disk, err := disks.DiskFromPartitionDeviceNode(structureDevice)
+		if err != nil {
+			return "", err
+		}
+
+		return disk.KernelDeviceNode(), nil
+	}
+	return "", nil
+}
+
 // AllDiskVolumeDeviceTraits takes a mapping of volume name to Volume
 // and produces a map of volume name to DiskVolumeDeviceTraits. Since
 // doing so uses DiskVolumeDeviceTraitsForDevice, it will also
 // validate that disk devices identified for the volume are compatible
 // and matching before returning.
 func AllDiskVolumeDeviceTraits(allVols map[string]*Volume, optsPerVolume map[string]*DiskVolumeValidationOptions) (map[string]DiskVolumeDeviceTraits, error) {
 	// build up the mapping of volumes to disk device traits
-
-	allTraits := map[string]DiskVolumeDeviceTraits{}
-
 	// find all devices which map to volumes to save the current state of the
 	// system
+	allTraits := map[string]DiskVolumeDeviceTraits{}
 	for name, vol := range allVols {
 		// try to find a device for a structure inside the volume, we have a
 		// loop to attempt to use all structures in the volume in case there are
 		// partitions we can't map to a device directly at first using the
 		// device symlinks that FindDeviceForStructure uses
+		opts := optsPerVolume[name]
+		if opts == nil {
+			opts = &DiskVolumeValidationOptions{}
+		}
+
 		dev := ""
 		for _, vs := range vol.Structure {
 			// TODO: This code works for volumes that have at least one
@@ -811,26 +842,16 @@ func AllDiskVolumeDeviceTraits(allVols map[string]*Volume, optsPerVolume map[str
 			// locations, aside from potentially reading and comparing the bytes
 			// at the expected locations, but that is probably fragile and very
 			// non-performant.
-
 			if !vs.IsPartition() {
 				// skip trying to find non-partitions on disk, it won't work
 				continue
 			}
 
-			structureDevice, err := FindDeviceForStructure(&vs)
-			if err != nil && err != ErrDeviceNotFound {
+			devNode, err := deviceNodeForStructure(opts.Device, &vs)
+			if err != nil {
 				return nil, err
-			}
-			if structureDevice != "" {
-				// we found a device for this structure, get the parent disk
-				// and save that as the device for this volume
-				disk, err := disks.DiskFromPartitionDeviceNode(structureDevice)
-				if err != nil {
-					return nil, err
-				}
-
-				dev = disk.KernelDeviceNode()
-				break
+			} else if devNode != "" {
+				dev = devNode
 			}
 		}
 
@@ -841,10 +862,6 @@ func AllDiskVolumeDeviceTraits(allVols map[string]*Volume, optsPerVolume map[str
 		// now that we have a candidate device for this disk, build up the
 		// traits for it, this will also validate concretely that the
 		// device we picked and the volume are compatible
-		opts := optsPerVolume[name]
-		if opts == nil {
-			opts = &DiskVolumeValidationOptions{}
-		}
 		traits, err := DiskTraitsFromDeviceAndValidate(vol, dev, opts)
 		if err != nil {
 			return nil, fmt.Errorf("cannot gather disk traits for device %s to use with volume %s: %v", dev, name, err)

gadget/install/install.go

@@ -402,7 +402,7 @@ func Run(model gadget.Model, gadgetRoot string, kernelSnapInfo *KernelSnapInfo,
 
 	// Step 1: create partitions
 	kernelRoot := kernelSnapInfo.MountPoint
-	bootVolGadgetName, created, bootVolSectorSize, err :=
+	bootVolumeName, created, bootVolSectorSize, err :=
 		createPartitions(volumes, gadgetRoot, bootDevice, perfTimings)
 	if err != nil {
 		return nil, err
@@ -465,12 +465,27 @@ func Run(model gadget.Model, gadgetRoot string, kernelSnapInfo *KernelSnapInfo,
 
 	// after we have created all partitions, build up the mapping of volumes
 	// to disk device traits and save it to disk for later usage
-	optsPerVol := map[string]*gadget.DiskVolumeValidationOptions{
-		// this assumes that the encrypted partitions above are always only on the
-		// system-boot volume, this assumption may change
-		bootVolGadgetName: {
-			ExpectedStructureEncryption: partsEncrypted,
-		},
+	optsPerVol := make(map[string]*gadget.DiskVolumeValidationOptions)
+	traitVolumes := make(map[string]*gadget.Volume)
+	for name, vol := range volumes {
+		traitVolumes[name] = vol.Volume
+		if name == bootVolumeName {
+			// this assumes that the encrypted partitions above are always only on the
+			// system-boot volume, this assumption may change
+			optsPerVol[name] = &gadget.DiskVolumeValidationOptions{
+				Device:                      vol.Device,
+				ExpectedStructureEncryption: partsEncrypted,
+			}
+		} else {
+			optsPerVol[name] = &gadget.DiskVolumeValidationOptions{
+				Device: vol.Device,
+			}
+		}
+	}
+
+	// save the traits to ubuntu-data host and optionally to ubuntu-save if it exists
+	if err := saveStorageTraits(model, traitVolumes, optsPerVol, hasSavePartition); err != nil {
+		return nil, err
 	}
 
 	// save the traits to ubuntu-data host and optionally to ubuntu-save if it exists
@@ -861,18 +876,28 @@ func FactoryReset(model gadget.Model, gadgetRoot string, kernelSnapInfo *KernelS
 
 	// after we have created all partitions, build up the mapping of volumes
 	// to disk device traits and save it to disk for later usage
-	optsPerVol := map[string]*gadget.DiskVolumeValidationOptions{
-		// this assumes that the encrypted partitions above are always only on the
-		// system-boot volume, this assumption may change
-		bootVol.Volume.Name: {
-			ExpectedStructureEncryption: volCompatOps.ExpectedStructureEncryption,
-		},
+	optsPerVol := make(map[string]*gadget.DiskVolumeValidationOptions)
+	traitVolumes := make(map[string]*gadget.Volume)
+	for name, vol := range volumes {
+		traitVolumes[name] = vol.Volume
+		if name == bootVolumeName {
+			// this assumes that the encrypted partitions above are always only on the
+			// system-boot volume, this assumption may change
+			optsPerVol[name] = &gadget.DiskVolumeValidationOptions{
+				Device:                      vol.Device,
+				ExpectedStructureEncryption: volCompatOps.ExpectedStructureEncryption,
+			}
+		} else {
+			optsPerVol[name] = &gadget.DiskVolumeValidationOptions{
+				Device: vol.Device,
+			}
+		}
 	}
+
 	// save the traits to ubuntu-data host and optionally to ubuntu-save if it exists
-	if err := saveStorageTraits(model, info.Volumes, optsPerVol, hasSavePartition); err != nil {
+	if err := saveStorageTraits(model, traitVolumes, optsPerVol, hasSavePartition); err != nil {
 		return nil, err
 	}
-
 	return &InstalledSystemSideData{
 		KeyForRole:    keyForRole,
 		DeviceForRole: deviceForRole,

gadget/update.go

@@ -638,6 +638,8 @@ const (
 // options provided to EnsureVolumeCompatibility via
 // EnsureVolumeCompatibilityOptions.
 type DiskVolumeValidationOptions struct {
+	// Device allows to specify a specific disk path for the volume
+	Device string
 	// AllowImplicitSystemData has the same meaning as the eponymously named
 	// field in VolumeCompatibilityOptions.
 	AllowImplicitSystemData bool
@@ -871,17 +873,16 @@ func buildNewVolumeToDeviceMapping(mod Model, oldVolumes, newVolumes map[string]
 	// search for matching devices that correspond to the gadget volume
 	dev := ""
 	for i := range vol.Volume.Structure {
-		// here it is okay that we require there to be either a partition label
-		// or a filesystem label since we require there to be a system-boot role
-		// on this volume which by definition must have a filesystem
 		var structureDevice string
 		var err error
 		if vol.Device != "" {
 			// If a specific device is assigned for the volume, then we must
-			// use that, so we verify that the structure will fit onto it and
-			// the device is valid.
-			structureDevice, err = VerifyDeviceForStructure(vol.Device, &vol.Volume.Structure[i])
+			// use that, so we verify that the device is valid.
+			structureDevice, err = ResolveDeviceForStructure(vol.Device)
 		} else {
+			// here it is okay that we require there to be either a partition label
+			// or a filesystem label since we require there to be a system-boot role
+			// on this volume which by definition must have a filesystem
 			structureDevice, err = FindDeviceForStructure(&vol.Volume.Structure[i])
 		}
 		if err == ErrDeviceNotFound {