Merge pull request #24333 from yuwata/sysctl

sysctl: improve performance for applying glob pattern

src/basic/path-util.c

@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 
 #include <errno.h>
+#include <fnmatch.h>
 #include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -17,6 +18,7 @@
 #include "extract-word.h"
 #include "fd-util.h"
 #include "fs-util.h"
+#include "glob-util.h"
 #include "log.h"
 #include "macro.h"
 #include "path-util.h"
@@ -1310,3 +1312,63 @@ bool prefixed_path_strv_contains(char **l, const char *path) {
 
         return false;
 }
+
+int path_glob_can_match(const char *pattern, const char *prefix, char **ret) {
+        assert(pattern);
+        assert(prefix);
+
+        for (const char *a = pattern, *b = prefix;;) {
+                _cleanup_free_ char *g = NULL, *h = NULL;
+                const char *p, *q;
+                int r, s;
+
+                r = path_find_first_component(&a, /* accept_dot_dot = */ false, &p);
+                if (r < 0)
+                        return r;
+
+                s = path_find_first_component(&b, /* accept_dot_dot = */ false, &q);
+                if (s < 0)
+                        return s;
+
+                if (s == 0) {
+                        /* The pattern matches the prefix. */
+                        if (ret) {
+                                char *t;
+
+                                t = path_join(prefix, p);
+                                if (!t)
+                                        return -ENOMEM;
+
+                                *ret = t;
+                        }
+                        return true;
+                }
+
+                if (r == 0)
+                        break;
+
+                if (r == s && strneq(p, q, r))
+                        continue; /* common component. Check next. */
+
+                g = strndup(p, r);
+                if (!g)
+                        return -ENOMEM;
+
+                if (!string_is_glob(g))
+                        break;
+
+                /* We found a glob component. Check if the glob pattern matches the prefix component. */
+
+                h = strndup(q, s);
+                if (!h)
+                        return -ENOMEM;
+
+                if (fnmatch(g, h, 0) != 0)
+                        break;
+        }
+
+        /* The pattern does not match the prefix. */
+        if (ret)
+                *ret = NULL;
+        return false;
+}

src/basic/path-util.h

