Minor tweaks (#14683)

Address a few small things noticed while doing code reviews.

.github/actions/setup-microceph/action.yml

@@ -81,8 +81,7 @@ runs:
 
               # MicroCeph does not accept partitions directly.
               # See: https://github.com/canonical/microceph/issues/251
-              disk="$(losetup -f)"
-              sudo losetup --direct-io=on "${disk}" "${ephemeral_disk}${i}"
+              disk="$(sudo losetup --find --nooverlap --direct-io=on --show "${ephemeral_disk}${i}")"
               sudo microceph disk add "${disk}"
             done
           else

.github/workflows/codeql.yml

@@ -51,7 +51,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        language: ['go']
+        language: ['go', 'python']
         # CodeQL supports [ 'c-cpp', 'csharp', 'go', 'java-kotlin', 'javascript-typescript', 'python', 'ruby', 'swift' ]
         # Use only 'java-kotlin' to analyze code written in Java, Kotlin or both
         # Use only 'javascript-typescript' to analyze code written in JavaScript, TypeScript or both

Makefile

@@ -143,6 +143,9 @@ update-protobuf:
 
 .PHONY: update-schema
 update-schema:
+ifeq ($(shell command -v goimports),)
+	(cd / ; go install golang.org/x/tools/cmd/goimports@latest)
+endif
 	cd lxd/db/generate && go build -v -trimpath -o $(GOPATH)/bin/lxd-generate -tags "$(TAG_SQLITE3)" $(DEBUG) && cd -
 	go generate ./...
 	gofmt -s -w ./lxd/db/

lxd/db/cluster/config.mapper.go

@@ -89,8 +89,9 @@ func GetConfig(ctx context.Context, tx *sql.Tx, parent string, filters ...Config
 
 	configObjectsLocal := strings.Replace(configObjects, "%s_id", fmt.Sprintf("%s_id", parent), -1)
 	fillParent := make([]any, strings.Count(configObjectsLocal, "%s"))
+	mangledParent := strings.Replace(parent, "_", "s_", -1) + "s"
 	for i := range fillParent {
-		fillParent[i] = strings.Replace(parent, "_", "s_", -1) + "s"
+		fillParent[i] = mangledParent
 	}
 
 	queryStr := fmt.Sprintf(configObjectsLocal, fillParent...)

lxd/db/cluster/devices.mapper.go

@@ -89,8 +89,9 @@ func GetDevices(ctx context.Context, tx *sql.Tx, parent string, filters ...Devic
 
 	deviceObjectsLocal := strings.Replace(deviceObjects, "%s_id", fmt.Sprintf("%s_id", parent), -1)
 	fillParent := make([]any, strings.Count(deviceObjectsLocal, "%s"))
+	mangledParent := strings.Replace(parent, "_", "s_", -1) + "s"
 	for i := range fillParent {
-		fillParent[i] = strings.Replace(parent, "_", "s_", -1) + "s"
+		fillParent[i] = mangledParent
 	}
 
 	queryStr := fmt.Sprintf(deviceObjectsLocal, fillParent...)

lxd/db/generate/db/method.go

@@ -178,8 +178,9 @@ func (m *Method) getMany(buf *file.Buffer) error {
 		stmtLocal := stmtVar + "Local"
 		buf.L("%s := strings.Replace(%s, \"%%s_id\", fmt.Sprintf(\"%%s_id\", parent), -1)", stmtLocal, stmtVar)
 		buf.L("fillParent := make([]any, strings.Count(%s, \"%%s\"))", stmtLocal)
+		buf.L("mangledParent := strings.Replace(parent, \"_\", \"s_\", -1) + \"s\"")
 		buf.L("for i := range fillParent {")
-		buf.L("fillParent[i] = strings.Replace(parent, \"_\", \"s_\", -1) + \"s\"")
+		buf.L("fillParent[i] = mangledParent")
 		buf.L("}")
 		buf.N()
 		buf.L("queryStr := fmt.Sprintf(%s, fillParent...)", stmtLocal)

lxd/resources/cpu.go

@@ -51,17 +51,17 @@ func parseRangedListToInt64Slice(input string) ([]int64, error) {
 	for _, chunk := range chunks {
 		if strings.Contains(chunk, "-") {
 			// Range
-			fields := strings.SplitN(chunk, "-", 2)
-			if len(fields) != 2 {
+			before, after, _ := strings.Cut(chunk, "-")
+			if after == "" {
 				return nil, fmt.Errorf("Invalid CPU/NUMA set value: %q", input)
 			}
 
-			low, err := strconv.ParseInt(fields[0], 10, 64)
+			low, err := strconv.ParseInt(before, 10, 64)
 			if err != nil {
 				return nil, fmt.Errorf("Invalid CPU/NUMA set value: %w", err)
 			}
 
-			high, err := strconv.ParseInt(fields[1], 10, 64)
+			high, err := strconv.ParseInt(after, 10, 64)
 			if err != nil {
 				return nil, fmt.Errorf("Invalid CPU/NUMA set value: %w", err)
 			}
@@ -243,8 +243,12 @@ func GetCPU() (*api.ResourcesCPU, error) {
 		}
 
 		// Extract cpu index
-		fields := strings.SplitN(line, ":", 2)
-		value := strings.TrimSpace(fields[1])
+		_, value, found := strings.Cut(line, ":")
+		if !found {
+			return nil, fmt.Errorf("Failed to parse /proc/cpuinfo: Missing separator")
+		}
+
+		value = strings.TrimSpace(value)
 		cpuSocket, err := strconv.ParseInt(value, 10, 64)
 		if err != nil {
 			return nil, fmt.Errorf("Failed to parse cpu index %q in /proc/cpuinfo: %w", value, err)
@@ -272,9 +276,13 @@ func GetCPU() (*api.ResourcesCPU, error) {
 			}
 
 			// Get key/value
-			fields := strings.SplitN(line, ":", 2)
-			key := strings.TrimSpace(fields[0])
-			value := strings.TrimSpace(fields[1])
+			key, value, found := strings.Cut(line, ":")
+			if !found {
+				return nil, fmt.Errorf("Failed to parse /proc/cpuinfo: Missing separator")
+			}
+
+			key = strings.TrimSpace(key)
+			value = strings.TrimSpace(value)
 
 			if key == "vendor_id" {
 				cpuInfo.Vendor = value

test/lint/test-tests.sh

@@ -58,3 +58,7 @@ if grep -rlE '\!.* \|\| true$' test/; then
     echo "Some tests commands are ignoring expected failures (! cmd_should_fail || true)" >&2
     exit 1
 fi
+if grep -rlE '^\s*[^\!]+ \|\| false$' test/; then
+    echo "Some tests commands use unneeded construct to fail (cmd_should_succeed || false)" >&2
+    exit 1
+fi

test/suites/backup.sh

@@ -266,7 +266,7 @@ EOF
     echo "After:"
     echo "${poolConfigAfter}"
 
-    [ "${poolConfigBefore}" =  "${poolConfigAfter}" ] || false
+    [ "${poolConfigBefore}" = "${poolConfigAfter}" ]
     lxc storage show "${poolName}"
 
     lxc info c1 | grep snap0

test/suites/basic.sh

@@ -421,13 +421,13 @@ test_basic_usage() {
 
   # Test user, group and cwd
   lxc exec foo -- mkdir /blah
-  [ "$(lxc exec foo --user 1000 -- id -u)" = "1000" ] || false
-  [ "$(lxc exec foo --group 1000 -- id -g)" = "1000" ] || false
-  [ "$(lxc exec foo --cwd /blah -- pwd)" = "/blah" ] || false
+  [ "$(lxc exec foo --user 1000 -- id -u)" = "1000" ]
+  [ "$(lxc exec foo --group 1000 -- id -g)" = "1000" ]
+  [ "$(lxc exec foo --cwd /blah -- pwd)" = "/blah" ]
 
-  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- id -u)" = "1234" ] || false
-  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- id -g)" = "5678" ] || false
-  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- pwd)" = "/blah" ] || false
+  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- id -u)" = "1234" ]
+  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- id -g)" = "5678" ]
+  [ "$(lxc exec foo --user 1234 --group 5678 --cwd /blah -- pwd)" = "/blah" ]
 
   # check that we can set the environment
   lxc exec foo -- pwd | grep /root

test/suites/clustering.sh

@@ -492,14 +492,14 @@ test_clustering_containers() {
   LXD_DIR="${LXD_TWO_DIR}" lxc move bar egg --target node2
   LXD_DIR="${LXD_ONE_DIR}" lxc info egg | grep -q "Location: node2"
   apply_template2=$(LXD_DIR="${LXD_TWO_DIR}" lxc config get egg volatile.apply_template)
-  [ "${apply_template1}" =  "${apply_template2}" ] || false
+  [ "${apply_template1}" =  "${apply_template2}" ]
 
   # Move back to node3 the container on node1, keeping the same name.
   apply_template1=$(LXD_DIR="${LXD_TWO_DIR}" lxc config get egg volatile.apply_template)
   LXD_DIR="${LXD_TWO_DIR}" lxc move egg --target node3
   LXD_DIR="${LXD_ONE_DIR}" lxc info egg | grep -q "Location: node3"
   apply_template2=$(LXD_DIR="${LXD_TWO_DIR}" lxc config get egg volatile.apply_template)
-  [ "${apply_template1}" =  "${apply_template2}" ] || false
+  [ "${apply_template1}" =  "${apply_template2}" ]
 
   if command -v criu >/dev/null 2>&1; then
     # If CRIU supported, then try doing a live move using same name,
@@ -1580,11 +1580,11 @@ test_clustering_update_cert() {
   # Send update request
   LXD_DIR="${LXD_ONE_DIR}" lxc cluster update-cert "${cert_path}" "${key_path}" -q
 
-  cmp -s "${LXD_ONE_DIR}/cluster.crt" "${cert_path}" || false
-  cmp -s "${LXD_TWO_DIR}/cluster.crt" "${cert_path}" || false
+  cmp -s "${LXD_ONE_DIR}/cluster.crt" "${cert_path}"
+  cmp -s "${LXD_TWO_DIR}/cluster.crt" "${cert_path}"
 
-  cmp -s "${LXD_ONE_DIR}/cluster.key" "${key_path}" || false
-  cmp -s "${LXD_TWO_DIR}/cluster.key" "${key_path}" || false
+  cmp -s "${LXD_ONE_DIR}/cluster.key" "${key_path}"
+  cmp -s "${LXD_TWO_DIR}/cluster.key" "${key_path}"
 
   LXD_DIR="${LXD_ONE_DIR}" lxc info --target node2 | grep -q "server_name: node2"
   LXD_DIR="${LXD_TWO_DIR}" lxc info --target node1 | grep -q "server_name: node1"
@@ -1754,11 +1754,11 @@ test_clustering_update_cert_token() {
   # Change the cluster cert
   LXD_DIR="${LXD_ONE_DIR}" lxc cluster update-cert "${cert_path}" "${key_path}" -q
 
-  cmp -s "${LXD_ONE_DIR}/cluster.crt" "${cert_path}" || false
-  cmp -s "${LXD_TWO_DIR}/cluster.crt" "${cert_path}" || false
+  cmp -s "${LXD_ONE_DIR}/cluster.crt" "${cert_path}"
+  cmp -s "${LXD_TWO_DIR}/cluster.crt" "${cert_path}"
 
-  cmp -s "${LXD_ONE_DIR}/cluster.key" "${key_path}" || false
-  cmp -s "${LXD_TWO_DIR}/cluster.key" "${key_path}" || false
+  cmp -s "${LXD_ONE_DIR}/cluster.key" "${key_path}"
+  cmp -s "${LXD_TWO_DIR}/cluster.key" "${key_path}"
 
   # Verify the token with the wrong cert fingerprint is not usable due to the fingerprint mismatch
   url="https://10.1.1.101:8443"
@@ -2160,8 +2160,8 @@ test_clustering_image_replication() {
   # Image replication will be performed across all nodes in the cluster by default
   images_minimal_replica1=$(LXD_DIR="${LXD_ONE_DIR}" lxc config get cluster.images_minimal_replica)
   images_minimal_replica2=$(LXD_DIR="${LXD_TWO_DIR}" lxc config get cluster.images_minimal_replica)
-  [ "$images_minimal_replica1" = "" ] || false
-  [ "$images_minimal_replica2" = "" ] || false
+  [ "$images_minimal_replica1" = "" ]
+  [ "$images_minimal_replica2" = "" ]
 
   # Import the test image on node1
   LXD_DIR="${LXD_ONE_DIR}" ensure_import_testimage
@@ -2172,8 +2172,8 @@ test_clustering_image_replication() {
 
   # The image tarball is available on both nodes
   fingerprint=$(LXD_DIR="${LXD_ONE_DIR}" lxc image info testimage | awk '/^Fingerprint/ {print $2}')
-  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ] || false
-  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ] || false
+  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ]
+  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ]
 
   # Spawn a third node
   setup_clustering_netns 3
@@ -2214,15 +2214,15 @@ test_clustering_image_replication() {
 
   # The image tarball is available on all three nodes
   fingerprint=$(LXD_DIR="${LXD_ONE_DIR}" lxc image info testimage | awk '/^Fingerprint/ {print $2}')
-  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ] || false
-  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ] || false
-  [ -f "${LXD_THREE_DIR}/images/${fingerprint}" ] || false
+  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ]
+  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ]
+  [ -f "${LXD_THREE_DIR}/images/${fingerprint}" ]
 
   # Delete the imported image
   LXD_DIR="${LXD_ONE_DIR}" lxc image delete testimage
-  [ ! -f "${LXD_ONE_DIR}/images/${fingerprint}" ] || false
-  [ ! -f "${LXD_TWO_DIR}/images/${fingerprint}" ] || false
-  [ ! -f "${LXD_THREE_DIR}/images/${fingerprint}" ] || false
+  [ ! -f "${LXD_ONE_DIR}/images/${fingerprint}" ]
+  [ ! -f "${LXD_TWO_DIR}/images/${fingerprint}" ]
+  [ ! -f "${LXD_THREE_DIR}/images/${fingerprint}" ]
 
   # Import the image from the container
   LXD_DIR="${LXD_ONE_DIR}" ensure_import_testimage
@@ -2234,9 +2234,9 @@ test_clustering_image_replication() {
   lxc publish c1 --alias new-image
 
   fingerprint=$(LXD_DIR="${LXD_ONE_DIR}" lxc image info new-image | awk '/^Fingerprint/ {print $2}')
-  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ] || false
-  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ] || false
-  [ -f "${LXD_THREE_DIR}/images/${fingerprint}" ] || false
+  [ -f "${LXD_ONE_DIR}/images/${fingerprint}" ]
+  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ]
+  [ -f "${LXD_THREE_DIR}/images/${fingerprint}" ]
 
   # Delete the imported image
   LXD_DIR="${LXD_TWO_DIR}" lxc image delete new-image
@@ -2270,7 +2270,7 @@ test_clustering_image_replication() {
 
   # The image tarball is only available on node2
   fingerprint=$(LXD_DIR="${LXD_TWO_DIR}" lxc image info testimage | awk '/^Fingerprint/ {print $2}')
-  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ] || false
+  [ -f "${LXD_TWO_DIR}/images/${fingerprint}" ]
   [ ! -f "${LXD_ONE_DIR}/images/${fingerprint}" ] || false
   [ ! -f "${LXD_THREE_DIR}/images/${fingerprint}" ] || false
 

test/suites/container_devices_disk.sh

@@ -44,15 +44,15 @@ _container_devices_disk_shift() {
 
   lxc start foo
   lxc config device add foo idmapped_mount disk source="${TEST_DIR}/shift-source" path=/mnt
-  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "65534:65534" ] || false
+  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "65534:65534" ]
   lxc config device remove foo idmapped_mount
 
   lxc config device add foo idmapped_mount disk source="${TEST_DIR}/shift-source" path=/mnt shift=true
-  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
+  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ]
 
   lxc stop foo -f
   lxc start foo
