Merge pull request #17344 from keszybz/bus-connect-more-logs

Add some debug logs to help diagnose bus connections

docs/BOOT_LOADER_SPECIFICATION.md

@@ -114,7 +114,23 @@ We define two directories below `$BOOT`:
 
 **Note:** _In all cases the `/loader/` directory should be located directly in the root of the file system. Specifically, if `$BOOT` is the ESP, then `/loader/` directory should be located directly in the root directory of the ESP, and not in the `/EFI/` subdirectory._
 
-Inside the `$BOOT/loader/entries/` directory each OS vendor may drop one or more configuration snippets with the suffix ".conf", one for each boot menu item. The file name of the file is used for identification of the boot item but shall never be presented to the user in the UI. The file name may be chosen freely but should be unique enough to avoid clashes between OS installations. More specifically it is suggested to include the machine ID (`/etc/machine-id` or the D-Bus machine ID for OSes that lack `/etc/machine-id`), the kernel version (as returned by `uname -r`) and an OS identifier (The ID field of `/etc/os-release`). Example: `$BOOT/loader/entries/6a9857a393724b7a981ebb5b8495b9ea-3.8.0-2.fc19.x86_64.conf`.
+Inside the `$BOOT/loader/entries/` directory each OS vendor may drop one or
+more configuration snippets with the suffix ".conf", one for each boot menu
+item. The file name of the file is used for identification of the boot item but
+shall never be presented to the user in the UI. The file name may be chosen
+freely but should be unique enough to avoid clashes between OS
+installations. More specifically it is suggested to include the machine ID
+(`/etc/machine-id` or the D-Bus machine ID for OSes that lack
+`/etc/machine-id`), the kernel version (as returned by `uname -r`) and an OS
+identifier (The ID field of `/etc/os-release`). Example:
+`$BOOT/loader/entries/6a9857a393724b7a981ebb5b8495b9ea-3.8.0-2.fc19.x86_64.conf`.
+
+In order to maximize compatibility with file system implementations and
+restricted boot loader environments, and to minimize conflicting character use
+with other progams, file names shall be chosen from a restricted character set:
+ASCII upper and lower case characters, digits, "+", "-", "_" and ".". Also, the
+file names should have a length of at least one and at most 255 characters
+(including file name suffix).
 
 These configuration snippets shall be Unix-style text files (i.e. line separation with a single newline character), in the UTF-8 encoding. The configuration snippets are loosely inspired on Grub1's configuration syntax. Lines beginning with '#' shall be ignored and used for commenting. The first word of a line is used as key and shall be separated by one or more spaces from its value. The following keys are known:
 
@@ -171,6 +187,10 @@ extension `.efi`. Support for images of this type is of course specific to
 systems with EFI firmware. Ignore this section if you work on systems not
 supporting EFI.
 
+Type #2 file names should be chosen from the same restricted character set as
+Type #1 described above (but use a different file name suffix of `.efi` instead
+of `.conf`).
+
 Images of this type have the advantage that all metadata and payload that makes
 up the boot entry is monopolized in a single PE file that can be signed
 cryptographically as one for the purpose of EFI SecureBoot.

man/sd_bus_default.xml

@@ -276,13 +276,20 @@
       <para>Returned errors may indicate the following problems:</para>
 
       <variablelist>
-
         <varlistentry>
           <term><constant>-EINVAL</constant></term>
 
           <listitem><para>The specified parameters are invalid.</para></listitem>
         </varlistentry>
 
+        <varlistentry>
+          <term><constant>-ENOMEDIUM</constant></term>
+
+          <listitem><para>The requested bus type is not available because of invalid environment (for example
+          the user session bus is not available because <varname>$XDG_RUNTIME_DIR</varname> is not set).
+          </para></listitem>
+        </varlistentry>
+
         <varlistentry>
           <term><constant>-ENOMEM</constant></term>
 

src/busctl/busctl.c

@@ -129,11 +129,11 @@ static int acquire_bus(bool set_monitor, sd_bus **ret) {
                 }
         }
         if (r < 0)
-                return log_error_errno(r, "Failed to set address: %m");
+                return bus_log_address_error(r);
 
         r = sd_bus_start(bus);
         if (r < 0)
-                return log_error_errno(r, "Failed to connect to bus: %m");
+                return bus_log_connect_error(r);
 
         *ret = TAKE_PTR(bus);
 

src/core/core-varlink.c

