From 073af33d588629dae80fc797ffc359c8535a2fd1 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 8 Oct 2021 14:36:45 -0500 Subject: gadget/gadget.go: SaveDiskVolumesDeviceTraits needs to take a dir actually Sadly, since we need to save this information on UC20+ during install mode, we actually need to write to boot.InstallHostWritableDir, but we can't import boot into gadget where SaveDiskVolumesDeviceTraits is defined, so instead we need it to take an argument of the dir to save the file under. Signed-off-by: Ian Johnson --- gadget/gadget.go | 5 ++--- gadget/gadget_test.go | 10 +++++++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/gadget/gadget.go b/gadget/gadget.go index caf92f0da7..b1bad65a60 100644 --- a/gadget/gadget.go +++ b/gadget/gadget.go @@ -273,14 +273,13 @@ type DiskStructureDeviceTraits struct { // SaveDiskVolumesDeviceTraits saves the mapping of volume names to volume / device // traits to a file on disk for later loading and verification. -func SaveDiskVolumesDeviceTraits(mapping map[string]DiskVolumeDeviceTraits) error { +func SaveDiskVolumesDeviceTraits(dir string, mapping map[string]DiskVolumeDeviceTraits) error { b, err := json.Marshal(mapping) if err != nil { return err } - // TODO: should this live in dirs? - filename := filepath.Join(dirs.SnapDeviceDir, "disk-mapping.json") + filename := filepath.Join(dir, "disk-mapping.json") if err := os.MkdirAll(filepath.Dir(filename), 0755); err != nil { return err diff --git a/gadget/gadget_test.go b/gadget/gadget_test.go index c54f0c730f..67c0cc1b18 100644 --- a/gadget/gadget_test.go +++ b/gadget/gadget_test.go @@ -3038,9 +3038,17 @@ func (s *gadgetYamlTestSuite) TestSaveLoadDiskVolumeDeviceTraits(c *C) { c.Assert(err, IsNil) c.Assert(mAbsent, HasLen, 0) - err = gadget.SaveDiskVolumesDeviceTraits(m) + // load looks in SnapDeviceDir since it is meant to be used during run mode + // when /var/lib/snapd/device/disk-mapping.json is the real version from + // ubuntu-data, but during install mode, we will need to save to the host + // ubuntu-data which is not located at /run/mnt/data or + // /var/lib/snapd/device, but rather + // /run/mnt/ubuntu-data/system-data/var/lib/snapd/device so this takes a + // directory argument when we save it + err = gadget.SaveDiskVolumesDeviceTraits(dirs.SnapDeviceDir, m) c.Assert(err, IsNil) + // now that it was saved to dirs.SnapDeviceDir, we can load it correctly m2, err := gadget.LoadDiskVolumesDeviceTraits() c.Assert(err, IsNil) -- cgit v1.2.3 From b096b4fbf9ec118644de0d07a921ec5797dec313 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Fri, 8 Oct 2021 15:33:03 -0500 Subject: osutil/disks: set hasPartitions from diskFromUdevProps This is used in DiskFromDeviceName and DiskFromDevicePath, and the issue is that if the returned disk did not have hasPartitions set to true, then calls to disk.Partitions() would always hard-code returning nil, which is wrong for many disks. There is still a wart that needs to be sorted out around mapper devices of the sort we get by mounting a .img file with partitions via kpartx at least, since there we don't have real partitions show up underneath the root "disk" device in /dev, not sure the proper thing to do there yet unfortunately... Signed-off-by: Ian Johnson --- osutil/disks/disks_linux.go | 21 ++++++++++++--- osutil/disks/disks_linux_test.go | 57 ++++++++++++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/osutil/disks/disks_linux.go b/osutil/disks/disks_linux.go index 7f2c05ac43..f73f3ceb2b 100644 --- a/osutil/disks/disks_linux.go +++ b/osutil/disks/disks_linux.go @@ -144,11 +144,24 @@ func diskFromUdevProps(deviceIdentifier string, deviceIDType string, props map[s // create the full path by pre-pending /sys, since udev doesn't include /sys devpath = filepath.Join(dirs.SysfsDir, devpath) + // TODO: this doesn't seem to work for /dev/mapper/loop1p1 devices like we + // get in the spread test for uc20-create-partitions* tests, needs more + // investigation + + // check if the device has partitions by attempting to actually search for + // them in /sys with the DEVPATH and DEVNAME + + paths, err := filepath.Glob(filepath.Join(devpath, filepath.Base(devname)+"*")) + if err != nil { + return nil, fmt.Errorf("internal error with glob pattern: %v", err) + } + return &disk{ - major: major, - minor: minor, - devname: devname, - devpath: devpath, + major: major, + minor: minor, + devname: devname, + devpath: devpath, + hasPartitions: len(paths) != 0, }, nil } diff --git a/osutil/disks/disks_linux_test.go b/osutil/disks/disks_linux_test.go index f12a5dcd6c..fb8f5f8170 100644 --- a/osutil/disks/disks_linux_test.go +++ b/osutil/disks/disks_linux_test.go @@ -89,8 +89,11 @@ var ( } ) -func createVirtioDevicesInSysfs(c *C, devsToPartition map[string]bool) { - diskDir := filepath.Join(dirs.SysfsDir, virtioDiskDevPath) +func createVirtioDevicesInSysfs(c *C, path string, devsToPartition map[string]bool) { + if path == "" { + path = virtioDiskDevPath + } + diskDir := filepath.Join(dirs.SysfsDir, path) for dev, isPartition := range devsToPartition { err := os.MkdirAll(filepath.Join(diskDir, dev), 0755) c.Assert(err, IsNil) @@ -111,7 +114,7 @@ func (s *diskSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) } -func (s *diskSuite) TestDiskFromNameHappy(c *C) { +func (s *diskSuite) TestDiskFromDeviceNameHappy(c *C) { const sdaSysfsPath = "/devices/pci0000:00/0000:00:01.1/0000:01:00.1/ata1/host0/target0:0:0/0:0:0:0/block/sda" restore := disks.MockUdevPropertiesForDevice(func(typeOpt, dev string) (map[string]string, error) { c.Assert(typeOpt, Equals, "--name") @@ -131,13 +134,29 @@ func (s *diskSuite) TestDiskFromNameHappy(c *C) { c.Assert(d.Dev(), Equals, "1:2") c.Assert(d.KernelDeviceNode(), Equals, "/dev/sda") c.Assert(d.KernelDevicePath(), Equals, filepath.Join(dirs.SysfsDir, sdaSysfsPath)) + // it doesn't have any partitions since we didn't mock any in sysfs + c.Assert(d.HasPartitions(), Equals, false) + + // if we mock some sysfs partitions then it has partitions when we it has + // some partitions on it it + createVirtioDevicesInSysfs(c, sdaSysfsPath, map[string]bool{ + "sda1": true, + "sda2": true, + }) + + d, err = disks.DiskFromDeviceName("sda") + c.Assert(err, IsNil) + c.Assert(d.Dev(), Equals, "1:2") + c.Assert(d.KernelDeviceNode(), Equals, "/dev/sda") + c.Assert(d.HasPartitions(), Equals, true) } -func (s *diskSuite) TestDiskFromPathHappy(c *C) { +func (s *diskSuite) TestDiskFromDevicePathHappy(c *C) { const vdaSysfsPath = "/devices/pci0000:00/0000:00:04.0/virtio2/block/vdb" + fullSysPath := filepath.Join("/sys", vdaSysfsPath) restore := disks.MockUdevPropertiesForDevice(func(typeOpt, dev string) (map[string]string, error) { c.Assert(typeOpt, Equals, "--path") - c.Assert(dev, Equals, filepath.Join("/sys", vdaSysfsPath)) + c.Assert(dev, Equals, fullSysPath) return map[string]string{ "MAJOR": "1", "MINOR": "2", @@ -148,15 +167,31 @@ func (s *diskSuite) TestDiskFromPathHappy(c *C) { }) defer restore() - d, err := disks.DiskFromDevicePath(filepath.Join("/sys", vdaSysfsPath)) + d, err := disks.DiskFromDevicePath(fullSysPath) c.Assert(err, IsNil) c.Assert(d.Dev(), Equals, "1:2") c.Assert(d.KernelDeviceNode(), Equals, "/dev/vdb") // note that we don't always prepend exactly /sys, we use dirs.SysfsDir c.Assert(d.KernelDevicePath(), Equals, filepath.Join(dirs.SysfsDir, vdaSysfsPath)) + + // it doesn't have any partitions since we didn't mock any in sysfs + c.Assert(d.HasPartitions(), Equals, false) + + // if we mock some sysfs partitions then it has partitions when we it has + // some partitions on it it + createVirtioDevicesInSysfs(c, vdaSysfsPath, map[string]bool{ + "vdb1": true, + "vdb2": true, + }) + + d, err = disks.DiskFromDevicePath(fullSysPath) + c.Assert(err, IsNil) + c.Assert(d.Dev(), Equals, "1:2") + c.Assert(d.KernelDeviceNode(), Equals, "/dev/vdb") + c.Assert(d.HasPartitions(), Equals, true) } -func (s *diskSuite) TestDiskFromNameUnhappyPartition(c *C) { +func (s *diskSuite) TestDiskFromDeviceNameUnhappyPartition(c *C) { restore := disks.MockUdevPropertiesForDevice(func(typeOpt, dev string) (map[string]string, error) { c.Assert(typeOpt, Equals, "--name") c.Assert(dev, Equals, "sda1") @@ -172,7 +207,7 @@ func (s *diskSuite) TestDiskFromNameUnhappyPartition(c *C) { c.Assert(err, ErrorMatches, "device \"sda1\" is not a disk, it has DEVTYPE of \"partition\"") } -func (s *diskSuite) TestDiskFromNameUnhappyBadUdevOutput(c *C) { +func (s *diskSuite) TestDiskFromDeviceNameUnhappyBadUdevOutput(c *C) { restore := disks.MockUdevPropertiesForDevice(func(typeOpt, dev string) (map[string]string, error) { c.Assert(typeOpt, Equals, "--name") c.Assert(dev, Equals, "sda") @@ -346,7 +381,7 @@ func (s *diskSuite) TestDiskFromMountPointHappySinglePartitionIgnoresNonPartitio // create just the single valid partition in sysfs, and an invalid // non-partition device that we should ignore - createVirtioDevicesInSysfs(c, map[string]bool{ + createVirtioDevicesInSysfs(c, "", map[string]bool{ "vda4": true, "vda5": false, }) @@ -601,7 +636,7 @@ func (s *diskSuite) TestDiskFromMountPointPartitionsHappy(c *C) { defer restore() // create all 4 partitions as device nodes in sysfs - createVirtioDevicesInSysfs(c, map[string]bool{ + createVirtioDevicesInSysfs(c, "", map[string]bool{ "vda1": true, "vda2": true, "vda3": true, @@ -833,7 +868,7 @@ func (s *diskSuite) TestDiskFromMountPointDecryptedDevicePartitionsHappy(c *C) { c.Assert(err, IsNil) // mock the dev nodes in sysfs for the partitions - createVirtioDevicesInSysfs(c, map[string]bool{ + createVirtioDevicesInSysfs(c, "", map[string]bool{ "vda1": true, "vda2": true, "vda3": true, -- cgit v1.2.3 From 7ea2047929511b06650049adcdeb18834d2e7181 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Mon, 11 Oct 2021 11:05:50 -0500 Subject: osutil/disks/disks_linux.go: rm TODO about more investigation More investigation isn't necessary for the spread test at least, the spread test is doing the right thing, but my test scripts are not allegedly. Signed-off-by: Ian Johnson --- osutil/disks/disks_linux.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/osutil/disks/disks_linux.go b/osutil/disks/disks_linux.go index f73f3ceb2b..bcb1e77999 100644 --- a/osutil/disks/disks_linux.go +++ b/osutil/disks/disks_linux.go @@ -144,10 +144,6 @@ func diskFromUdevProps(deviceIdentifier string, deviceIDType string, props map[s // create the full path by pre-pending /sys, since udev doesn't include /sys devpath = filepath.Join(dirs.SysfsDir, devpath) - // TODO: this doesn't seem to work for /dev/mapper/loop1p1 devices like we - // get in the spread test for uc20-create-partitions* tests, needs more - // investigation - // check if the device has partitions by attempting to actually search for // them in /sys with the DEVPATH and DEVNAME -- cgit v1.2.3 From 2543412cb6b4ba0cc2495e40f73f67632ba8ab36 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Wed, 13 Oct 2021 10:01:05 -0500 Subject: gadget: take a directory argument in LoadDiskVolumesDeviceTraits too Save needs a directory so for symmetry Load should take one too. Thanks to Samuele for the suggestion. Signed-off-by: Ian Johnson --- gadget/gadget.go | 9 ++++----- gadget/gadget_test.go | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/gadget/gadget.go b/gadget/gadget.go index b1bad65a60..1004c14f6f 100644 --- a/gadget/gadget.go +++ b/gadget/gadget.go @@ -35,7 +35,6 @@ import ( "gopkg.in/yaml.v2" "github.com/snapcore/snapd/asserts" - "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/gadget/edition" "github.com/snapcore/snapd/gadget/quantity" "github.com/snapcore/snapd/metautil" @@ -271,8 +270,8 @@ type DiskStructureDeviceTraits struct { Size quantity.Size `json:"size"` } -// SaveDiskVolumesDeviceTraits saves the mapping of volume names to volume / device -// traits to a file on disk for later loading and verification. +// SaveDiskVolumesDeviceTraits saves the mapping of volume names to volume / +// device traits to a file on disk for later loading and verification. func SaveDiskVolumesDeviceTraits(dir string, mapping map[string]DiskVolumeDeviceTraits) error { b, err := json.Marshal(mapping) if err != nil { @@ -290,10 +289,10 @@ func SaveDiskVolumesDeviceTraits(dir string, mapping map[string]DiskVolumeDevice // LoadDiskVolumesDeviceTraits loads the mapping of volumes to disk traits if // there is any. If there is no file with the mapping available, nil is // returned. -func LoadDiskVolumesDeviceTraits() (map[string]DiskVolumeDeviceTraits, error) { +func LoadDiskVolumesDeviceTraits(dir string) (map[string]DiskVolumeDeviceTraits, error) { var mapping map[string]DiskVolumeDeviceTraits - filename := filepath.Join(dirs.SnapDeviceDir, "disk-mapping.json") + filename := filepath.Join(dir, "disk-mapping.json") if !osutil.FileExists(filename) { return nil, nil } diff --git a/gadget/gadget_test.go b/gadget/gadget_test.go index 67c0cc1b18..a8817b34da 100644 --- a/gadget/gadget_test.go +++ b/gadget/gadget_test.go @@ -3034,7 +3034,7 @@ func (s *gadgetYamlTestSuite) TestSaveLoadDiskVolumeDeviceTraits(c *C) { // when there is no mapping file, it is not an error, the map returned is // just nil/has no items in it - mAbsent, err := gadget.LoadDiskVolumesDeviceTraits() + mAbsent, err := gadget.LoadDiskVolumesDeviceTraits(dirs.SnapDeviceDir) c.Assert(err, IsNil) c.Assert(mAbsent, HasLen, 0) @@ -3049,7 +3049,7 @@ func (s *gadgetYamlTestSuite) TestSaveLoadDiskVolumeDeviceTraits(c *C) { c.Assert(err, IsNil) // now that it was saved to dirs.SnapDeviceDir, we can load it correctly - m2, err := gadget.LoadDiskVolumesDeviceTraits() + m2, err := gadget.LoadDiskVolumesDeviceTraits(dirs.SnapDeviceDir) c.Assert(err, IsNil) c.Assert(m, DeepEquals, m2) -- cgit v1.2.3 From bfd32f2a2ce2a829b94d1085fd8d2e4fa136fdf1 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Mon, 18 Oct 2021 10:45:50 -0500 Subject: gadget/gadget.go: fix doc-comment Thanks to Maciej for the suggestion Co-authored-by: Maciej Borzecki --- gadget/gadget.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gadget/gadget.go b/gadget/gadget.go index 65dc3021be..7129af57f2 100644 --- a/gadget/gadget.go +++ b/gadget/gadget.go @@ -272,7 +272,8 @@ type DiskStructureDeviceTraits struct { } // SaveDiskVolumesDeviceTraits saves the mapping of volume names to volume / -// device traits to a file on disk for later loading and verification. +// device traits to a file inside the provided directory on disk for +// later loading and verification. func SaveDiskVolumesDeviceTraits(dir string, mapping map[string]DiskVolumeDeviceTraits) error { b, err := json.Marshal(mapping) if err != nil { -- cgit v1.2.3 From ec6dff9e139406b871d47ed7e0ac6025283fb6d3 Mon Sep 17 00:00:00 2001 From: Ian Johnson Date: Mon, 18 Oct 2021 11:19:55 -0500 Subject: gadget/gadget.go: fix whitespace Thanks gofmt Signed-off-by: Ian Johnson --- gadget/gadget.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gadget/gadget.go b/gadget/gadget.go index 7129af57f2..a0da7a8830 100644 --- a/gadget/gadget.go +++ b/gadget/gadget.go @@ -272,7 +272,7 @@ type DiskStructureDeviceTraits struct { } // SaveDiskVolumesDeviceTraits saves the mapping of volume names to volume / -// device traits to a file inside the provided directory on disk for +// device traits to a file inside the provided directory on disk for // later loading and verification. func SaveDiskVolumesDeviceTraits(dir string, mapping map[string]DiskVolumeDeviceTraits) error { b, err := json.Marshal(mapping) -- cgit v1.2.3