core/varlink: several cleanups for metrics varlink server

Follow-up for bb1ef2edf7d62de35291702635067ee85f09bad5

The commit introduced a new "metrics" varlink server, but for
user scope stuff it is not bound anywhere. The copy-pasted
"fresh" handling for deserialization is also essentially
meaningless as metrics_setup_varlink_server() doesn't even report
whether the varlink server is fresh (let alone that no serialization
is being done at all right now). Moreover, currently the event
priority is hardcoded, while event loop and associated priority
assignment ought to be subject to each daemon.

While fixing the mentioned issues I took the chance to restructure
the existing code a bit for readability. Note that serialization
for the metrics server is still missing - it will be tackled
in subsequent commits.

diff --git a/src/core/varlink.c b/src/core/varlink.c
index b5b00cdf74044549b809792a2b71619dcf58b5bb..76beeba434607aadaac2ec452fe2d044cd3b25dc 100644 (file)
--- a/src/core/varlink.c
+++ b/src/core/varlink.c
@@ -9,7 +9,6 @@
 #include "path-util.h"
 #include "pidref.h"
 #include "string-util.h"
-#include "strv.h"
 #include "unit.h"
 #include "varlink.h"
 #include "varlink-dynamic-user.h"
@@ -375,6 +374,8 @@ int manager_setup_varlink_server(Manager *m) {
         if (r < 0)
                 return log_debug_errno(r, "Failed to allocate Varlink server: %m");
 
+        (void) sd_varlink_server_set_description(s, "varlink-api");
+
         r = sd_varlink_server_add_interface_many(
                         s,
                         &vl_interface_io_systemd_Manager,
@@ -425,26 +426,64 @@ int manager_setup_varlink_server(Manager *m) {
         return 1;
 }
 
-static int manager_setup_varlink_metrics_server(Manager *m) {
-        sd_varlink_server_flags_t flags = SD_VARLINK_SERVER_INHERIT_USERDATA;
-        int r;
-
+int manager_setup_varlink_metrics_server(Manager *m) {
         assert(m);
 
+        sd_varlink_server_flags_t flags = SD_VARLINK_SERVER_INHERIT_USERDATA;
         if (MANAGER_IS_SYSTEM(m))
                 flags |= SD_VARLINK_SERVER_ACCOUNT_UID;
 
-        r = metrics_setup_varlink_server(
-                        &m->metrics_varlink_server, flags, m->event, vl_method_list, vl_method_describe, m);
-        if (r < 0)
-                return r;
+        return metrics_setup_varlink_server(&m->metrics_varlink_server, flags,
+                                            m->event, EVENT_PRIORITY_IPC,
+                                            vl_method_list_metrics, vl_method_describe_metrics,
+                                            m);
+}
 
-        return 0;
+static int varlink_server_listen_many_idempotent_sentinel(
+                sd_varlink_server *s,
+                bool known_fresh,
+                const char *prefix,
+                ...) {
+
+        va_list ap;
+        int r = 0;
+
+        assert(s);
+
+        va_start(ap, prefix);
+        for (const char *address; (address = va_arg(ap, const char*)); ) {
+                _cleanup_free_ char *p = NULL;
+
+                if (prefix) {
+                        p = path_join(prefix, address);
+                        if (!p) {
+                                r = log_oom();
+                                break;
+                        }
+
+                        address = p;
+                }
+
+                /* We might have got sockets through deserialization. Do not bind to them twice. */
+                if (!known_fresh && varlink_server_contains_socket(s, address))
+                        continue;
+
+                r = sd_varlink_server_listen_address(s, address, 0666 | SD_VARLINK_SERVER_MODE_MKDIR_0755);
+                if (r < 0) {
+                        log_error_errno(r, "Failed to bind to varlink socket '%s': %m", address);
+                        break;
+                }
+        }
+        va_end(ap);
+
+        return r;
 }
 
-static int manager_varlink_init_system(Manager *m) {
+#define varlink_server_listen_many_idempotent(s, known_fresh, prefix, ...) \
+        varlink_server_listen_many_idempotent_sentinel((s), (known_fresh), (prefix), __VA_ARGS__, NULL)
+
+static int manager_varlink_init_system_api(Manager *m) {
         int r;
-        _cleanup_free_ char *metrics_address = NULL;
 
         assert(m);
 
@@ -453,38 +492,21 @@ static int manager_varlink_init_system(Manager *m) {
                 return log_error_errno(r, "Failed to set up varlink server: %m");
         bool fresh = r > 0;
 
-        r = manager_setup_varlink_metrics_server(m);
-        if (r < 0)
-                return log_error_errno(r, "Failed to set up metrics varlink server: %m");
-        bool metrics_fresh = r > 0;
-
-        r = runtime_directory_generic(m->runtime_scope, "systemd/report/io.systemd.Manager", &metrics_address);
-        if (r < 0)
-                return r;
-
         if (!MANAGER_IS_TEST_RUN(m)) {
-                FOREACH_STRING(address,
-                               "/run/systemd/userdb/io.systemd.DynamicUser",
-                               VARLINK_PATH_MANAGED_OOM_SYSTEM,
-                               "/run/systemd/io.systemd.Manager",
-                               metrics_address) {
-
-                        sd_varlink_server *server = streq(address, metrics_address) ? m->metrics_varlink_server : m->varlink_server;
-                        fresh = streq(address, metrics_address) ? metrics_fresh : fresh;
-                        /* We might have got sockets through deserialization. Do not bind to them twice. */
-                        if (!fresh && varlink_server_contains_socket(server, address))
-                                continue;
-
-                        r = sd_varlink_server_listen_address(server, address, 0666 | SD_VARLINK_SERVER_MODE_MKDIR_0755);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to bind to varlink socket '%s': %m", address);
-                }
+                r = varlink_server_listen_many_idempotent(
+                                m->varlink_server, fresh,
+                                /* prefix = */ NULL,
+                                "/run/systemd/io.systemd.Manager",
+                                "/run/systemd/userdb/io.systemd.DynamicUser",
+                                VARLINK_PATH_MANAGED_OOM_SYSTEM);
+                if (r < 0)
+                        return r;
         }
 
-        return 1;
+        return 0;
 }
 
-static int manager_varlink_init_user(Manager *m) {
+static int manager_varlink_init_user_api(Manager *m) {
         int r;
 
         assert(m);
@@ -497,30 +519,46 @@ static int manager_varlink_init_user(Manager *m) {
                 return log_error_errno(r, "Failed to set up varlink server: %m");
         bool fresh = r > 0;
 
-        FOREACH_STRING(a,
-                       "systemd/io.systemd.Manager") {
-                _cleanup_free_ char *address = NULL;
-                address = path_join(m->prefix[EXEC_DIRECTORY_RUNTIME], a);
-                if (!address)
-                        return -ENOMEM;
-                /* We might have got sockets through deserialization. Do not bind to them twice. */
-                if (!fresh && varlink_server_contains_socket(m->varlink_server, address))
-                        continue;
+        r = varlink_server_listen_many_idempotent(
+                        m->varlink_server, fresh,
+                        m->prefix[EXEC_DIRECTORY_RUNTIME],
+                        "systemd/io.systemd.Manager");
+        if (r < 0)
+                return r;
 
-                r = sd_varlink_server_listen_address(m->varlink_server, address, 0666 | SD_VARLINK_SERVER_MODE_MKDIR_0755);
-                if (r < 0)
-                        return log_error_errno(r, "Failed to bind to varlink socket '%s': %m", address);
-        }
+        return manager_varlink_managed_oom_connect(m);
+}
+
+static int manager_varlink_init_metrics(Manager *m) {
+        int r;
+
+        assert(m);
+
+        if (MANAGER_IS_TEST_RUN(m))
+                return 0;
 
         r = manager_setup_varlink_metrics_server(m);
         if (r < 0)
                 return log_error_errno(r, "Failed to set up metrics varlink server: %m");
+        bool fresh = r > 0;
 
-        return manager_varlink_managed_oom_connect(m);
+        return varlink_server_listen_many_idempotent(
+                        m->metrics_varlink_server, fresh,
+                        m->prefix[EXEC_DIRECTORY_RUNTIME],
+                        "systemd/report/io.systemd.Manager");
 }
 
 int manager_varlink_init(Manager *m) {
-        return MANAGER_IS_SYSTEM(m) ? manager_varlink_init_system(m) : manager_varlink_init_user(m);
+        int r;
+
+        if (MANAGER_IS_SYSTEM(m))
+                r = manager_varlink_init_system_api(m);
+        else
+                r = manager_varlink_init_user_api(m);
+        if (r < 0)
+                return r;
+
+        return manager_varlink_init_metrics(m);
 }
 
 void manager_varlink_done(Manager *m) {
@@ -534,6 +572,7 @@ void manager_varlink_done(Manager *m) {
 
         m->varlink_server = sd_varlink_server_unref(m->varlink_server);
         m->managed_oom_varlink = sd_varlink_close_unref(m->managed_oom_varlink);
+
         m->metrics_varlink_server = sd_varlink_server_unref(m->metrics_varlink_server);
 }
 

diff --git a/src/core/varlink.h b/src/core/varlink.h
index f959cf8a670e109329484a427876580e8d9de3b9..119394386b1b2fc25ba937b85e09255b5a26477c 100644 (file)
--- a/src/core/varlink.h
+++ b/src/core/varlink.h
@@ -4,6 +4,7 @@
 #include "core-forward.h"
 
 int manager_setup_varlink_server(Manager *m);
+int manager_setup_varlink_metrics_server(Manager *m);
 
 int manager_varlink_init(Manager *m);
 void manager_varlink_done(Manager *m);

diff --git a/src/shared/metrics.c b/src/shared/metrics.c
index 5e0b0a115884b64b89aabbe4d5bb733eb831aef4..3b8965dbdc2d5b1c5d45ac895c0dee78baee4912 100644 (file)
--- a/src/shared/metrics.c
+++ b/src/shared/metrics.c
@@ -11,6 +11,7 @@ int metrics_setup_varlink_server(
                 sd_varlink_server **server, /* in and out param */
                 sd_varlink_server_flags_t flags,
                 sd_event *event,
+                int64_t priority,
                 sd_varlink_method_t vl_method_list_cb,
                 sd_varlink_method_t vl_method_describe_cb,
                 void *userdata) {
@@ -45,13 +46,12 @@ int metrics_setup_varlink_server(
         if (r < 0)
                 return log_debug_errno(r, "Failed to set varlink metrics server description: %m");
 
-        r = sd_varlink_server_attach_event(s, event, SD_EVENT_PRIORITY_NORMAL);
+        r = sd_varlink_server_attach_event(s, event, priority);
         if (r < 0)
                 return log_debug_errno(r, "Failed to attach varlink metrics server to event loop: %m");
 
         *server = TAKE_PTR(s);
-
-        return 0;
+        return 1;
 }
 
 static const char * const metric_family_type_table[_METRIC_FAMILY_TYPE_MAX] = {

diff --git a/src/shared/metrics.h b/src/shared/metrics.h
index cec0153e6806647a27e5ca14dcf3fee7a872cad4..ffc28cb0a9d8cafce623074863ef4075f63fd303 100644 (file)
--- a/src/shared/metrics.h
+++ b/src/shared/metrics.h
@@ -31,6 +31,7 @@ int metrics_setup_varlink_server(
                 sd_varlink_server **server, /* in and out param */
                 sd_varlink_server_flags_t flags,
                 sd_event *event,
+                int64_t priority,
                 sd_varlink_method_t vl_method_list_cb,
                 sd_varlink_method_t vl_method_describe_cb,
                 void *userdata);