-  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
+  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ]
   lxc config device remove foo idmapped_mount
   lxc stop foo -f
 
@@ -80,10 +80,10 @@ _container_devices_disk_shift() {
   lxc exec foo -- touch /mnt/a
   lxc exec foo -- chown 123:456 /mnt/a
 
-  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
-  [ "$(lxc exec foo-priv -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
-  [ "$(lxc exec foo-isol1 -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
-  [ "$(lxc exec foo-isol2 -- stat /mnt/a -c '%u:%g')" = "123:456" ] || false
+  [ "$(lxc exec foo -- stat /mnt/a -c '%u:%g')" = "123:456" ]
+  [ "$(lxc exec foo-priv -- stat /mnt/a -c '%u:%g')" = "123:456" ]
+  [ "$(lxc exec foo-isol1 -- stat /mnt/a -c '%u:%g')" = "123:456" ]
+  [ "$(lxc exec foo-isol2 -- stat /mnt/a -c '%u:%g')" = "123:456" ]
 
   lxc delete -f foo-priv foo-isol1 foo-isol2
   lxc config device remove foo shifted
@@ -99,19 +99,19 @@ _container_devices_raw_mount_options() {
   lxc launch testimage foo-priv -c security.privileged=true
 
   lxc config device add foo-priv loop_raw_mount_options disk source="${loop_device_1}" path=/mnt
-  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "0:0" ] || false
+  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "0:0" ]
   lxc exec foo-priv -- touch /mnt/foo
   lxc config device remove foo-priv loop_raw_mount_options
 
   lxc config device add foo-priv loop_raw_mount_options disk source="${loop_device_1}" path=/mnt raw.mount.options=uid=123,gid=456,ro
-  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "123:456" ] || false
+  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "123:456" ]
   ! lxc exec foo-priv -- touch /mnt/foo || false
   lxc config device remove foo-priv loop_raw_mount_options
 
   lxc stop foo-priv -f
   lxc config device add foo-priv loop_raw_mount_options disk source="${loop_device_1}" path=/mnt raw.mount.options=uid=123,gid=456,ro
   lxc start foo-priv
