podman-update: fix block device handling

docs/source/markdown/podman-update.1.md.in

@@ -95,21 +95,21 @@ Changing this setting resets the timer, depending on the state of the container.
 @@option unsetenv.update
 
 
-## EXAMPLEs
+## EXAMPLES
 
-Update a container with a new cpu quota and period.
+Update a container with a new cpu quota and period:
 ```
-podman update --cpus=5 myCtr
+podman update --cpus=0.5 ctrID
 ```
 
-Update a container with all available options for cgroups v2.
+Update a container with multiple options at ones:
 ```
-podman update --cpus 5 --cpuset-cpus 0 --cpu-shares 123 --cpuset-mems 0 --memory 1G --memory-swap 2G --memory-reservation 2G --blkio-weight-device /dev/zero:123 --blkio-weight 123 --device-read-bps /dev/zero:10mb --device-write-bps /dev/zero:10mb --device-read-iops /dev/zero:1000 --device-write-iops /dev/zero:1000 --pids-limit 123 ctrID
-```
-
-Update a container with all available options for cgroups v1.
-```
-podman update --cpus 5 --cpuset-cpus 0 --cpu-shares 123 --cpuset-mems 0 --memory 1G --memory-swap 2G --memory-reservation 2G --memory-swappiness 50 --pids-limit 123 ctrID
+podman update --cpus 5 --cpuset-cpus 0 --cpu-shares 123 --cpuset-mems 0 \\
+  --memory 1G --memory-swap 2G --memory-reservation 2G \\
+  --blkio-weight-device /dev/sda:123 --blkio-weight 123 \\
+  --device-read-bps /dev/sda:10mb --device-write-bps /dev/sda:10mb \\
+  --device-read-iops /dev/sda:1000 --device-write-iops /dev/sda:1000 \\
+  --pids-limit 123 ctrID
 ```
 
 ## SEE ALSO

pkg/specgen/utils_linux.go

@@ -9,94 +9,105 @@ import (
 	"golang.org/x/sys/unix"
 )
 
+// statBlkDev returns path's major and minor, or an error, if path is not a block device.
+func statBlkDev(path string) (int64, int64, error) {
+	var stat unix.Stat_t
+
+	if err := unix.Stat(path, &stat); err != nil {
+		return 0, 0, fmt.Errorf("could not parse device %s: %w", path, err)
+	}
+	if stat.Mode&unix.S_IFMT != unix.S_IFBLK {
+		return 0, 0, fmt.Errorf("%s: not a block device", path)
+	}
+	rdev := uint64(stat.Rdev) //nolint:unconvert // Some architectures have different type.
+	return int64(unix.Major(rdev)), int64(unix.Minor(rdev)), nil
+}
+
+// fillThrottleDev fills in dev.Major and dev.Minor fields based on path to a block device.
+func fillThrottleDev(path string, dev *spec.LinuxThrottleDevice) error {
+	major, minor, err := statBlkDev(path)
+	if err != nil {
+		return err
+	}
+
+	dev.Major, dev.Minor = major, minor
+
+	return nil
+}
+
 // FinishThrottleDevices takes the temporary representation of the throttle