@@ -196,3 +196,5 @@ static inline const char *empty_to_root(const char *path) {
 
 bool path_strv_contains(char **l, const char *path);
 bool prefixed_path_strv_contains(char **l, const char *path);
+
+int path_glob_can_match(const char *pattern, const char *prefix, char **ret);

src/sysctl/sysctl.c

@@ -56,18 +56,7 @@ static bool test_prefix(const char *p) {
         if (strv_isempty(arg_prefixes))
                 return true;
 
-        STRV_FOREACH(i, arg_prefixes) {
-                const char *t;
-
-                t = path_startswith(*i, "/proc/sys/");
-                if (!t)
-                        t = *i;
-
-                if (path_startswith(p, t))
-                        return true;
-        }
-
-        return false;
+        return path_startswith_strv(p, arg_prefixes);
 }
 
 static Option *option_new(
@@ -97,7 +86,7 @@ static Option *option_new(
         return TAKE_PTR(o);
 }
 
-static int sysctl_write_or_warn(const char *key, const char *value, bool ignore_failure) {
+static int sysctl_write_or_warn(const char *key, const char *value, bool ignore_failure, bool ignore_enoent) {
         int r;
 
         r = sysctl_write(key, value);
@@ -111,7 +100,7 @@ static int sysctl_write_or_warn(const char *key, const char *value, bool ignore_
                  * In all other cases log an error and make the tool fail. */
                 if (ignore_failure || (!arg_strict && (r == -EROFS || ERRNO_IS_PRIVILEGE(r))))
                         log_debug_errno(r, "Couldn't write '%s' to '%s', ignoring: %m", value, key);
-                else if (!arg_strict && r == -ENOENT)
+                else if (ignore_enoent && r == -ENOENT)
                         log_warning_errno(r, "Couldn't write '%s' to '%s', ignoring: %m", value, key);
                 else
                         return log_error_errno(r, "Couldn't write '%s' to '%s': %m", value, key);
@@ -120,6 +109,94 @@ static int sysctl_write_or_warn(const char *key, const char *value, bool ignore_
         return 0;
 }
 
+static int apply_glob_option_with_prefix(OrderedHashmap *sysctl_options, Option *option, const char *prefix) {
+        _cleanup_strv_free_ char **paths = NULL;
+        _cleanup_free_ char *pattern = NULL;
+        int r, k;
+
+        assert(sysctl_options);
+        assert(option);
+
+        if (prefix) {
+                _cleanup_free_ char *key = NULL;
+
+                r = path_glob_can_match(option->key, prefix, &key);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to check if the glob '%s' matches prefix '%s': %m",
+                                               option->key, prefix);
+                if (r == 0) {
+                        log_debug("The glob '%s' does not match prefix '%s'.", option->key, prefix);
+                        return 0;
+                }
+
+                log_debug("The glob '%s' is prefixed with '%s': '%s'", option->key, prefix, key);
+
+                if (!string_is_glob(key)) {
+                        /* The prefixed pattern is not glob anymore. Let's skip to call glob(). */
+                        if (ordered_hashmap_contains(sysctl_options, key)) {
+                                log_debug("Not setting %s (explicit setting exists).", key);
+                                return 0;
+                        }
+
+                        return sysctl_write_or_warn(key, option->value,
+                                                    /* ignore_failure = */ option->ignore_failure,
+                                                    /* ignore_enoent = */ true);
+                }
+
+                pattern = path_join("/proc/sys", key);
+        } else
+                pattern = path_join("/proc/sys", option->key);
+        if (!pattern)
+                return log_oom();
+
+        r = glob_extend(&paths, pattern, GLOB_NOCHECK);
+        if (r < 0) {
+                if (r == -ENOENT) {
+                        log_debug("No match for glob: %s", option->key);
+                        return 0;
+                }
+                if (option->ignore_failure || ERRNO_IS_PRIVILEGE(r)) {
+                        log_debug_errno(r, "Failed to resolve glob '%s', ignoring: %m", option->key);
+                        return 0;
+                } else
+                        return log_error_errno(r, "Couldn't resolve glob '%s': %m", option->key);
+        }
+
+        STRV_FOREACH(s, paths) {
+                const char *key;
+
+                assert_se(key = path_startswith(*s, "/proc/sys"));
+
+                if (ordered_hashmap_contains(sysctl_options, key)) {
+                        log_debug("Not setting %s (explicit setting exists).", key);
+                        continue;
+                }
+
+                k = sysctl_write_or_warn(key, option->value,
+                                         /* ignore_failure = */ option->ignore_failure,
+                                         /* ignore_enoent = */ !arg_strict);
+                if (k < 0 && r >= 0)
+                        r = k;
+        }
+
+        return r;
+}
+
+static int apply_glob_option(OrderedHashmap *sysctl_options, Option *option) {
+        int r = 0, k;
+
+        if (strv_isempty(arg_prefixes))
+                return apply_glob_option_with_prefix(sysctl_options, option, NULL);
+
+        STRV_FOREACH(i, arg_prefixes) {
+                k = apply_glob_option_with_prefix(sysctl_options, option, *i);
+                if (k < 0 && r >= 0)
+                        r = k;
+        }
+
+        return r;
+}
+
 static int apply_all(OrderedHashmap *sysctl_options) {
         Option *option;
         int r = 0;
@@ -131,52 +208,14 @@ static int apply_all(OrderedHashmap *sysctl_options) {
                 if (!option->value)
                         continue;
 
-                if (string_is_glob(option->key)) {
-                        _cleanup_strv_free_ char **paths = NULL;
-                        _cleanup_free_ char *pattern = NULL;
-
-                        pattern = path_join("/proc/sys", option->key);
-                        if (!pattern)
-                                return log_oom();
-
-                        k = glob_extend(&paths, pattern, GLOB_NOCHECK);
-                        if (k < 0) {
-                                if (option->ignore_failure || ERRNO_IS_PRIVILEGE(k))
-                                        log_debug_errno(k, "Failed to resolve glob '%s', ignoring: %m",
-                                                        option->key);
-                                else {
-                                        log_error_errno(k, "Couldn't resolve glob '%s': %m",
-                                                        option->key);
-                                        if (r == 0)
-                                                r = k;
-                                }
-
-                        } else if (strv_isempty(paths))
-                                log_debug("No match for glob: %s", option->key);
-
-                        STRV_FOREACH(s, paths) {
-                                const char *key;
-
-                                assert_se(key = path_startswith(*s, "/proc/sys"));
-
-                                if (!test_prefix(key))
-                                        continue;
-
-                                if (ordered_hashmap_contains(sysctl_options, key)) {
-                                        log_debug("Not setting %s (explicit setting exists).", key);
-                                        continue;
-                                }
-
-                                k = sysctl_write_or_warn(key, option->value, option->ignore_failure);
-                                if (r == 0)
-                                        r = k;
-                        }
-
-                } else {
-                        k = sysctl_write_or_warn(option->key, option->value, option->ignore_failure);
-                        if (r == 0)
-                                r = k;
-                }
+                if (string_is_glob(option->key))
+                        k = apply_glob_option(sysctl_options, option);
+                else
+                        k = sysctl_write_or_warn(option->key, option->value,
+                                                 /* ignore_failure = */ option->ignore_failure,
+                                                 /* ignore_enoent = */ !arg_strict);
+                if (k < 0 && r >= 0)
+                        r = k;
         }
 
         return r;
@@ -254,9 +293,6 @@ static int parse_file(OrderedHashmap **sysctl_options, const char *path, bool ig
                     !test_prefix(p))
                         continue;
 
-                if (ordered_hashmap_ensure_allocated(sysctl_options, &option_hash_ops) < 0)
-                        return log_oom();
-
                 existing = ordered_hashmap_get(*sysctl_options, p);
                 if (existing) {
                         if (streq_ptr(value, existing->value)) {
@@ -272,7 +308,7 @@ static int parse_file(OrderedHashmap **sysctl_options, const char *path, bool ig
                 if (!new_option)
                         return log_oom();
 
-                k = ordered_hashmap_put(*sysctl_options, new_option->key, new_option);
+                k = ordered_hashmap_ensure_put(sysctl_options, &option_hash_ops, new_option->key, new_option);
                 if (k < 0)
                         return log_error_errno(k, "Failed to add sysctl variable %s to hashmap: %m", p);
 
@@ -363,6 +399,7 @@ static int parse_argv(int argc, char *argv[]) {
                         break;
 
                 case ARG_PREFIX: {
+                        const char *s;
                         char *p;
 
                         /* We used to require people to specify absolute paths
@@ -371,10 +408,8 @@ static int parse_argv(int argc, char *argv[]) {
                          * sysctl name available. */
                         sysctl_normalize(optarg);
 
-                        if (path_startswith(optarg, "/proc/sys"))
-                                p = strdup(optarg);
-                        else
-                                p = path_join("/proc/sys", optarg);
+                        s = path_startswith(optarg, "/proc/sys");
+                        p = strdup(s ?: optarg);
                         if (!p)
                                 return log_oom();
 

src/test/test-path-util.c

@@ -1027,6 +1027,44 @@ TEST(path_startswith_strv) {
         assert_se(streq_ptr(path_startswith_strv("/foo2/bar", STRV_MAKE("/foo/quux", "", "/zzz")), NULL));
 }
 
+static void test_path_glob_can_match_one(const char *pattern, const char *prefix, const char *expected) {
+        _cleanup_free_ char *result = NULL;
+
+        log_debug("%s(%s, %s, %s)", __func__, pattern, prefix, strnull(expected));
+
+        assert_se(path_glob_can_match(pattern, prefix, &result) == !!expected);
+        assert_se(streq_ptr(result, expected));
+}
+
+TEST(path_glob_can_match) {
+        test_path_glob_can_match_one("/foo/hoge/aaa", "/foo/hoge/aaa/bbb", NULL);
+        test_path_glob_can_match_one("/foo/hoge/aaa", "/foo/hoge/aaa", "/foo/hoge/aaa");
+        test_path_glob_can_match_one("/foo/hoge/aaa", "/foo/hoge", "/foo/hoge/aaa");
+        test_path_glob_can_match_one("/foo/hoge/aaa", "/foo", "/foo/hoge/aaa");
+        test_path_glob_can_match_one("/foo/hoge/aaa", "/", "/foo/hoge/aaa");
+
+        test_path_glob_can_match_one("/foo/*/aaa", "/foo/hoge/aaa/bbb", NULL);
+        test_path_glob_can_match_one("/foo/*/aaa", "/foo/hoge/aaa", "/foo/hoge/aaa");
+        test_path_glob_can_match_one("/foo/*/aaa", "/foo/hoge", "/foo/hoge/aaa");
+        test_path_glob_can_match_one("/foo/*/aaa", "/foo", "/foo/*/aaa");
+        test_path_glob_can_match_one("/foo/*/aaa", "/", "/foo/*/aaa");
+
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/foo/xxx/yyy/aaa/bbb", NULL);
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/foo/xxx/yyy/aaa", "/foo/xxx/yyy/aaa");
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/foo/xxx/yyy", "/foo/xxx/yyy/aaa");
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/foo/xxx", "/foo/xxx/*/aaa");
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/foo", "/foo/*/*/aaa");
+        test_path_glob_can_match_one("/foo/*/*/aaa", "/", "/foo/*/*/aaa");
+
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo/xxx/aaa/bbb/ccc", NULL);
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo/xxx/aaa/bbb", "/foo/xxx/aaa/bbb");
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo/xxx/ccc", NULL);
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo/xxx/aaa", "/foo/xxx/aaa/*");
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo/xxx", "/foo/xxx/aaa/*");
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/foo", "/foo/*/aaa/*");
+        test_path_glob_can_match_one("/foo/*/aaa/*", "/", "/foo/*/aaa/*");
+}
+
 TEST(print_MAX) {
         log_info("PATH_MAX=%zu\n"
                  "FILENAME_MAX=%zu\n"

test/units/testsuite-76.sh

@@ -3,14 +3,37 @@
 set -eux
 set -o pipefail
 
+# shellcheck source=test/units/assert.sh
+. "$(dirname "$0")"/assert.sh
+
 export SYSTEMD_LOG_LEVEL=debug
 
-echo "foo.bar=42" > /etc/sysctl.d/foo.conf
-[[ $(/usr/lib/systemd/systemd-sysctl /etc/sysctl.d/foo.conf)$? -eq 0 ]]
-[[ $(/usr/lib/systemd/systemd-sysctl --strict /etc/sysctl.d/foo.conf)$? -ne 0 ]]
+echo "foo.bar=42" > /tmp/foo.conf
+assert_rc 0 /usr/lib/systemd/systemd-sysctl /tmp/foo.conf
+assert_rc 1 /usr/lib/systemd/systemd-sysctl --strict /tmp/foo.conf
 
-echo "-foo.foo=42" > /etc/sysctl.d/foo.conf
-[[ $(/usr/lib/systemd/systemd-sysctl /etc/sysctl.d/foo.conf)$? -eq 0 ]]
-[[ $(/usr/lib/systemd/systemd-sysctl --strict /etc/sysctl.d/foo.conf)$? -eq 0 ]]
+echo "-foo.foo=42" > /tmp/foo.conf
+assert_rc 0 /usr/lib/systemd/systemd-sysctl /tmp/foo.conf
+assert_rc 0 /usr/lib/systemd/systemd-sysctl --strict /tmp/foo.conf
+
+if ! systemd-detect-virt --quiet --container; then
+    ip link add hoge type dummy
+    udevadm wait /sys/class/net/hoge
+
+    cat >/tmp/foo.conf <<EOF
+net.ipv4.conf.*.drop_gratuitous_arp=1
+net.ipv4.*.*.bootp_relay=1
+net.ipv4.aaa.*.disable_policy=1
+EOF
+
+    echo 0 > /proc/sys/net/ipv4/conf/hoge/drop_gratuitous_arp
+    echo 0 > /proc/sys/net/ipv4/conf/hoge/bootp_relay
+    echo 0 > /proc/sys/net/ipv4/conf/hoge/disable_policy
+
+    assert_rc 0 /usr/lib/systemd/systemd-sysctl --prefix=/net/ipv4/conf/hoge /tmp/foo.conf
+    assert_eq "$(cat /proc/sys/net/ipv4/conf/hoge/drop_gratuitous_arp)" "1"
+    assert_eq "$(cat /proc/sys/net/ipv4/conf/hoge/bootp_relay)" "1"
+    assert_eq "$(cat /proc/sys/net/ipv4/conf/hoge/disable_policy)" "0"
+fi
 
 touch /testok