From bd4dcc565c93dc5de629deba915fb17a04f9b199 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 15 Sep 2020 13:55:15 +1000 Subject: [PATCH 1/2] device: Rename pciDeviceMap in sandbox struct pciDeviceMap is essentially just a map from sysfs paths to corresponding device nodes. The only thing PCI related is that the sysfs paths we're usually looking up in it usually involve PCI addresses. There's no reason that has to be the case, however, and I have future uses which will not be PCI. Therefore, rename it (and change some surrounding comments) to reflect its more general nature. While we're there, fix a typo in a related comment. Signed-off-by: David Gibson --- agent.go | 14 +++++++------- device.go | 4 ++-- device_test.go | 8 ++++---- mount_test.go | 10 +++++----- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/agent.go b/agent.go index 33ee869f2c..031600296d 100644 --- a/agent.go +++ b/agent.go @@ -133,7 +133,7 @@ type sandbox struct { mounts []string subreaper reaper server *grpc.Server - pciDeviceMap map[string]string + sysToDevMap map[string]string deviceWatchers map[string](chan string) sharedUTSNs namespace sharedIPCNs namespace @@ -742,9 +742,9 @@ func (s *sandbox) listenToUdevEvents() { }) if uEv.Action == "remove" { - fieldLogger.Infof("Remove dev from pciDeviceMap") + fieldLogger.Infof("Remove dev from sysToDevMap") s.Lock() - delete(s.pciDeviceMap, uEv.DevPath) + delete(s.sysToDevMap, uEv.DevPath) s.Unlock() goto FINISH_SPAN } @@ -758,12 +758,12 @@ func (s *sandbox) listenToUdevEvents() { // Check if device hotplug event results in a device node being created. if uEv.DevName != "" && (strings.HasPrefix(uEv.DevPath, rootBusPath) || strings.HasPrefix(uEv.DevPath, acpiDevPath)) { - // Lock is needed to safey read and modify the pciDeviceMap and deviceWatchers. + // Lock is needed to safely read and modify the sysToDevMap and deviceWatchers. // This makes sure that watchers do not access the map while it is being updated. s.Lock() - // Add the device node name to the pci device map. - s.pciDeviceMap[uEv.DevPath] = uEv.DevName + // Add the device node name to the device map. + s.sysToDevMap[uEv.DevPath] = uEv.DevName // Notify watchers that are interested in the udev event. // Close the channel after watcher has been notified. @@ -1548,7 +1548,7 @@ func realMain() error { // Documentation/filesystem/ramfs-rootfs-initramfs.txt noPivotRoot: (fsType == typeRootfs), subreaper: r, - pciDeviceMap: make(map[string]string), + sysToDevMap: make(map[string]string), deviceWatchers: make(map[string](chan string)), storages: make(map[string]*sandboxStorage), stopServer: make(chan struct{}), diff --git a/device.go b/device.go index 28d98a063a..3c8b83c24d 100644 --- a/device.go +++ b/device.go @@ -144,10 +144,10 @@ func getDeviceName(s *sandbox, devID string) (string, error) { // Check if the dev identifier is in PCI device map. s.Lock() - for key, value := range s.pciDeviceMap { + for key, value := range s.sysToDevMap { if strings.Contains(key, devID) { devName = value - fieldLogger.Infof("Device: %s found in pci device map", devID) + fieldLogger.Infof("Device: %s found in device map", devID) break } } diff --git a/device_test.go b/device_test.go index 51b9df04fa..e9d928b590 100644 --- a/device_test.go +++ b/device_test.go @@ -872,12 +872,12 @@ func TestGetDeviceName(t *testing.T) { busID := "0.0.0005" devPath := path.Join("/devices/css0/0.0.0004", busID, "virtio4/block", devName) - pcidevicemap := make(map[string]string) - pcidevicemap[devPath] = devName + systodevmap := make(map[string]string) + systodevmap[devPath] = devName sb := sandbox{ deviceWatchers: make(map[string](chan string)), - pciDeviceMap: pcidevicemap, + sysToDevMap: systodevmap, } name, err := getDeviceName(&sb, busID) @@ -885,7 +885,7 @@ func TestGetDeviceName(t *testing.T) { assert.Nil(err) assert.Equal(name, path.Join(devRootPath, devName)) - delete(sb.pciDeviceMap, devPath) + delete(sb.sysToDevMap, devPath) go func() { for { diff --git a/mount_test.go b/mount_test.go index 823472ac6f..913098a9ec 100644 --- a/mount_test.go +++ b/mount_test.go @@ -248,11 +248,11 @@ func TestVirtioBlkStorageHandlerSuccessful(t *testing.T) { defer syscall.Unmount(storage.MountPoint, 0) s := &sandbox{ - pciDeviceMap: make(map[string]string), + sysToDevMap: make(map[string]string), } s.Lock() - s.pciDeviceMap[completePCIAddr] = devPath + s.sysToDevMap[completePCIAddr] = devPath s.Unlock() storage.Fstype = "bind" @@ -268,7 +268,7 @@ func TestVirtioBlkStorageHandlerSuccessful(t *testing.T) { func TestNvdimmStorageHandlerSuccessful(t *testing.T) { skipUnlessRoot(t) - completePCIAddr := "/devices/LNXSYSTM/LNXSYBUS/ACPI/ndbus0/region1/pfn1.1/block/pmem0" + sysfsPath := "/devices/LNXSYSTM/LNXSYBUS/ACPI/ndbus0/region1/pfn1.1/block/pmem0" pmemDev := "/dev/pmem0" devPath, err := createFakeDevicePath() if err != nil { @@ -289,11 +289,11 @@ func TestNvdimmStorageHandlerSuccessful(t *testing.T) { defer syscall.Unmount(storage.MountPoint, 0) s := &sandbox{ - pciDeviceMap: make(map[string]string), + sysToDevMap: make(map[string]string), } s.Lock() - s.pciDeviceMap[completePCIAddr] = devPath + s.sysToDevMap[sysfsPath] = devPath s.Unlock() storage.Fstype = "bind" From 0a4d443c81f25f04453aac5be9db616a0e0de391 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Tue, 22 Sep 2020 15:31:28 +1000 Subject: [PATCH 2/2] device: Simplify uevent matching in listenToUdevEvents() This function watches for uvents that match addresses stored in s.deviceWatchers() and if there's a matching entry signals the associated channel. "Matching" in this instance means soem sort of comparison between the address we're watching for and the sysfs path in the uevent. Currently we match any of a bunch of different cases for different types of device. Being very specific with how we match *sounds* like a good idea, but it's complicated, tightly coupled to other parts of the code and, ultimately, pointless. The user of the data we're collecting here is getDeviceName(). Before registering a watcher it checks s.sysToDevMap in case the uevent it's interested in has already happened. To have consistent behaviour regardless of when the uevent arrives, the matching that getDeviceName() uses against s.sysToDevMap must be the same as we use in listenToUdevEvents() and right now, it's not. So, change the matching in listenToUdevEvents() to be the same as the much simpler version in getDeviceName(). fixes #848 Signed-off-by: David Gibson --- agent.go | 32 ++++++-------------------------- device_amd64.go | 4 ---- device_arm64.go | 4 ---- device_ppc64le.go | 4 ---- device_s390x.go | 4 ---- 5 files changed, 6 insertions(+), 42 deletions(-) diff --git a/agent.go b/agent.go index 031600296d..4c0ff340bf 100644 --- a/agent.go +++ b/agent.go @@ -774,34 +774,14 @@ func (s *sandbox) listenToUdevEvents() { fieldLogger.Infof("Got a wait channel for device %s", devAddress) - // blk driver case - if strings.HasPrefix(uEv.DevPath, filepath.Join(rootBusPath, devAddress)) { - goto OUT - } - - // pmem/nvdimm case - if strings.Contains(uEv.DevPath, pfnDevPrefix) && strings.HasSuffix(uEv.DevPath, devAddress) { - goto OUT - } - + // This is a pretty imperfect way of + // matching, but it's the same as we + // use in getDeviceName() if strings.Contains(uEv.DevPath, devAddress) { - // scsi driver case - if strings.HasSuffix(devAddress, scsiBlockSuffix) { - goto OUT - } - // blk-ccw driver case - if strings.HasSuffix(devAddress, blkCCWSuffix) { - goto OUT - } + ch <- uEv.DevName + close(ch) + delete(s.deviceWatchers, devAddress) } - - continue - - OUT: - ch <- uEv.DevName - close(ch) - delete(s.deviceWatchers, devAddress) - } s.Unlock() diff --git a/device_amd64.go b/device_amd64.go index 66bc0522a8..fe9f8b785d 100644 --- a/device_amd64.go +++ b/device_amd64.go @@ -16,8 +16,4 @@ const ( // processors, thermal zones. Those objects are exported to user space via // sysfs as directories in the subtree under /sys/devices/LNXSYSTM:00 acpiDevPath = "/devices/LNXSYSTM" - - // /dev/pmemX devices exported in the ACPI sysfs (/devices/LNXSYSTM) are - // in a subdirectory whose prefix is pfn (page frame number). - pfnDevPrefix = "/pfn" ) diff --git a/device_arm64.go b/device_arm64.go index b73b582d45..11c4e70d84 100644 --- a/device_arm64.go +++ b/device_arm64.go @@ -15,8 +15,4 @@ const ( // processors, thermal zones. Those objects are exported to user space via // sysfs as directories in the subtree under /sys/devices/LNXSYSTM:00 acpiDevPath = "/devices/LNXSYSTM" - - // /dev/pmemX devices exported in the ACPI sysfs (/devices/LNXSYSTM) are - // in a subdirectory whose prefix is pfn (page frame number). - pfnDevPrefix = "/pfn" ) diff --git a/device_ppc64le.go b/device_ppc64le.go index 66bc0522a8..fe9f8b785d 100644 --- a/device_ppc64le.go +++ b/device_ppc64le.go @@ -16,8 +16,4 @@ const ( // processors, thermal zones. Those objects are exported to user space via // sysfs as directories in the subtree under /sys/devices/LNXSYSTM:00 acpiDevPath = "/devices/LNXSYSTM" - - // /dev/pmemX devices exported in the ACPI sysfs (/devices/LNXSYSTM) are - // in a subdirectory whose prefix is pfn (page frame number). - pfnDevPrefix = "/pfn" ) diff --git a/device_s390x.go b/device_s390x.go index dd8cecab58..e28074c44d 100644 --- a/device_s390x.go +++ b/device_s390x.go @@ -16,8 +16,4 @@ const ( // processors, thermal zones. Those objects are exported to user space via // sysfs as directories in the subtree under /sys/devices/LNXSYSTM:00 acpiDevPath = "/devices/LNXSYSTM" - - // /dev/pmemX devices exported in the ACPI sysfs (/devices/LNXSYSTM) are - // in a subdirectory whose prefix is pfn (page frame number). - pfnDevPrefix = "/pfn" )