From bd62b7448623fbe36665e089977731efb55524c0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 15:51:08 +0100 Subject: [PATCH 1/5] sd-bus: add sd_bus_close_unref() helper It's similar to sd_bus_flush_close_unref() but doesn't do the flushing. This is useful since this will still discnnect the connection properly but not synchronously wait for the peer to take our messages. Primary usecase is within _cleanup_() expressions where synchronously waiting on the peer is not OK. --- man/sd_bus_new.xml | 44 ++++++++++++++++++++++++---------- src/core/dbus.c | 2 +- src/libsystemd/libsystemd.sym | 5 ++++ src/libsystemd/sd-bus/sd-bus.c | 13 +++++++--- src/systemd/sd-bus.h | 2 ++ 5 files changed, 50 insertions(+), 16 deletions(-) diff --git a/man/sd_bus_new.xml b/man/sd_bus_new.xml index 1bc011d70a..cc08e6be17 100644 --- a/man/sd_bus_new.xml +++ b/man/sd_bus_new.xml @@ -23,6 +23,8 @@ sd_bus_ref sd_bus_unref sd_bus_unrefp + sd_bus_close_unref + sd_bus_close_unrefp sd_bus_flush_close_unref sd_bus_flush_close_unrefp @@ -49,8 +51,8 @@ - void sd_bus_unrefp - sd_bus **busp + sd_bus *sd_bus_close_unref + sd_bus *bus @@ -58,6 +60,16 @@ sd_bus *bus + + void sd_bus_unrefp + sd_bus **busp + + + + void sd_bus_close_unrefp + sd_bus **busp + + void sd_bus_flush_close_unrefp sd_bus **busp @@ -124,17 +136,25 @@ execute no operation if that is NULL. - sd_bus_flush_close_unref() is similar to sd_bus_unref(), but first - executes sd_bus_flush3 as well - as sd_bus_close3, ensuring that - any pending messages are properly flushed out before the reference to the connection is dropped and possibly the - object freed. This call is particularly useful immediately before exiting from a program as it ensures that any - pending outgoing messages are written out, and unprocessed but queued incoming messages released before the - connection is terminated and released. + sd_bus_close_unref() is similar to sd_bus_unref(), but + first executes + sd_bus_close3, + ensuring that the connection is terminated before the reference to the connection is dropped and possibly + the object freed. - sd_bus_flush_close_unrefp() is similar to - sd_bus_flush_close_unref(), but may be used in GCC's and LLVM's Clean-up Variable Attribute, - see above. + sd_bus_flush_close_unref() is similar to sd_bus_unref(), + but first executes + sd_bus_flush3 as well + as sd_bus_close3, + ensuring that any pending messages are synchronously flushed out before the reference to the connection + is dropped and possibly the object freed. This call is particularly useful immediately before exiting + from a program as it ensures that any pending outgoing messages are written out, and unprocessed but + queued incoming messages released before the connection is terminated and released. + + sd_bus_close_unrefp() is similar to + sd_bus_close_unref(), but may be used in GCC's and LLVM's Clean-up Variable + Attribute, see above. Similarly, sd_bus_flush_close_unrefp() is similar to + sd_bus_flush_close_unref(). diff --git a/src/core/dbus.c b/src/core/dbus.c index 5908ad792a..ae595e0567 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -611,7 +611,7 @@ static int bus_setup_disconnected_match(Manager *m, sd_bus *bus) { } static int bus_on_connection(sd_event_source *s, int fd, uint32_t revents, void *userdata) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; _cleanup_close_ int nfd = -1; Manager *m = userdata; sd_id128_t id; diff --git a/src/libsystemd/libsystemd.sym b/src/libsystemd/libsystemd.sym index 96e6347795..a6748ceb20 100644 --- a/src/libsystemd/libsystemd.sym +++ b/src/libsystemd/libsystemd.sym @@ -671,3 +671,8 @@ global: sd_event_source_get_floating; sd_event_source_set_floating; } LIBSYSTEMD_239; + +LIBSYSTEMD_241 { +global: + sd_bus_close_unref; +} LIBSYSTEMD_240; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 3b00bc8157..1ff858f32d 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -1556,17 +1556,24 @@ _public_ void sd_bus_close(sd_bus *bus) { bus_close_inotify_fd(bus); } +_public_ sd_bus *sd_bus_close_unref(sd_bus *bus) { + if (!bus) + return NULL; + + sd_bus_close(bus); + + return sd_bus_unref(bus); +} + _public_ sd_bus* sd_bus_flush_close_unref(sd_bus *bus) { if (!bus) return NULL; /* Have to do this before flush() to prevent hang */ bus_kill_exec(bus); - sd_bus_flush(bus); - sd_bus_close(bus); - return sd_bus_unref(bus); + return sd_bus_close_unref(bus); } void bus_enter_closing(sd_bus *bus) { diff --git a/src/systemd/sd-bus.h b/src/systemd/sd-bus.h index 4c1acab9f3..129cc93328 100644 --- a/src/systemd/sd-bus.h +++ b/src/systemd/sd-bus.h @@ -170,6 +170,7 @@ void sd_bus_close(sd_bus *bus); sd_bus *sd_bus_ref(sd_bus *bus); sd_bus *sd_bus_unref(sd_bus *bus); +sd_bus *sd_bus_close_unref(sd_bus *bus); sd_bus *sd_bus_flush_close_unref(sd_bus *bus); void sd_bus_default_flush_close(void); @@ -493,6 +494,7 @@ int sd_bus_track_get_destroy_callback(sd_bus_track *s, sd_bus_destroy_t *ret); /* Define helpers so that __attribute__((cleanup(sd_bus_unrefp))) and similar may be used. */ _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_bus, sd_bus_unref); +_SD_DEFINE_POINTER_CLEANUP_FUNC(sd_bus, sd_bus_close_unref); _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_bus, sd_bus_flush_close_unref); _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_bus_slot, sd_bus_slot_unref); _SD_DEFINE_POINTER_CLEANUP_FUNC(sd_bus_message, sd_bus_message_unref); From 92e31da10065b6eadb06f6f92e386b97a0058f28 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 15:53:15 +0100 Subject: [PATCH 2/5] tree-wide: make sure to flush/close all bus connections when our daemons exit Let's make sure everything is written out ebfore we exit, so that no messages keep our bus connections referenced. Fixes: #11462 --- src/login/logind.c | 2 +- src/login/user-runtime-dir.c | 2 +- src/machine/machined.c | 2 +- src/network/networkd-manager.c | 2 +- src/portable/portabled.c | 2 +- src/resolve/resolved-manager.c | 2 +- src/stdio-bridge/stdio-bridge.c | 2 +- src/timesync/timesyncd-manager.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/login/logind.c b/src/login/logind.c index 8d85de9b43..95ec0a57c6 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -146,7 +146,7 @@ static Manager* manager_unref(Manager *m) { bus_verify_polkit_async_registry_free(m->polkit_registry); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); safe_close(m->reserve_vt_fd); diff --git a/src/login/user-runtime-dir.c b/src/login/user-runtime-dir.c index 5e58e4baad..eb66e18de1 100644 --- a/src/login/user-runtime-dir.c +++ b/src/login/user-runtime-dir.c @@ -22,7 +22,7 @@ static int acquire_runtime_dir_size(uint64_t *ret) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL; int r; r = sd_bus_default_system(&bus); diff --git a/src/machine/machined.c b/src/machine/machined.c index 9f23e369a4..0b92b1c6ee 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -81,7 +81,7 @@ static Manager* manager_unref(Manager *m) { bus_verify_polkit_async_registry_free(m->polkit_registry); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); return mfree(m); diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 81c81f18af..c8d369e2a0 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1456,7 +1456,7 @@ void manager_free(Manager *m) { sd_device_monitor_unref(m->device_monitor); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); free(m->dynamic_timezone); free(m->dynamic_hostname); diff --git a/src/portable/portabled.c b/src/portable/portabled.c index 63fc340469..49a359fd31 100644 --- a/src/portable/portabled.c +++ b/src/portable/portabled.c @@ -53,7 +53,7 @@ static Manager* manager_unref(Manager *m) { bus_verify_polkit_async_registry_free(m->polkit_registry); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); return mfree(m); diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index b7dc09ae37..b3d35c8341 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -689,7 +689,7 @@ Manager *manager_free(Manager *m) { manager_mdns_stop(m); manager_dns_stub_stop(m); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); sd_event_source_unref(m->sigusr1_event_source); sd_event_source_unref(m->sigusr2_event_source); diff --git a/src/stdio-bridge/stdio-bridge.c b/src/stdio-bridge/stdio-bridge.c index 3a21aa4aed..7060897ab7 100644 --- a/src/stdio-bridge/stdio-bridge.c +++ b/src/stdio-bridge/stdio-bridge.c @@ -91,7 +91,7 @@ static int parse_argv(int argc, char *argv[]) { } static int run(int argc, char *argv[]) { - _cleanup_(sd_bus_unrefp) sd_bus *a = NULL, *b = NULL; + _cleanup_(sd_bus_flush_close_unrefp) sd_bus *a = NULL, *b = NULL; sd_id128_t server_id; bool is_unix; int r, in_fd, out_fd; diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 4b0696f3a3..b3bd7235c9 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -937,7 +937,7 @@ void manager_free(Manager *m) { sd_resolve_unref(m->resolve); sd_event_unref(m->event); - sd_bus_unref(m->bus); + sd_bus_flush_close_unref(m->bus); free(m); } From aae82d1a36e01bf7efdaa51ec1c12664fef6db04 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 15:54:37 +0100 Subject: [PATCH 3/5] initctl: shorten and update code a bit --- src/initctl/initctl.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/initctl/initctl.c b/src/initctl/initctl.c index c60d4bd740..260dc2eb61 100644 --- a/src/initctl/initctl.c +++ b/src/initctl/initctl.c @@ -233,12 +233,8 @@ static void server_done(Server *s) { while (s->fifos) fifo_free(s->fifos); - safe_close(s->epoll_fd); - - if (s->bus) { - sd_bus_flush(s->bus); - sd_bus_unref(s->bus); - } + s->epoll_fd = safe_close(s->epoll_fd); + s->bus = sd_bus_flush_close_unref(s->bus); } static int server_init(Server *s, unsigned n_sockets) { From b1a4981aed3f5e939dbe5a211ca066ac69699b24 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 16:07:22 +0100 Subject: [PATCH 4/5] tree-wide: whenever we allocate a new bus object, close it before dropping final ref This doesn't really change much, but feels more correct to do, as it ensures that all messages currently queued in the bus connections are definitely unreffed and thus destryoing of the connection object will follow immediately. Strictly speaking this change is entirely unnecessary, since nothing else could have acquired a ref to the connection and queued a message in, however, now that we have the new sd_bus_close_unref() helper it makes a lot of sense to use it here, to ensure that whatever happens nothing that might have been queued fucks with us. --- src/busctl/busctl.c | 2 +- src/core/dbus.c | 4 ++-- src/machine/machine-dbus.c | 2 +- src/shared/bus-util.c | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 96b4177495..08d9e70c96 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -67,7 +67,7 @@ static int json_transform_message(sd_bus_message *m, JsonVariant **ret); static void json_dump_with_flags(JsonVariant *v, FILE *f); static int acquire_bus(bool set_monitor, sd_bus **ret) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; r = sd_bus_new(&bus); diff --git a/src/core/dbus.c b/src/core/dbus.c index ae595e0567..bab4a1a81b 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -876,7 +876,7 @@ static int bus_setup_api(Manager *m, sd_bus *bus) { } int bus_init_api(Manager *m) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; if (m->api_bus) @@ -940,7 +940,7 @@ static int bus_setup_system(Manager *m, sd_bus *bus) { } int bus_init_system(Manager *m) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; if (m->system_bus) diff --git a/src/machine/machine-dbus.c b/src/machine/machine-dbus.c index 48270b3709..7a558df898 100644 --- a/src/machine/machine-dbus.c +++ b/src/machine/machine-dbus.c @@ -454,7 +454,7 @@ static int container_bus_new(Machine *m, sd_bus_error *error, sd_bus **ret) { break; case MACHINE_CONTAINER: { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; char *address; r = sd_bus_new(&bus); diff --git a/src/shared/bus-util.c b/src/shared/bus-util.c index 976643e4ce..cbcf698e96 100644 --- a/src/shared/bus-util.c +++ b/src/shared/bus-util.c @@ -559,7 +559,7 @@ int bus_check_peercred(sd_bus *c) { } int bus_connect_system_systemd(sd_bus **_bus) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; assert(_bus); @@ -592,7 +592,7 @@ int bus_connect_system_systemd(sd_bus **_bus) { } int bus_connect_user_systemd(sd_bus **_bus) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *ee = NULL; const char *e; int r; @@ -1279,7 +1279,7 @@ int bus_map_all_properties( } int bus_connect_transport(BusTransport transport, const char *host, bool user, sd_bus **ret) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; int r; assert(transport >= 0); @@ -1666,7 +1666,7 @@ int bus_track_add_name_many(sd_bus_track *t, char **l) { } int bus_open_system_watch_bind_with_description(sd_bus **ret, const char *description) { - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_close_unrefp) sd_bus *bus = NULL; const char *e; int r; From 81a40363f5b221aad7c65860001e265a07bf4fd1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 17 Jan 2019 16:10:58 +0100 Subject: [PATCH 5/5] core: shorten code by using new helper --- src/core/dbus.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index bab4a1a81b..255b86e7a4 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -1080,8 +1080,7 @@ static void destroy_bus(Manager *m, sd_bus **bus) { sd_bus_flush(*bus); /* And destroy the object */ - sd_bus_close(*bus); - *bus = sd_bus_unref(*bus); + *bus = sd_bus_close_unref(*bus); } void bus_done_api(Manager *m) {