-// devices in the specgen and looks up the major and major minors. it then
-// sets the throttle devices proper in the specgen
+// devices in the specgen, fills in major and minor numbers, and amends the
+// specgen with the specified devices. It returns an error if any device
+// specified doesn't exist or is not a block device.
 func FinishThrottleDevices(s *SpecGenerator) error {
+	if len(s.ThrottleReadBpsDevice)+len(s.ThrottleWriteBpsDevice)+len(s.ThrottleReadIOPSDevice)+len(s.ThrottleWriteIOPSDevice) == 0 {
+		return nil
+	}
+
 	if s.ResourceLimits == nil {
 		s.ResourceLimits = &spec.LinuxResources{}
 	}
-	if bps := s.ThrottleReadBpsDevice; len(bps) > 0 {
-		if s.ResourceLimits.BlockIO == nil {
-			s.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
-		}
-		for k, v := range bps {
-			statT := unix.Stat_t{}
-			if err := unix.Stat(k, &statT); err != nil {
-				return fmt.Errorf("could not parse throttle device at %s: %w", k, err)
-			}
-			v.Major = (int64(unix.Major(uint64(statT.Rdev)))) //nolint: unconvert
-			v.Minor = (int64(unix.Minor(uint64(statT.Rdev)))) //nolint: unconvert
-			if s.ResourceLimits.BlockIO == nil {
-				s.ResourceLimits.BlockIO = new(spec.LinuxBlockIO)
-			}
-			s.ResourceLimits.BlockIO.ThrottleReadBpsDevice = append(s.ResourceLimits.BlockIO.ThrottleReadBpsDevice, v)
-		}
+	if s.ResourceLimits.BlockIO == nil {
+		s.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
 	}
-	if bps := s.ThrottleWriteBpsDevice; len(bps) > 0 {
-		if s.ResourceLimits.BlockIO == nil {
-			s.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
-		}
-		for k, v := range bps {
-			statT := unix.Stat_t{}
-			if err := unix.Stat(k, &statT); err != nil {
-				return fmt.Errorf("could not parse throttle device at %s: %w", k, err)
-			}
-			v.Major = (int64(unix.Major(uint64(statT.Rdev)))) //nolint: unconvert
-			v.Minor = (int64(unix.Minor(uint64(statT.Rdev)))) //nolint: unconvert
-			s.ResourceLimits.BlockIO.ThrottleWriteBpsDevice = append(s.ResourceLimits.BlockIO.ThrottleWriteBpsDevice, v)
+
+	for k, v := range s.ThrottleReadBpsDevice {
+		if err := fillThrottleDev(k, &v); err != nil {
+			return err
 		}
+		s.ResourceLimits.BlockIO.ThrottleReadBpsDevice = append(s.ResourceLimits.BlockIO.ThrottleReadBpsDevice, v)
 	}
-	if iops := s.ThrottleReadIOPSDevice; len(iops) > 0 {
-		if s.ResourceLimits.BlockIO == nil {
-			s.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
-		}
-		for k, v := range iops {
-			statT := unix.Stat_t{}
-			if err := unix.Stat(k, &statT); err != nil {
-				return fmt.Errorf("could not parse throttle device at %s: %w", k, err)
-			}
-			v.Major = (int64(unix.Major(uint64(statT.Rdev)))) //nolint: unconvert
-			v.Minor = (int64(unix.Minor(uint64(statT.Rdev)))) //nolint: unconvert
-			s.ResourceLimits.BlockIO.ThrottleReadIOPSDevice = append(s.ResourceLimits.BlockIO.ThrottleReadIOPSDevice, v)
+
+	for k, v := range s.ThrottleWriteBpsDevice {
+		if err := fillThrottleDev(k, &v); err != nil {
+			return err
 		}
+		s.ResourceLimits.BlockIO.ThrottleWriteBpsDevice = append(s.ResourceLimits.BlockIO.ThrottleWriteBpsDevice, v)
 	}
-	if iops := s.ThrottleWriteIOPSDevice; len(iops) > 0 {
-		if s.ResourceLimits.BlockIO == nil {
-			s.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
+
+	for k, v := range s.ThrottleReadIOPSDevice {
+		if err := fillThrottleDev(k, &v); err != nil {
+			return err
 		}
-		for k, v := range iops {
-			statT := unix.Stat_t{}
-			if err := unix.Stat(k, &statT); err != nil {
-				return fmt.Errorf("could not parse throttle device at %s: %w", k, err)
-			}
-			v.Major = (int64(unix.Major(uint64(statT.Rdev)))) //nolint: unconvert
-			v.Minor = (int64(unix.Minor(uint64(statT.Rdev)))) //nolint: unconvert
-			s.ResourceLimits.BlockIO.ThrottleWriteIOPSDevice = append(s.ResourceLimits.BlockIO.ThrottleWriteIOPSDevice, v)
+		s.ResourceLimits.BlockIO.ThrottleReadIOPSDevice = append(s.ResourceLimits.BlockIO.ThrottleReadIOPSDevice, v)
+	}
+
+	for k, v := range s.ThrottleWriteIOPSDevice {
+		if err := fillThrottleDev(k, &v); err != nil {
+			return err
 		}
+		s.ResourceLimits.BlockIO.ThrottleWriteIOPSDevice = append(s.ResourceLimits.BlockIO.ThrottleWriteIOPSDevice, v)
 	}
+
 	return nil
 }
 
 func WeightDevices(specgen *SpecGenerator) error {
-	devs := []spec.LinuxWeightDevice{}
+	if len(specgen.WeightDevice) == 0 {
+		return nil
+	}
+
 	if specgen.ResourceLimits == nil {
 		specgen.ResourceLimits = &spec.LinuxResources{}
 	}
+	if specgen.ResourceLimits.BlockIO == nil {
+		specgen.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
+	}
+
 	for k, v := range specgen.WeightDevice {
-		statT := unix.Stat_t{}
-		if err := unix.Stat(k, &statT); err != nil {
-			return fmt.Errorf("failed to inspect '%s' in --blkio-weight-device: %w", k, err)
-		}
-		dev := new(spec.LinuxWeightDevice)
-		dev.Major = (int64(unix.Major(uint64(statT.Rdev)))) //nolint: unconvert
-		dev.Minor = (int64(unix.Minor(uint64(statT.Rdev)))) //nolint: unconvert
-		dev.Weight = v.Weight
-		devs = append(devs, *dev)
-		if specgen.ResourceLimits.BlockIO == nil {
-			specgen.ResourceLimits.BlockIO = &spec.LinuxBlockIO{}
+		major, minor, err := statBlkDev(k)
+		if err != nil {
+			return fmt.Errorf("bad --blkio-weight-device: %w", err)
 		}
-		specgen.ResourceLimits.BlockIO.WeightDevice = devs
+		specgen.ResourceLimits.BlockIO.WeightDevice = append(specgen.ResourceLimits.BlockIO.WeightDevice,
+			spec.LinuxWeightDevice{
+				LinuxBlockIODevice: spec.LinuxBlockIODevice{
+					Major: major,
+					Minor: minor,
+				},
+				Weight: v.Weight,
+			})
 	}
+
 	return nil
 }

test/system/280-update.bats

@@ -326,4 +326,27 @@ function nrand() {
 
     run_podman rm -t 0 -f $ctrname
 }
+
+# bats test_tags=ci:parallel
+@test "podman update - non-block device rejected by --*device* options" {
+    local dev=/dev/zero # Not a block device.
+    local block_opts=(
+        "--blkio-weight-device=$dev:123"
+        "--device-read-bps=$dev:10mb"
+        "--device-write-bps=$dev:10mb"
+        "--device-read-iops=$dev:1000"
+        "--device-write-iops=$dev:1000"
+    )
+    run_podman run -d "$IMAGE" /home/podman/pause
+    cid="$output"
+
+    defer-assertion-failures
+    for opt in "${block_opts[@]}"; do
+        run_podman 125 update "$cid" "$opt"
+        assert "$output" =~ "$dev: not a block device"
+    done
+    immediate-assertion-failures
+
+    run_podman rm -t 0 -f "$cid"
+}
 # vim: filetype=sh