From 4da7422f045cced003c74eea82f404bd18cfcbb6 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Thu, 3 Oct 2024 15:00:14 +0200 Subject: [PATCH] gadget: fix discovering of multiple disks when building disk traits --- gadget/device_darwin.go | 2 +- gadget/device_linux.go | 38 +++++-------------------- gadget/device_test.go | 25 ++++++++--------- gadget/gadget.go | 59 +++++++++++++++++++++++++-------------- gadget/install/install.go | 55 ++++++++++++++++++++++++++---------- gadget/update.go | 13 +++++---- 6 files changed, 104 insertions(+), 88 deletions(-) diff --git a/gadget/device_darwin.go b/gadget/device_darwin.go index 0ae853be87f..b0a10598524 100644 --- a/gadget/device_darwin.go +++ b/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 } diff --git a/gadget/device_linux.go b/gadget/device_linux.go index 43cada36dd1..dd125e39624 100644 --- a/gadget/device_linux.go +++ b/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}) } diff --git a/gadget/device_test.go b/gadget/device_test.go index c44761c59c6..59b28c42e1b 100644 --- a/gadget/device_test.go +++ b/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, "") } diff --git a/gadget/gadget.go b/gadget/gadget.go index 849835577a7..248d0687af1 100644 --- a/gadget/gadget.go +++ b/gadget/gadget.go @@ -782,6 +782,34 @@ 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 @@ -789,16 +817,19 @@ func LoadDiskVolumesDeviceTraits(dir string) (map[string]DiskVolumeDeviceTraits, // 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) diff --git a/gadget/install/install.go b/gadget/install/install.go index 94ce9f96726..30f50d72063 100644 --- a/gadget/install/install.go +++ b/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, diff --git a/gadget/update.go b/gadget/update.go index 68bf8f6df1b..54e7209a74f 100644 --- a/gadget/update.go +++ b/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 {