Merge pull request #24601 from yuwata/sd-device-enumerator-drop-recursion

sd-device-enumerator: do not recursively find child devices

2 files changed, 154 insertions, 106 deletions

diff --git a/src/libsystemd/sd-device/device-enumerator.c b/src/libsystemd/sd-device/device-enumerator.c
index ffed6fef9b..66845c43a1 100644
--- a/src/libsystemd/sd-device/device-enumerator.c
+++ b/src/libsystemd/sd-device/device-enumerator.c
@@ -16,8 +16,6 @@
 #include "string-util.h"
 #include "strv.h"
 
-#define DEVICE_ENUMERATE_MAX_DEPTH 256
-
 typedef enum DeviceEnumerationType {
         DEVICE_ENUMERATION_TYPE_DEVICES,
         DEVICE_ENUMERATION_TYPE_SUBSYSTEMS,
@@ -541,29 +539,70 @@ static int match_initialized(sd_device_enumerator *enumerator, sd_device *device
         return (enumerator->match_initialized == MATCH_INITIALIZED_NO) == (r == 0);
 }
 
+static bool match_subsystem(sd_device_enumerator *enumerator, const char *subsystem) {
+        assert(enumerator);
+
+        if (!subsystem)
+                return false;
+
+        return set_fnmatch(enumerator->match_subsystem, enumerator->nomatch_subsystem, subsystem);
+}
+
+typedef enum MatchFlag {
+        MATCH_SYSNAME     = 1u << 0,
+        MATCH_SUBSYSTEM   = 1u << 1,
+        MATCH_PARENT      = 1u << 2,
+        MATCH_TAG         = 1u << 3,
+
+        MATCH_ALL         = (1u << 4) - 1,
+} MatchFlag;
+
 static int test_matches(
                 sd_device_enumerator *enumerator,
                 sd_device *device,
-                bool ignore_parent_match) {
+                MatchFlag flags) {
 
         int r;
 
         assert(enumerator);
         assert(device);
 
-        /* Checks all matches, except for the sysname match (which the caller should check beforehand) */
+        if (FLAGS_SET(flags, MATCH_SYSNAME)) {
+                const char *sysname;
 
-        r = match_initialized(enumerator, device);
-        if (r <= 0)
-                return r;
+                r = sd_device_get_sysname(device, &sysname);
+                if (r < 0)
+                        return r;
+
+                if (!match_sysname(enumerator, sysname))
+                        return false;
+        }
+
+        if (FLAGS_SET(flags, MATCH_SUBSYSTEM)) {
+                const char *subsystem;
+
+                r = sd_device_get_subsystem(device, &subsystem);
+                if (r == -ENOENT)
+                        return false;
+                if (r < 0)
+                        return r;
+
+                if (!match_subsystem(enumerator, subsystem))
+                        return false;
+        }
 
-        if (!ignore_parent_match &&
+        if (FLAGS_SET(flags, MATCH_PARENT) &&
             !device_match_parent(device, enumerator->match_parent, NULL))
                 return false;
 
-        if (!match_tag(enumerator, device))
+        if (FLAGS_SET(flags, MATCH_TAG) &&
+            !match_tag(enumerator, device))
                 return false;
 
+        r = match_initialized(enumerator, device);
+        if (r <= 0)
+                return r;
+
         if (!match_property(enumerator, device))
                 return false;
 
@@ -573,19 +612,10 @@ static int test_matches(
         return true;
 }
 
-static bool match_subsystem(sd_device_enumerator *enumerator, const char *subsystem) {
-        assert(enumerator);
-
-        if (!subsystem)
-                return false;
-
-        return set_fnmatch(enumerator->match_subsystem, enumerator->nomatch_subsystem, subsystem);
-}
-
 static int enumerator_add_parent_devices(
                 sd_device_enumerator *enumerator,
                 sd_device *device,
-                bool ignore_parent_match) {
+                MatchFlag flags) {
 
         int k, r = 0;
 
@@ -593,8 +623,6 @@ static int enumerator_add_parent_devices(
         assert(device);
 
         for (;;) {
-                const char *ss, *usn;
-
                 k = sd_device_get_parent(device, &device);
                 if (k == -ENOENT) /* Reached the top? */
                         break;
@@ -603,27 +631,7 @@ static int enumerator_add_parent_devices(
                         break;
                 }
 
-                k = sd_device_get_subsystem(device, &ss);
-                if (k == -ENOENT) /* Has no subsystem? */
-                        continue;
-                if (k < 0) {
-                        r = k;
-                        break;
-                }
-
-                if (!match_subsystem(enumerator, ss))
-                        continue;
-
-                k = sd_device_get_sysname(device, &usn);
-                if (k < 0) {
-                        r = k;
-                        break;
-                }
-
-                if (!match_sysname(enumerator, usn))
-                        continue;
-
-                k = test_matches(enumerator, device, ignore_parent_match);
+                k = test_matches(enumerator, device, flags);
                 if (k < 0) {
                         r = k;
                         break;
@@ -644,7 +652,7 @@ static int enumerator_add_parent_devices(
 }
 
 int device_enumerator_add_parent_devices(sd_device_enumerator *enumerator, sd_device *device) {
-        return enumerator_add_parent_devices(enumerator, device, /* ignore_parent_match = */ true);
+        return enumerator_add_parent_devices(enumerator, device, MATCH_ALL & (~MATCH_PARENT));
 }
 
 static bool relevant_sysfs_subdir(const struct dirent *de) {
@@ -705,7 +713,7 @@ static int enumerator_scan_dir_and_add_devices(
                         continue;
                 }
 
-                k = test_matches(enumerator, device, /* ignore_parent_match = */ false);
+                k = test_matches(enumerator, device, MATCH_ALL & (~MATCH_SYSNAME)); /* sysname is already tested. */
                 if (k <= 0) {
                         if (k < 0)
                                 r = k;
@@ -719,7 +727,7 @@ static int enumerator_scan_dir_and_add_devices(
                 /* Also include all potentially matching parent devices in the enumeration. These are things
                  * like root busses — e.g. /sys/devices/pci0000:00/ or /sys/devices/pnp0/, which ar not
                  * linked from /sys/class/ or /sys/bus/, hence pick them up explicitly here. */
-                k = enumerator_add_parent_devices(enumerator, device, /* ignore_parent_match = */ false);
+                k = enumerator_add_parent_devices(enumerator, device, MATCH_ALL);
                 if (k < 0)
                         r = k;
         }
@@ -781,7 +789,6 @@ static int enumerator_scan_devices_tag(sd_device_enumerator *enumerator, const c
 
         FOREACH_DIRENT_ALL(de, dir, return -errno) {
                 _cleanup_(sd_device_unrefp) sd_device *device = NULL;
-                const char *subsystem, *sysname;
                 int k;
 
                 if (de->d_name[0] == '.')
@@ -796,33 +803,11 @@ static int enumerator_scan_devices_tag(sd_device_enumerator *enumerator, const c
                         continue;
                 }
 
-                k = sd_device_get_subsystem(device, &subsystem);
-                if (k < 0) {
-                        if (k != -ENOENT)
-                                /* this is necessarily racy, so ignore missing devices */
-                                r = k;
-                        continue;
-                }
-
-                if (!match_subsystem(enumerator, subsystem))
-                        continue;
-
-                k = sd_device_get_sysname(device, &sysname);
-                if (k < 0) {
+                /* Generated from tag, hence not necessary to check tag again. */
+                k = test_matches(enumerator, device, MATCH_ALL & (~MATCH_TAG));
+                if (k < 0)
                         r = k;
-                        continue;
-                }
-
-                if (!match_sysname(enumerator, sysname))
-                        continue;
-
-                if (!device_match_parent(device, enumerator->match_parent, NULL))
-                        continue;
-
-                if (!match_property(enumerator, device))
-                        continue;
-
-                if (!device_match_sysattr(device, enumerator->match_sysattr, enumerator->nomatch_sysattr))
+                if (k <= 0)
                         continue;
 
                 k = device_enumerator_add_device(enumerator, device);
@@ -854,7 +839,6 @@ static int enumerator_scan_devices_tags(sd_device_enumerator *enumerator) {
 
 static int parent_add_child(sd_device_enumerator *enumerator, const char *path) {
         _cleanup_(sd_device_unrefp) sd_device *device = NULL;
-        const char *subsystem, *sysname;
         int r;
 
         r = sd_device_new_from_syspath(&device, path);
@@ -864,42 +848,27 @@ static int parent_add_child(sd_device_enumerator *enumerator, const char *path)
         else if (r < 0)
                 return r;
 
-        r = sd_device_get_subsystem(device, &subsystem);
-        if (r == -ENOENT)
-                return 0;
-        if (r < 0)
-                return r;
-
-        if (!match_subsystem(enumerator, subsystem))
-                return 0;
-
-        r = sd_device_get_sysname(device, &sysname);
-        if (r < 0)
-                return r;
-
-        if (!match_sysname(enumerator, sysname))
-                return 0;
-
-        if (!match_property(enumerator, device))
-                return 0;
-
-        if (!device_match_sysattr(device, enumerator->match_sysattr, enumerator->nomatch_sysattr))
-                return 0;
-
-        r = device_enumerator_add_device(enumerator, device);
-        if (r < 0)
+        r = test_matches(enumerator, device, MATCH_ALL & (~MATCH_PARENT));
+        if (r <= 0)
                 return r;
 
-        return 1;
+        return device_enumerator_add_device(enumerator, device);
 }
 
-static int parent_crawl_children(sd_device_enumerator *enumerator, const char *path, unsigned maxdepth) {
+static int parent_crawl_children(sd_device_enumerator *enumerator, const char *path, Set **stack) {
         _cleanup_closedir_ DIR *dir = NULL;
         int r = 0;
 
+        assert(enumerator);
+        assert(path);
+        assert(stack);
+
         dir = opendir(path);
-        if (!dir)
-                return log_debug_errno(errno, "sd-device-enumerator: Failed to open parent directory %s: %m", path);
+        if (!dir) {
+                if (errno == ENOENT)
+                        return 0;
+                return log_debug_errno(errno, "sd-device-enumerator: Failed to open %s: %m", path);
+        }
 
         FOREACH_DIRENT_ALL(de, dir, return -errno) {
                 _cleanup_free_ char *child = NULL;
@@ -919,25 +888,33 @@ static int parent_crawl_children(sd_device_enumerator *enumerator, const char *p
                 if (k < 0)
                         r = k;
 
-                if (maxdepth > 0)
-                        parent_crawl_children(enumerator, child, maxdepth - 1);
-                else
-                        log_debug("sd-device-enumerator: Max depth reached, %s: ignoring devices", child);
+                k = set_ensure_consume(stack, &path_hash_ops_free, TAKE_PTR(child));
+                if (k < 0)
+                        r = k;
         }
 
         return r;
 }
 
 static int enumerator_scan_devices_children(sd_device_enumerator *enumerator) {
+        _cleanup_set_free_ Set *stack = NULL;
         const char *path;
         int r = 0, k;
 
+        assert(enumerator);
+
         SET_FOREACH(path, enumerator->match_parent) {
                 k = parent_add_child(enumerator, path);
                 if (k < 0)
                         r = k;
 
-                k = parent_crawl_children(enumerator, path, DEVICE_ENUMERATE_MAX_DEPTH);
+                k = parent_crawl_children(enumerator, path, &stack);
+                if (k < 0)
+                        r = k;
+        }
+
+        for (char *p; (p = set_steal_first(stack)); free(p)) {
+                k = parent_crawl_children(enumerator, p, &stack);
                 if (k < 0)
                         r = k;
         }
diff --git a/src/libsystemd/sd-device/test-sd-device.c b/src/libsystemd/sd-device/test-sd-device.c
index 0016aa08e7..c5fed8c053 100644
--- a/src/libsystemd/sd-device/test-sd-device.c
+++ b/src/libsystemd/sd-device/test-sd-device.c
@@ -401,6 +401,77 @@ TEST(sd_device_enumerator_add_match_property) {
         assert_se(ifindex == 1);
 }
 
+static void check_parent_match(sd_device_enumerator *e, sd_device *dev) {
+        const char *syspath;
+        bool found = false;
+        sd_device *d;
+
+        assert_se(sd_device_get_syspath(dev, &syspath) >= 0);
+
+        FOREACH_DEVICE(e, d) {
+                const char *s;
+
+                assert_se(sd_device_get_syspath(d, &s) >= 0);
+                if (streq(s, syspath)) {
+                        found = true;
+                        break;
+                }
+        }
+
+        if (!found) {
+                log_device_debug(dev, "not enumerated, already removed??");
+                /* If the original device not found, then the device should be already removed. */
+                assert_se(access(syspath, F_OK) < 0);
+                assert_se(errno == ENOENT);
+        }
+}
+
+TEST(sd_device_enumerator_add_match_parent) {
+        _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL;
+        sd_device *dev;
+        int r;
+
+        assert_se(sd_device_enumerator_new(&e) >= 0);
+        assert_se(sd_device_enumerator_allow_uninitialized(e) >= 0);
+        /* See comments in TEST(sd_device_enumerator_devices). */
+        assert_se(sd_device_enumerator_add_match_subsystem(e, "bdi", false) >= 0);
+        assert_se(sd_device_enumerator_add_nomatch_sysname(e, "loop*") >= 0);
+        assert_se(sd_device_enumerator_add_match_subsystem(e, "net", false) >= 0);
+
+        if (!slow_tests_enabled())
+                assert_se(sd_device_enumerator_add_match_subsystem(e, "block", true) >= 0);
+
+        FOREACH_DEVICE(e, dev) {
+                _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *p = NULL;
+                const char *syspath;
+                sd_device *parent;
+
+                assert_se(sd_device_get_syspath(dev, &syspath) >= 0);
+
+                r = sd_device_get_parent(dev, &parent);
+                if (r < 0) {
+                        assert_se(ERRNO_IS_DEVICE_ABSENT(r));
+                        continue;
+                }
+
+                log_debug("> %s", syspath);
+
+                assert_se(sd_device_enumerator_new(&p) >= 0);
+                assert_se(sd_device_enumerator_allow_uninitialized(p) >= 0);
+                assert_se(sd_device_enumerator_add_match_parent(p, parent) >= 0);
+
+                check_parent_match(p, dev);
+
+                /* If the device does not have subsystem, then it is not enumerated. */
+                r = sd_device_get_subsystem(parent, NULL);
+                if (r < 0) {
+                        assert_se(r == -ENOENT);
+                        continue;
+                }
+                check_parent_match(p, parent);
+        }
+}
+
 TEST(sd_device_new_from_nulstr) {
         const char *devlinks =
                 "/dev/disk/by-partuuid/1290d63a-42cc-4c71-b87c-xxxxxxxxxxxx\0"