@@ -52,10 +52,9 @@ static bool user_match_lookup_parameters(LookupParameters *p, const char *name,
 }
 
 static int build_managed_oom_json_array_element(Unit *u, const char *property, JsonVariant **ret_v) {
-        _cleanup_(json_variant_unrefp) JsonVariant *v = NULL;
+        bool use_limit = false;
         CGroupContext *c;
         const char *mode;
-        int r;
 
         assert(u);
         assert(property);
@@ -74,19 +73,17 @@ static int build_managed_oom_json_array_element(Unit *u, const char *property, J
                 mode = managed_oom_mode_to_string(MANAGED_OOM_AUTO);
         else if (streq(property, "ManagedOOMSwap"))
                 mode = managed_oom_mode_to_string(c->moom_swap);
-        else if (streq(property, "ManagedOOMMemoryPressure"))
+        else if (streq(property, "ManagedOOMMemoryPressure")) {
                 mode = managed_oom_mode_to_string(c->moom_mem_pressure);
-        else
+                use_limit = true;
+        } else
                 return -EINVAL;
 
-        r = json_build(&v, JSON_BUILD_OBJECT(
+        return json_build(ret_v, JSON_BUILD_OBJECT(
                                  JSON_BUILD_PAIR("mode", JSON_BUILD_STRING(mode)),
                                  JSON_BUILD_PAIR("path", JSON_BUILD_STRING(u->cgroup_path)),
                                  JSON_BUILD_PAIR("property", JSON_BUILD_STRING(property)),
-                                JSON_BUILD_PAIR("limit", JSON_BUILD_UNSIGNED(c->moom_mem_pressure_limit))));
+                                 JSON_BUILD_PAIR_CONDITION(use_limit, "limit", JSON_BUILD_UNSIGNED(c->moom_mem_pressure_limit))));
-
-        *ret_v = TAKE_PTR(v);
-        return r;
 }
 
 int manager_varlink_send_managed_oom_update(Unit *u) {
@@ -478,10 +475,8 @@ void manager_varlink_done(Manager *m) {
         assert(m);
 
         /* Send the final message if we still have a subscribe request open. */
-        if (m->managed_oom_varlink_request) {
+        if (m->managed_oom_varlink_request)
-                (void) varlink_error(m->managed_oom_varlink_request, VARLINK_ERROR_DISCONNECTED, NULL);
+                m->managed_oom_varlink_request = varlink_close_unref(m->managed_oom_varlink_request);
-                m->managed_oom_varlink_request = varlink_unref(m->managed_oom_varlink_request);
-        }
 
         m->varlink_server = varlink_server_unref(m->varlink_server);
 }

src/core/load-fragment-gperf.gperf.m4

@@ -226,7 +226,7 @@ $1.IPIngressFilterPath,          config_parse_ip_filter_bpf_progs,   0,
 $1.IPEgressFilterPath,                   config_parse_ip_filter_bpf_progs,            0,                                  offsetof($1, cgroup_context.ip_filters_egress)
 $1.ManagedOOMSwap,                       config_parse_managed_oom_mode,               0,                                  offsetof($1, cgroup_context.moom_swap)
 $1.ManagedOOMMemoryPressure,             config_parse_managed_oom_mode,               0,                                  offsetof($1, cgroup_context.moom_mem_pressure)
-$1.ManagedOOMMemoryPressureLimitPercent,config_parse_managed_oom_mem_pressure_limit,0,              offsetof($1, cgroup_context)
+$1.ManagedOOMMemoryPressureLimitPercent, config_parse_managed_oom_mem_pressure_limit, 0,                                  offsetof($1, cgroup_context.moom_mem_pressure_limit)
 $1.NetClass,                             config_parse_warn_compat,                    DISABLED_LEGACY,                    0'
 )m4_dnl
 Unit.Description,                        config_parse_unit_string_printf,             0,                                  offsetof(Unit, description)

src/core/load-fragment.c

@@ -3824,7 +3824,6 @@ int config_parse_managed_oom_mode(
                 const char *rvalue,
                 void *data,
                 void *userdata) {
-        Unit *u = userdata;
         ManagedOOMMode *mode = data, m;
         UnitType t;
 
@@ -3836,7 +3835,7 @@ int config_parse_managed_oom_mode(
 
         if (isempty(rvalue)) {
                 *mode = MANAGED_OOM_AUTO;
-                goto finish;
+                return 0;
         }
 
         m = managed_oom_mode_from_string(rvalue);
@@ -3845,9 +3844,6 @@ int config_parse_managed_oom_mode(
                 return 0;
         }
         *mode = m;
-
-finish:
-        (void) manager_varlink_send_managed_oom_update(u);
         return 0;
 }
 
@@ -3862,8 +3858,7 @@ int config_parse_managed_oom_mem_pressure_limit(
                 const char *rvalue,
                 void *data,
                 void *userdata) {
-        Unit *u = userdata;
+        int *limit = data;
-        CGroupContext *c = data;
         UnitType t;
         int r;
 
@@ -3874,8 +3869,8 @@ int config_parse_managed_oom_mem_pressure_limit(
                 return log_syntax(unit, LOG_WARNING, filename, line, 0, "%s= is not supported for this unit type, ignoring.", lvalue);
 
         if (isempty(rvalue)) {
-                c->moom_mem_pressure_limit = 0;
+                *limit = 0;
-                goto finish;
+                return 0;
         }
 
         r = parse_percent(rvalue);
@@ -3884,12 +3879,7 @@ int config_parse_managed_oom_mem_pressure_limit(
                 return 0;
         }
 
-        c->moom_mem_pressure_limit = r;
+        *limit = r;
-
-finish:
-        /* Only update the limit if memory pressure detection is enabled because the information is irrelevant otherwise */
-        if (c->moom_mem_pressure == MANAGED_OOM_KILL)
-                (void) manager_varlink_send_managed_oom_update(u);
         return 0;
 }
 

src/core/unit.c

@@ -1684,6 +1684,7 @@ int unit_load(Unit *u) {
 
         unit_add_to_dbus_queue(unit_follow_merge(u));
         unit_add_to_gc_queue(u);
+        (void) manager_varlink_send_managed_oom_update(u);
 
         return 0;
 
@@ -2630,7 +2631,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag
                  * sets one of the ManagedOOM*= properties to "kill", then later removes it. systemd-oomd needs to
                  * know to stop monitoring when the unit changes from "kill" -> "auto" on daemon-reload, but we don't
                  * have the information on the property. Thus, indiscriminately send an update. */
-                if (UNIT_IS_INACTIVE_OR_FAILED(ns) || ns == UNIT_ACTIVE)
+                if (UNIT_IS_INACTIVE_OR_FAILED(ns) || UNIT_IS_ACTIVE_OR_RELOADING(ns))
                         (void) manager_varlink_send_managed_oom_update(u);
         }
 

src/home/homectl.c

@@ -99,7 +99,7 @@ static int acquire_bus(sd_bus **bus) {
 
         r = bus_connect_transport(arg_transport, arg_host, false, bus);
         if (r < 0)
-                return log_error_errno(r, "Failed to connect to bus: %m");
+                return bus_log_connect_error(r);
 
         (void) sd_bus_set_allow_interactive_authorization(*bus, arg_ask_password);
 

src/libsystemd/sd-bus/bus-container.c

@@ -9,6 +9,7 @@
 #include "fd-util.h"
 #include "namespace-util.h"
 #include "process-util.h"
+#include "string-util.h"
 #include "util.h"
 
 int bus_container_connect_socket(sd_bus *b) {
@@ -24,10 +25,15 @@ int bus_container_connect_socket(sd_bus *b) {
         assert(b->nspid > 0 || b->machine);
 
         if (b->nspid <= 0) {
+                log_debug("sd-bus: connecting bus%s%s to machine %s...",
+                          b->description ? " " : "", strempty(b->description), b->machine);
+
                 r = container_get_leader(b->machine, &b->nspid);
                 if (r < 0)
                         return r;
-        }
+        } else
+                log_debug("sd-bus: connecting bus%s%s to namespace of PID "PID_FMT"...",
+                          b->description ? " " : "", strempty(b->description), b->nspid);
 
         r = namespace_open(b->nspid, &pidnsfd, &mntnsfd, NULL, &usernsfd, &rootfd);
         if (r < 0)

src/libsystemd/sd-bus/bus-socket.c

@@ -885,6 +885,13 @@ int bus_socket_connect(sd_bus *b) {
                 assert(b->output_fd < 0);
                 assert(b->sockaddr.sa.sa_family != AF_UNSPEC);
 
+                if (DEBUG_LOGGING) {
+                        _cleanup_free_ char *pretty = NULL;
+                        (void) sockaddr_pretty(&b->sockaddr.sa, b->sockaddr_size, false, true, &pretty);
+                        log_debug("sd-bus: starting bus%s%s by connecting to %s...",
+                                  b->description ? " " : "", strempty(b->description), strnull(pretty));
+                }
+
                 b->input_fd = socket(b->sockaddr.sa.sa_family, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0);
                 if (b->input_fd < 0)
                         return -errno;
@@ -956,6 +963,9 @@ int bus_socket_exec(sd_bus *b) {
         assert(b->exec_path);
         assert(b->busexec_pid == 0);
 
+        log_debug("sd-bus: starting bus%s%s with %s...",
+                  b->description ? " " : "", strempty(b->description), b->exec_path);
+
         r = socketpair(AF_UNIX, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0, s);
         if (r < 0)
                 return -errno;

src/libsystemd/sd-bus/sd-bus.c

@@ -1155,6 +1155,16 @@ static int bus_start_fd(sd_bus *b) {
         assert(b->input_fd >= 0);
         assert(b->output_fd >= 0);
 
+        if (DEBUG_LOGGING) {
+                _cleanup_free_ char *pi = NULL, *po = NULL;
+                (void) fd_get_path(b->input_fd, &pi);
+                (void) fd_get_path(b->output_fd, &po);
+                log_debug("sd-bus: starting bus%s%s on fds %d/%d (%s, %s)...",
+                          b->description ? " " : "", strempty(b->description),
+                          b->input_fd, b->output_fd,
+                          pi ?: "???", po ?: "???");
+        }
+
         r = fd_nonblock(b->input_fd, true);
         if (r < 0)
                 return r;
@@ -1330,7 +1340,8 @@ int bus_set_address_user(sd_bus *b) {
 
                 e = secure_getenv("XDG_RUNTIME_DIR");
                 if (!e)
-                        return -ENOENT;
+                        return log_debug_errno(SYNTHETIC_ERRNO(ENOMEDIUM),
+                                               "sd-bus: $XDG_RUNTIME_DIR not set, cannot connect to user bus.");
 
                 ee = bus_address_escape(e);
                 if (!ee)

src/libsystemd/sd-bus/test-bus-cleanup.c

@@ -23,7 +23,7 @@ static int test_bus_open(void) {
         int r;
 
         r = sd_bus_open_user(&bus);
-        if (IN_SET(r, -ECONNREFUSED, -ENOENT)) {
+        if (IN_SET(r, -ECONNREFUSED, -ENOENT, -ENOMEDIUM)) {
                 r = sd_bus_open_system(&bus);
                 if (IN_SET(r, -ECONNREFUSED, -ENOENT))
                         return r;

src/libsystemd/sd-bus/test-bus-track.c

@@ -55,7 +55,7 @@ int main(int argc, char *argv[]) {
         assert_se(r >= 0);
 
         r = sd_bus_open_user(&a);
-        if (IN_SET(r, -ECONNREFUSED, -ENOENT)) {
+        if (IN_SET(r, -ECONNREFUSED, -ENOENT, -ENOMEDIUM)) {
                 r = sd_bus_open_system(&a);
                 if (IN_SET(r, -ECONNREFUSED, -ENOENT))
                         return log_tests_skipped("Failed to connect to bus");

src/login/inhibit.c

@@ -282,7 +282,7 @@ static int run(int argc, char *argv[]) {
 
         r = sd_bus_default_system(&bus);
         if (r < 0)
-                return log_error_errno(r, "Failed to connect to bus: %m");
+                return bus_log_connect_error(r);
 
         if (arg_action == ACTION_LIST)
                 return print_inhibitors(bus);

src/oom/oomd-manager.c

@@ -170,15 +170,12 @@ static int recursively_get_cgroup_context(Hashmap *new_h, const char *path) {
                 if (r < 0)
                         return (r == -ENOMEM) ? r : 0;
 
-                if (oom_group) {
+                if (oom_group)
                         r = oomd_insert_cgroup_context(NULL, new_h, cg_path);
-                        if (r == -ENOMEM)
+                else
-                                return r;
-                } else {
                         r = recursively_get_cgroup_context(new_h, cg_path);
                 if (r == -ENOMEM)
                         return r;
-                }
         } while ((r = cg_read_subgroup(d, &subpath)) > 0);
 
         return 0;

src/oom/test-oomd-util.c

@@ -39,6 +39,7 @@ static int fork_and_sleep(unsigned sleep_min) {
 static void test_oomd_cgroup_kill(void) {
         _cleanup_free_ char *cgroup_root = NULL, *cgroup = NULL;
         int pid[2];
+        int r;
 
         if (geteuid() != 0)
                 return (void) log_tests_skipped("not root");
@@ -52,19 +53,20 @@ static void test_oomd_cgroup_kill(void) {
          * by the test so that pid1 doesn't delete it before we can read the xattrs. */
         cgroup = path_join(cgroup_root, "oomdkilltest");
         assert(cgroup);
+        assert_se(cg_create(SYSTEMD_CGROUP_CONTROLLER, cgroup) >= 0);
 
         /* If we don't have permissions to set xattrs we're likely in a userns or missing capabilities */
-        if (cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_test", "test", 4, 0) == -EPERM)
+        r = cg_set_xattr(SYSTEMD_CGROUP_CONTROLLER, cgroup, "user.oomd_test", "test", 4, 0);
-                return (void) log_tests_skipped("no permissions to set user xattrs");
+        if (IN_SET(r, -EPERM, -ENOTSUP))
+                return (void) log_tests_skipped("Cannot set user xattrs");
 
         /* Do this twice to also check the increment behavior on the xattrs */
         for (int i = 0; i < 2; i++) {
                 _cleanup_free_ char *v = NULL;
-                int r;
 
                 for (int j = 0; j < 2; j++) {
                         pid[j] = fork_and_sleep(5);
-                        assert_se(cg_create_and_attach(SYSTEMD_CGROUP_CONTROLLER, cgroup, pid[j]) >= 0);
+                        assert_se(cg_attach(SYSTEMD_CGROUP_CONTROLLER, cgroup, pid[j]) >= 0);
                 }
 
                 r = oomd_cgroup_kill(cgroup, false /* recurse */, false /* dry run */);

src/portable/portablectl.c

@@ -178,7 +178,7 @@ static int acquire_bus(sd_bus **bus) {
 
         r = bus_connect_transport(arg_transport, arg_host, false, bus);
         if (r < 0)
-                return log_error_errno(r, "Failed to connect to bus: %m");
+                return bus_log_connect_error(r);
 
         (void) sd_bus_set_allow_interactive_authorization(*bus, arg_ask_password);
 

src/shared/bootspec.c

@@ -80,7 +80,7 @@ static int boot_entry_load(
                 return log_oom();
 
         if (!efi_loader_entry_name_valid(tmp.id))
-                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry: %s", tmp.id);
+                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry name: %s", tmp.id);
 
         tmp.path = strdup(path);
         if (!tmp.path)
@@ -327,7 +327,7 @@ static int boot_entry_load_unified(
                 return log_oom();
 
         if (!efi_loader_entry_name_valid(tmp.id))
-                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry: %s", tmp.id);
+                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid loader entry name: %s", tmp.id);
 
         tmp.path = strdup(path);
         if (!tmp.path)

src/shared/bus-util.h

@@ -38,8 +38,14 @@ int bus_connect_user_systemd(sd_bus **_bus);
 int bus_connect_transport(BusTransport transport, const char *host, bool user, sd_bus **bus);
 int bus_connect_transport_systemd(BusTransport transport, const char *host, bool user, sd_bus **bus);
 
+#define bus_log_address_error(r)                                        \
+        log_error_errno(r,                                              \
+                r == -ENOMEDIUM ? "Failed to set bus address: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined" : \
+                                  "Failed to set bus address: %m")
 #define bus_log_connect_error(r)                                        \
-        log_error_errno(r, "Failed to create bus connection: %m")
+        log_error_errno(r,                                              \
+                r == -ENOMEDIUM ? "Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined" : \
+                                  "Failed to connect to bus: %m")
 
 #define bus_log_parse_error(r)                                  \
         log_error_errno(r, "Failed to parse bus message: %m")

src/systemctl/systemctl-util.c

@@ -52,7 +52,7 @@ int acquire_bus(BusFocus focus, sd_bus **ret) {
                 else
                         r = bus_connect_transport(arg_transport, arg_host, user, &buses[focus]);
                 if (r < 0)
-                        return log_error_errno(r, "Failed to connect to bus: %m");
+                        return bus_log_connect_error(r);
 
                 (void) sd_bus_set_allow_interactive_authorization(buses[focus], arg_ask_password);
         }

src/tmpfiles/tmpfiles.c

@@ -1063,7 +1063,7 @@ static int parse_acls_from_arg(Item *item) {
         if (r < 0)
                 log_warning_errno(r, "Failed to parse ACL \"%s\": %m. Ignoring", item->argument);
 #else
-        log_warning_errno(SYNTHETIC_ERRNO(ENOSYS), "ACLs are not supported. Ignoring");
+        log_warning("ACLs are not supported. Ignoring.");
 #endif
 
         return 0;

units/systemd-oomd.service.in

@@ -10,7 +10,6 @@
 [Unit]
 Description=Userspace Out-Of-Memory (OOM) Killer
 Documentation=man:systemd-oomd.service(8)
-ConditionCapability=CAP_KILL
 DefaultDependencies=no
 Before=multi-user.target shutdown.target
 Conflicts=shutdown.target