From b97b424aa4cfd0afaaea4a1075163ca77e360667 Mon Sep 17 00:00:00 2001 From: Ian Wienand Date: Thu, 12 Feb 2026 21:23:31 +1100 Subject: [PATCH] block_device: remove cmp_mount_order This comparator is buggy, and orders differently on python 3.13 $ python3.14 ./test.py [('/', 'fake_name'), ('/var/log', 'fake_log'), ('/boot', 'fake_boot'), ('/var', 'fake_name')] $ python3.10 ./test.py [('/', 'fake_name'), ('/var', 'fake_name'), ('/var/log', 'fake_log'), ('/boot', 'fake_boot')] It is saying that if it's not equal, or doesn't start with the same prefix, it is bigger. Hence "/boot > /var" but also "/var > /boot" which confuses things. I really think what we want here is to ensure that parents mount before children. So string comparision is fine? "/var < /var/log < /var/log/foo"? Replace it with that Change-Id: Ieac277c90b13bdb8b2c651b17fa4617ac4c1b845 Signed-off-by: Ian Wienand --- .../block_device/level3/mount.py | 30 +++---------------- .../block_device/tests/test_mount_order.py | 27 ----------------- 2 files changed, 4 insertions(+), 53 deletions(-) diff --git a/diskimage_builder/block_device/level3/mount.py b/diskimage_builder/block_device/level3/mount.py index b21f8b6c2..ab02e9332 100644 --- a/diskimage_builder/block_device/level3/mount.py +++ b/diskimage_builder/block_device/level3/mount.py @@ -12,7 +12,6 @@ # License for the specific language governing permissions and limitations # under the License. -import functools import logging import os import time @@ -137,30 +136,6 @@ class MountPointNode(NodeBase): self.umount() -def cmp_mount_order(this, other): - """Sort comparision function for mount-point sorting - - See if ``this`` comes before ``other`` in mount-order list. In - words: if the other mount-point has us as it's parent, we come - before it (are less than it). e.g. ``/var < /var/log < - /var/log/foo`` - - :param this: tuple of mount_point, node name - :param other: tuple of mount_point, node name - :returns int: cmp value - - """ - # sort is only based on the mount_point. - this, _ = this - other, _ = other - if this == other: - return 0 - if other.startswith(this): - return -1 - else: - return 1 - - class Mount(PluginBase): def __init__(self, config, defaults, state): super(Mount, self).__init__() @@ -185,7 +160,10 @@ class Mount(PluginBase): "Mount point [%s] specified more than once" % self.node.mount_point) sorted_mount_points.append((self.node.mount_point, self.node.name)) - sorted_mount_points.sort(key=functools.cmp_to_key(cmp_mount_order)) + # Sort by mount point so hierarchy is in shortest to longest, such + # that parents mount before children; e.g. + # /var < /var/log + sorted_mount_points.sort(key=lambda x: x[0]) # Save the state if it's new (otherwise this is idempotent update) state['sorted_mount_points'] = sorted_mount_points logger.debug("Ordered mounts now: %s", sorted_mount_points) diff --git a/diskimage_builder/block_device/tests/test_mount_order.py b/diskimage_builder/block_device/tests/test_mount_order.py index 28362bb6e..65aff614c 100644 --- a/diskimage_builder/block_device/tests/test_mount_order.py +++ b/diskimage_builder/block_device/tests/test_mount_order.py @@ -10,7 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import functools import logging import os from unittest import mock @@ -20,37 +19,11 @@ import diskimage_builder.block_device.tests.test_config as tc from diskimage_builder.block_device.config import config_tree_to_graph from diskimage_builder.block_device.config import create_graph from diskimage_builder.block_device.level2.mkfs import FilesystemNode -from diskimage_builder.block_device.level3.mount import cmp_mount_order from diskimage_builder.block_device.level3.mount import MountPointNode -from diskimage_builder.block_device.tests.test_base import TestBase logger = logging.getLogger(__name__) -class TestMountComparator(TestBase): - - def test_mount_comparator(self): - # This tests cmp_mount_order to ensure it sorts in the - # expected order. The comparator takes a tuple of - # (mount_point, node_name) but we can ignore the name - partitions = [ - ('/var/log', 'fake_log'), - ('/boot', 'fake_boot'), - ('/', 'fake_name'), - ('/var', 'fake_name')] - partitions.sort(key=functools.cmp_to_key(cmp_mount_order)) - - res = list(x[0] for x in partitions) - - # "/" must be first - self.assertEqual(res[0], '/') - - # /var before /var/log - var_pos = res.index('/var') - var_log_pos = res.index('/var/log') - self.assertGreater(var_log_pos, var_pos) - - class TestMountOrder(tc.TestGraphGeneration): def _exec_sudo_log(*args, **kwargs):