-  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "123:456" ] || false
+  [ "$(lxc exec foo-priv -- stat /mnt -c '%u:%g')" = "123:456" ]
   ! lxc exec foo-priv -- touch /mnt/foo || false
   lxc config device remove foo-priv loop_raw_mount_options
 
@@ -163,19 +163,19 @@ _container_devices_disk_cephfs() {
 _container_devices_disk_socket() {
   lxc start foo
   lxc config device add foo unix-socket disk source="${LXD_DIR}/unix.socket" path=/root/lxd.sock
-  [ "$(lxc exec foo -- stat /root/lxd.sock -c '%F')" = "socket" ] || false
+  [ "$(lxc exec foo -- stat /root/lxd.sock -c '%F')" = "socket" ]
   lxc restart -f foo
-  [ "$(lxc exec foo -- stat /root/lxd.sock -c '%F')" = "socket" ] || false
+  [ "$(lxc exec foo -- stat /root/lxd.sock -c '%F')" = "socket" ]
   lxc config device remove foo unix-socket
   lxc stop foo -f
 }
 
 _container_devices_disk_char() {
   lxc start foo
   lxc config device add foo char disk source=/dev/zero path=/root/zero
-  [ "$(lxc exec foo -- stat /root/zero -c '%F')" = "character special file" ] || false
+  [ "$(lxc exec foo -- stat /root/zero -c '%F')" = "character special file" ]
   lxc restart -f foo
-  [ "$(lxc exec foo -- stat /root/zero -c '%F')" = "character special file" ] || false
+  [ "$(lxc exec foo -- stat /root/zero -c '%F')" = "character special file" ]
   lxc config device remove foo char
   lxc stop foo -f
 }

test/suites/container_devices_disk_restricted.sh

@@ -63,7 +63,7 @@ test_container_devices_disk_restricted() {
   lxc stop -f c1
   lxc config device set c1 d1 source="${testRoot}/allowed1/foolink" path=/mnt/foolink
   lxc start c1
-  [ "$(lxc exec c1 --project restricted  -- stat /mnt/foolink -c '%u:%g')" = "65534:65534" ] || false
+  [ "$(lxc exec c1 --project restricted  -- stat /mnt/foolink -c '%u:%g')" = "65534:65534" ]
   lxc stop -f c1
 
   # Check usage of raw.idmap is restricted.
@@ -86,12 +86,12 @@ test_container_devices_disk_restricted() {
   # Check single entry raw.idmap has taken effect on disk share.
   lxc config device set c1 d1 source="${testRoot}/allowed1" path=/mnt
   lxc start c1 || (lxc info --show-log c1 ; false)
-  [ "$(lxc exec c1 --project restricted -- stat /mnt/foo1 -c '%u:%g')" = "1000:1000" ] || false
-  [ "$(lxc exec c1 --project restricted -- stat /mnt/foo2 -c '%u:%g')" = "65534:65534" ] || false
+  [ "$(lxc exec c1 --project restricted -- stat /mnt/foo1 -c '%u:%g')" = "1000:1000" ]
+  [ "$(lxc exec c1 --project restricted -- stat /mnt/foo2 -c '%u:%g')" = "65534:65534" ]
 
   # Check adding unix socket is allowed.
   lxc config device add c1 unix-socket disk source="${testRoot}/allowed1/lxd.sock" path=/root/lxd.sock
-  [ "$(lxc exec c1 --project restricted -- stat /root/lxd.sock -c '%F')" = "socket" ] || false
+  [ "$(lxc exec c1 --project restricted -- stat /root/lxd.sock -c '%F')" = "socket" ]
 
   lxc delete -f c1
   lxc project switch default