Script 'mail_helper' called by obssrc Hello community, here is the log from the commit of package dbus-broker for openSUSE:Factory checked in at 2021-02-15 23:20:09 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Comparing /work/SRC/openSUSE:Factory/dbus-broker (Old) and /work/SRC/openSUSE:Factory/.dbus-broker.new.28504 (New) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Package is "dbus-broker" Mon Feb 15 23:20:09 2021 rev:10 rq:872356 version:27 Changes: -------- --- /work/SRC/openSUSE:Factory/dbus-broker/dbus-broker.changes 2020-12-10 15:59:21.642923515 +0100 +++ /work/SRC/openSUSE:Factory/.dbus-broker.new.28504/dbus-broker.changes 2021-02-15 23:21:54.251897996 +0100 @@ -1,0 +2,24 @@ +Mon Feb 15 10:47:52 UTC 2021 - Jan Engelhardt <[email protected]> + +- Update to release 27 + * Fix several bugs with the new service-activation tracking, + including a race-condition when restarting activatable + services. + * Be more verbose about denied configuration access and print + the file-path for better diagnostics. + +------------------------------------------------------------------- +Thu Jan 21 13:28:09 UTC 2021 - Jan Engelhardt <[email protected]> + +- Update to release 26 + * Improve the service activation tracking of the compatibility + launcher. We now track spawned systemd units for their entire + lifetime, so we can properly detect when activations fail. + * Work around a kernel off-by-one error in the socket queue + accounting to fix a race-condition where dbus clients might + not be dispatched. + * Support running without `shmem` configured in the kernel. + This will make the broker run better on limited embedded + devices. + +------------------------------------------------------------------- Old: ---- dbus-broker-25.tar.xz New: ---- dbus-broker-27.tar.xz ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Other differences: ------------------ ++++++ dbus-broker.spec ++++++ --- /var/tmp/diff_new_pack.GBDPpY/_old 2021-02-15 23:21:54.827898857 +0100 +++ /var/tmp/diff_new_pack.GBDPpY/_new 2021-02-15 23:21:54.831898863 +0100 @@ -1,7 +1,7 @@ # # spec file for package dbus-broker # -# Copyright (c) 2020 SUSE LLC +# Copyright (c) 2021 SUSE LLC # # All modifications and additions to the file contributed by third parties # remain the property of their copyright owners, unless otherwise agreed @@ -17,7 +17,7 @@ Name: dbus-broker -Version: 25 +Version: 27 Release: 0 Summary: XDG-conforming message bus implementation License: Apache-2.0 ++++++ dbus-broker-25.tar.xz -> dbus-broker-27.tar.xz ++++++ diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/AUTHORS new/dbus-broker-27/AUTHORS --- old/dbus-broker-25/AUTHORS 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/AUTHORS 2021-02-04 15:01:53.756929200 +0100 @@ -15,6 +15,7 @@ Copyright (C) 2016-2019 Red Hat, Inc. AUTHORS: (ordered alphabetically) + Chris Paulson-Ellis Chris PeBenito <[email protected]> Daniel Rusek <[email protected]> Daniele Nicolodi <[email protected]> @@ -27,5 +28,6 @@ Marc-Antoine Perennou <[email protected]> Michal Schmidt <[email protected]> Mike Gilbert <[email protected]> + Tim Gates <[email protected]> Tom Gundersen <[email protected]> Yanko Kaneti <[email protected]> diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/NEWS.md new/dbus-broker-27/NEWS.md --- old/dbus-broker-25/NEWS.md 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/NEWS.md 2021-02-15 10:25:22.568052000 +0100 @@ -1,5 +1,35 @@ # dbus-broker - Linux D-Bus Message Broker +## CHANGES WITH 27: + + * Fix several bugs with the new service-activation tracking, including + a race-condition when restarting activatable services. Note that this + includes a change to the internal controller API, which is used to + communicate between the launcher and the broker. + + * Be more verbose about denied configuration access and print the + file-path for better diagnostics. + + Contributions from: David Rheinsberg + + - Du??lingen, 2021-02-24 + +## CHANGES WITH 26: + + * Improve the service activation tracking of the compatibility + launcher. We now track spawned systemd units for their entire + lifetime, so we can properly detect when activations fail. + + * Work around a kernel off-by-one error in the socket queue accounting + to fix a race-condition where dbus clients might not be dispatched. + + * Support running without `shmem` configured in the kernel. This will + make the broker run better on limited embedded devices. + + Contributions from: Chris Paulson-Ellis, David Rheinsberg, Tim Gates + + - Du??lingen, 2021-01-20 + ## CHANGES WITH 25: * Fix an assertion failure when disconnecting monitors with active diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/docs/dbus-broker.rst new/dbus-broker-27/docs/dbus-broker.rst --- old/dbus-broker-25/docs/dbus-broker.rst 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/docs/dbus-broker.rst 2021-02-08 13:49:11.669213300 +0100 @@ -189,14 +189,22 @@ | **method** Release() -> () | | # Reset the activation state of this name. Any pending activation -| # requests are canceled. -| **method** Reset() -> () +| # requests are canceled. The call requires a serial number to be +| # passed along. This must be the serial number received by the last +| # activation even on this name. Calls for other serial numbers are +| # silently ignored and considered stale. +| **method** Reset(**t** *serial*) -> () | | # This signal is sent whenever a client requests activation of this | # name. Note that multiple activation requests are coalesced by the | # broker. The controller can cancel outstanding requests via the | # **Reset()** method. -| **signal** Activate() +| # The broker sends a serial number with the event. This number +| # represents the activation request and must be used when reacting +| # to the request with methods like *Reset()*. The serial number is +| # unique for each event, and is never reused. A serial number of 0 +| # is never sent and considered invalid. +| **signal** Activate(**t** *serial*) | | } | } diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/meson.build new/dbus-broker-27/meson.build --- old/dbus-broker-25/meson.build 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/meson.build 2021-02-15 10:25:37.068088800 +0100 @@ -5,7 +5,7 @@ project( 'dbus-broker', 'c', - version: '25', + version: '27', license: 'Apache', default_options: [ 'c_std=c11', diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/broker/controller-dbus.c new/dbus-broker-27/src/broker/controller-dbus.c --- old/dbus-broker-25/src/broker/controller-dbus.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/broker/controller-dbus.c 2021-02-08 13:49:11.669213300 +0100 @@ -54,6 +54,13 @@ _body \ ) +static const CDVarType controller_type_in_t[] = { + C_DVAR_T_INIT( + C_DVAR_T_TUPLE1( + C_DVAR_T_t + ) + ) +}; static const CDVarType controller_type_in_v[] = { C_DVAR_T_INIT( C_DVAR_T_TUPLE1( @@ -304,7 +311,7 @@ if (!name) return CONTROLLER_E_NAME_NOT_FOUND; - r = controller_name_reset(name); + r = controller_name_reset(name, name->activation.pending); if (r) return error_trace(r); @@ -352,9 +359,10 @@ static int controller_method_name_reset(Controller *controller, const char *path, CDVar *in_v, FDList *fds, CDVar *out_v) { ControllerName *name; + uint64_t serial; int r; - c_dvar_read(in_v, "()"); + c_dvar_read(in_v, "(t)", &serial); r = controller_end_read(in_v); if (r) @@ -364,7 +372,7 @@ if (!name) return CONTROLLER_E_NAME_NOT_FOUND; - r = controller_name_reset(name); + r = controller_name_reset(name, serial); if (r) return error_trace(r); @@ -457,7 +465,7 @@ static int controller_dispatch_name(Controller *controller, uint32_t serial, const char *method, const char *path, const char *signature, Message *message) { static const ControllerMethod methods[] = { - { "Reset", controller_method_name_reset, c_dvar_type_unit, controller_type_out_unit }, + { "Reset", controller_method_name_reset, controller_type_in_t, controller_type_out_unit }, { "Release", controller_method_name_release, c_dvar_type_unit, controller_type_out_unit }, }; @@ -636,11 +644,13 @@ /** * controller_dbus_send_activation() - XXX */ -int controller_dbus_send_activation(Controller *controller, const char *path) { +int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial) { static const CDVarType type[] = { C_DVAR_T_INIT( CONTROLLER_T_MESSAGE( - C_DVAR_T_TUPLE0 + C_DVAR_T_TUPLE1( + C_DVAR_T_t + ) ) ) }; @@ -651,11 +661,13 @@ int r; c_dvar_begin_write(&var, (__BYTE_ORDER == __BIG_ENDIAN), type, 1); - c_dvar_write(&var, "((yyyyuu[(y<o>)(y<s>)(y<s>)])())", + c_dvar_write(&var, "((yyyyuu[(y<o>)(y<s>)(y<s>)(y<g>)])(t))", c_dvar_is_big_endian(&var) ? 'B' : 'l', DBUS_MESSAGE_TYPE_SIGNAL, DBUS_HEADER_FLAG_NO_REPLY_EXPECTED, 1, 0, (uint32_t)-1, DBUS_MESSAGE_FIELD_PATH, c_dvar_type_o, path, DBUS_MESSAGE_FIELD_INTERFACE, c_dvar_type_s, "org.bus1.DBus.Name", - DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate"); + DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate", + DBUS_MESSAGE_FIELD_SIGNATURE, c_dvar_type_g, "t", + serial); r = c_dvar_end_write(&var, &data, &n_data); if (r) diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/broker/controller.c new/dbus-broker-27/src/broker/controller.c --- old/dbus-broker-25/src/broker/controller.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/broker/controller.c 2021-02-08 13:49:11.669213300 +0100 @@ -66,10 +66,10 @@ /** * controller_name_reset() - XXX */ -int controller_name_reset(ControllerName *name) { +int controller_name_reset(ControllerName *name, uint64_t serial) { int r; - r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation); + r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation, serial); if (r) return error_fold(r); @@ -79,8 +79,8 @@ /** * controller_name_activate() - XXX */ -int controller_name_activate(ControllerName *name) { - return controller_dbus_send_activation(name->controller, name->path); +int controller_name_activate(ControllerName *name, uint64_t serial) { + return controller_dbus_send_activation(name->controller, name->path, serial); } static int controller_reload_compare(CRBTree *t, void *k, CRBNode *rb) { @@ -311,7 +311,7 @@ if (r) return error_trace(r); - r = activation_init(&name->activation, name_entry, user_entry); + r = activation_init(&name->activation, &controller->broker->bus, name_entry, user_entry); if (r) return (r == ACTIVATION_E_ALREADY_ACTIVATABLE) ? CONTROLLER_E_NAME_IS_ACTIVATABLE : error_fold(r); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/broker/controller.h new/dbus-broker-27/src/broker/controller.h --- old/dbus-broker-25/src/broker/controller.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/broker/controller.h 2021-02-08 13:49:11.669213300 +0100 @@ -101,8 +101,8 @@ /* names */ ControllerName *controller_name_free(ControllerName *name); -int controller_name_reset(ControllerName *name); -int controller_name_activate(ControllerName *name); +int controller_name_reset(ControllerName *name, uint64_t serial); +int controller_name_activate(ControllerName *name, uint64_t serial); C_DEFINE_CLEANUP(ControllerName *, controller_name_free); @@ -144,7 +144,7 @@ ControllerReload *controller_find_reload(Controller *controller, uint32_t serial); int controller_dbus_dispatch(Controller *controller, Message *message); -int controller_dbus_send_activation(Controller *controller, const char *path); +int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial); int controller_dbus_send_reload(Controller *controller, User *user, uint32_t serial); int controller_dbus_send_environment(Controller *controller, const char * const *env, size_t n_env); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/bus/activation.c new/dbus-broker-27/src/bus/activation.c --- old/dbus-broker-25/src/bus/activation.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/bus/activation.c 2021-02-08 13:49:11.682546600 +0100 @@ -7,6 +7,7 @@ #include <stdlib.h> #include "broker/controller.h" #include "bus/activation.h" +#include "bus/bus.h" #include "bus/name.h" #include "bus/policy.h" #include "dbus/message.h" @@ -48,13 +49,14 @@ /** * activation_init() - XXX */ -int activation_init(Activation *a, Name *name, User *user) { +int activation_init(Activation *a, Bus *bus, Name *name, User *user) { _c_cleanup_(activation_deinitp) Activation *activation = a; if (name->activation) return ACTIVATION_E_ALREADY_ACTIVATABLE; *activation = (Activation)ACTIVATION_NULL(*activation); + activation->bus = bus; activation->name = name_ref(name); activation->user = user_ref(user); @@ -113,14 +115,15 @@ static int activation_request(Activation *activation) { int r; - if (activation->requested) + if (activation->pending) return 0; - r = controller_name_activate(CONTROLLER_NAME(activation)); + r = controller_name_activate(CONTROLLER_NAME(activation), + ++activation->bus->activation_ids); if (r) return error_fold(r); - activation->requested = true; + activation->pending = activation->bus->activation_ids; return 0; } diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/bus/activation.h new/dbus-broker-27/src/bus/activation.h --- old/dbus-broker-25/src/bus/activation.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/bus/activation.h 2021-02-08 13:49:11.685880000 +0100 @@ -13,6 +13,7 @@ typedef struct Activation Activation; typedef struct ActivationMessage ActivationMessage; typedef struct ActivationRequest ActivationRequest; +typedef struct Bus Bus; typedef struct Message Message; typedef struct Name Name; typedef struct NameOwner NameOwner; @@ -43,11 +44,12 @@ }; struct Activation { + Bus *bus; Name *name; User *user; CList activation_messages; CList activation_requests; - bool requested : 1; + uint64_t pending; }; #define ACTIVATION_NULL(_x) { \ @@ -65,7 +67,7 @@ /* activation */ -int activation_init(Activation *activation, Name *name, User *user); +int activation_init(Activation *activation, Bus *bus, Name *name, User *user); void activation_deinit(Activation *activation); void activation_get_stats_for(Activation *activation, diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/bus/bus.h new/dbus-broker-27/src/bus/bus.h --- old/dbus-broker-25/src/bus/bus.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/bus/bus.h 2021-02-08 13:49:11.685880000 +0100 @@ -50,6 +50,7 @@ uint64_t n_monitors; uint64_t listener_ids; + uint64_t activation_ids; Metrics metrics; }; diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/bus/driver.c new/dbus-broker-27/src/bus/driver.c --- old/dbus-broker-25/src/bus/driver.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/bus/driver.c 2021-02-08 13:49:11.692546800 +0100 @@ -721,13 +721,19 @@ return 0; } -int driver_name_activation_failed(Bus *bus, Activation *activation) { +int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial) { ActivationRequest *request, *request_safe; ActivationMessage *message, *message_safe; int r; - /* in case the name is activated again in the future, we should request it again */ - activation->requested = false; + /* + * If there is no pending activation, or if it is an event for an old + * activation, we ignore it silently. + */ + if (!activation->pending || serial != activation->pending) + return 0; + + activation->pending = 0; c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) { Peer *sender; @@ -767,7 +773,7 @@ return 0; /* in case the name is dropped again in the future, we should request it again */ - activation->requested = false; + activation->pending = 0; c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) { Peer *sender; diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/bus/driver.h new/dbus-broker-27/src/bus/driver.h --- old/dbus-broker-25/src/bus/driver.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/bus/driver.h 2021-02-08 13:49:11.695880200 +0100 @@ -63,7 +63,7 @@ _DRIVER_E_MAX, }; -int driver_name_activation_failed(Bus *bus, Activation *activation); +int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial); int driver_reload_config_completed(Bus *bus, uint64_t sender_id, uint32_t reply_serial); int driver_reload_config_invalid(Bus *bus, uint64_t sender_id, uint32_t reply_serial); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/dbus/socket.c new/dbus-broker-27/src/dbus/socket.c --- old/dbus-broker-25/src/dbus/socket.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/dbus/socket.c 2021-02-04 15:01:53.776929100 +0100 @@ -743,7 +743,46 @@ if (r < 0) return error_origin(-errno); - if (v > 0) /* treat like EAGAIN */ + /* + * We would like to check for an empty queue here: + * + * if (v > 0) + * return 0; + * + * Unfortunately, the kernel uses the write-buffer as a + * reference counter. This effectively means that when it drops + * buffers from the write-queue, it drops all but 1 from the + * `sk_wmem_alloc` counter, then notifies user-space, and + * eventually drops its final reference, possibly freeing the + * underlying socket structures, if this was the last + * reference. + * + * While it is possible to hit this small race with a single + * kernel thread currently holding this temporary reference, + * technically there can be many threads in parallel. However, + * this would require multiple readers on the other end of the + * dbus-socket, and all of them dispatching data in parallel + * (which does not make sense for stream sockets), and all in + * exactly the same kernel path at the same time. While we + * consider it impossible to hit this with more than one thread + * in the same path, we use 128 as a safety measure here. + * + * Note that the kernel uses `skb->truesize` for write-buffer + * allocations, meaning that even transmitting a single byte + * will allocate buffers larger than 128 bytes. Therefore, this + * seems like a suitable tradeoff. + * + * The preferred fix would be the kernel returning the actual + * data, rather than misusing the counter, but that is not how + * things currently work. + * + * Lastly, we simply return 0 and treat this condition as + * EAGAIN. We know that the kernel will send us an EPOLLOUT + * notification as soon as the write-buffer clears, so we will + * be woken up again when there is no more pending data in the + * outgoing queues. + */ + if (v > 128) return 0; c_list_for_each_entry_safe(buffer, safe, &socket->out.pending, link) diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/launch/config.c new/dbus-broker-27/src/launch/config.c --- old/dbus-broker-25/src/launch/config.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/launch/config.c 2021-02-04 15:01:53.780262500 +0100 @@ -1059,6 +1059,11 @@ if (errno == ENOENT || errno == ENOTDIR) break; + if (errno == EACCES || errno == EPERM) { + CONFIG_ERR(state, "Access denied", ": %s", + state->current->includedir.dir->path); + } + state->error = error_origin(-errno); return; } @@ -1266,6 +1271,10 @@ CONFIG_ERR(&parser->state, "Missing configuration file", ": %s", node->include.file->path); return CONFIG_E_INVALID; + } else if (errno == EACCES || errno == EPERM) { + CONFIG_ERR(&parser->state, "Access denied", + ": %s", node->include.file->path); + return CONFIG_E_INVALID; } return error_origin(-errno); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/launch/launcher.c new/dbus-broker-27/src/launch/launcher.c --- old/dbus-broker-25/src/launch/launcher.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/launch/launcher.c 2021-02-08 13:49:11.695880200 +0100 @@ -386,8 +386,13 @@ static int launcher_on_name_activate(Launcher *launcher, sd_bus_message *m, const char *id) { Service *service; + uint64_t serial; int r; + r = sd_bus_message_read(m, "t", &serial); + if (r < 0) + return error_origin(r); + service = c_rbtree_find_entry(&launcher->services, service_compare, id, @@ -403,7 +408,7 @@ return 0; } - r = service_activate(service); + r = service_activate(service, serial); if (r) return error_fold(r); @@ -1254,6 +1259,35 @@ return 0; } +static int launcher_subscribe(Launcher *launcher) { + int r; + + /* + * The systemd APIs only ever send signals if there is at least one + * subscriber. On the system-bus, the systemd tools themselves already + * subscribe, so the feature is a no-op. However, on the session bus + * this is not always the case. Regardless, we just properly subscribe + * in all circumstances, since we require unit state-change + * notifications. + */ + + r = sd_bus_call_method_async( + launcher->bus_regular, + NULL, + "org.freedesktop.systemd1", + "/org/freedesktop/systemd1", + "org.freedesktop.systemd1.Manager", + "Subscribe", + NULL, + NULL, + "" + ); + if (r < 0) + return error_origin(r); + + return 0; +} + static int bus_method_reload_config(sd_bus_message *message, void *userdata, sd_bus_error *error) { Launcher *launcher = userdata; int r; @@ -1390,6 +1424,10 @@ if (r < 0) return error_origin(r); + r = launcher_subscribe(launcher); + if (r) + return error_trace(r); + if (launcher->uid != (uint32_t)-1) { r = util_drop_permissions(launcher->uid, launcher->gid); if (r) diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/launch/service.c new/dbus-broker-27/src/launch/service.c --- old/dbus-broker-25/src/launch/service.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/launch/service.c 2021-02-08 13:49:11.695880200 +0100 @@ -87,7 +87,9 @@ free(service->unit); free(service->name); free(service->path); - sd_bus_slot_unref(service->slot); + service->slot_start_unit = sd_bus_slot_unref(service->slot_start_unit); + service->slot_query_unit = sd_bus_slot_unref(service->slot_query_unit); + service->slot_watch_unit = sd_bus_slot_unref(service->slot_watch_unit); free(service); return NULL; @@ -178,18 +180,261 @@ return 0; } +static void service_discard_activation(Service *service) { + service->slot_start_unit = sd_bus_slot_unref(service->slot_start_unit); + service->slot_query_unit = sd_bus_slot_unref(service->slot_query_unit); + service->slot_watch_unit = sd_bus_slot_unref(service->slot_watch_unit); +} + +static int service_reset_activation(Service *service) { + _c_cleanup_(c_freep) char *object_path = NULL; + int r; + + service_discard_activation(service); + + r = asprintf(&object_path, "/org/bus1/DBus/Name/%s", service->id); + if (r < 0) + return error_origin(-errno); + + /* + * XXX: Ideally, we would include more detailed error information as + * payload. This would allow the broker to forward this in the + * dbus error to each pending transaction. + * So far, we did not define such errors, so this is left for the + * future. + */ + r = sd_bus_call_method(service->launcher->bus_controller, + NULL, + object_path, + "org.bus1.DBus.Name", + "Reset", + NULL, + NULL, + "t", + service->last_serial); + if (r < 0) + return error_origin(r); + + return 0; +} + +static int service_handle_active_state(Service *service, const char *value) { + int r; + + /* + * The possible values of "ActiveState" are: + * + * active, reloading, inactive, failed, activating, deactivating + * + * We are never interested in positive results, because the broker + * already gets those by tracking the name to be acquired. Therefore, + * we only ever track negative results. This means we only ever react + * to "failed". + * We could also react to units entering "inactive", but we cannot know + * upfront whether the unit is just a oneshot unit and thus is expected + * to enter "inactive" when it finished. Hence, we simply require + * anything to explicitly fail if they want to reset the activation. + */ + + if (!strcmp(value, "failed")) { + r = service_reset_activation(service); + if (r) + return error_trace(r); + } + + return 0; +} + +static int service_query_unit_handler(sd_bus_message *message, void *userdata, sd_bus_error *errorp) { + Service *service = userdata; + const char *v; + int r; + + service->slot_query_unit = sd_bus_slot_unref(service->slot_query_unit); + + r = sd_bus_message_enter_container(message, 'v', "s"); + if (r < 0) + return error_origin(r); + + { + r = sd_bus_message_read(message, "s", &v); + if (r < 0) + return error_origin(r); + } + + r = sd_bus_message_exit_container(message); + if (r < 0) + return error_origin(r); + + r = service_handle_active_state(service, v); + if (r) + return error_trace(r); + + return 0; +} + +static int service_watch_unit_handler(sd_bus_message *message, void *userdata, sd_bus_error *errorp) { + Service *service = userdata; + const char *interface = NULL, *property = NULL, *value = NULL; + bool invalidated = false; + int r; + + /* + * The properties of the bus unit changed. We are only interested in + * the "ActiveState" property. We check whether it is included in the + * payload. If not, we query it, in case it was invalidated. + */ + + /* Parse: "s" */ + { + r = sd_bus_message_read(message, "s", &interface); + if (r < 0) + return error_origin(r); + } + + /* We are not interested in properties other than the Unit-Interface */ + if (strcmp(interface, "org.freedesktop.systemd1.Unit") != 0) + return 0; + + /* Parse: "a{sv}" */ + { + r = sd_bus_message_enter_container(message, 'a', "{sv}"); + if (r < 0) + return error_origin(r); + + while (!sd_bus_message_at_end(message, false)) { + r = sd_bus_message_enter_container(message, 'e', "sv"); + if (r < 0) + return error_origin(r); + + r = sd_bus_message_read(message, "s", &property); + if (r < 0) + return error_origin(r); + + if (!strcmp(property, "ActiveState")) { + r = sd_bus_message_enter_container(message, 'v', "s"); + if (r < 0) + return error_origin(r); + + r = sd_bus_message_read(message, "s", &value); + if (r < 0) + return error_origin(r); + + r = sd_bus_message_exit_container(message); + if (r < 0) + return error_origin(r); + } else { + r = sd_bus_message_skip(message, "v"); + if (r < 0) + return error_origin(r); + } + + r = sd_bus_message_exit_container(message); + if (r < 0) + return error_origin(r); + } + + r = sd_bus_message_exit_container(message); + if (r < 0) + return error_origin(r); + } + + /* Parse: "as" */ + { + r = sd_bus_message_enter_container(message, 'a', "s"); + if (r < 0) + return error_origin(r); + + while (!sd_bus_message_at_end(message, false)) { + r = sd_bus_message_read(message, "s", &property); + if (r < 0) + return error_origin(r); + + if (!strcmp(property, "ActiveState")) + invalidated = true; + } + + r = sd_bus_message_exit_container(message); + if (r < 0) + return error_origin(r); + } + + /* If the value neither changed or was invalidated, discard it. */ + if (!value && !invalidated) + return 0; + + service->slot_query_unit = sd_bus_slot_unref(service->slot_query_unit); + + if (value) { + /* We got a new property value, so handle it. */ + r = service_handle_active_state(service, value); + if (r) + return error_trace(r); + } else { + /* The property was invalidated, so query it. */ + r = sd_bus_call_method_async( + service->launcher->bus_regular, + &service->slot_query_unit, + "org.freedesktop.systemd1", + sd_bus_message_get_path(message), + "org.freedesktop.DBus.Properties", + "Get", + service_query_unit_handler, + service, + "ss", + "org.freedesktop.systemd1.Unit", + "ActiveState" + ); + if (r < 0) + return error_origin(r); + } + + return 0; +} + +static int service_watch_unit(Service *service, const char *unit) { + _c_cleanup_(c_freep) char *object_path = NULL; + int r; + + assert(!service->slot_watch_unit); + assert(!service->slot_query_unit); + + r = sd_bus_path_encode( + "/org/freedesktop/systemd1/unit", + unit, + &object_path + ); + if (r < 0) + return error_origin(r); + + r = sd_bus_match_signal_async( + service->launcher->bus_regular, + &service->slot_watch_unit, + "org.freedesktop.systemd1", + object_path, + "org.freedesktop.DBus.Properties", + "PropertiesChanged", + service_watch_unit_handler, + NULL, + service + ); + if (r < 0) + return error_origin(r); + + return 0; +} + static int service_start_unit_handler(sd_bus_message *message, void *userdata, sd_bus_error *errorp) { Service *service = userdata; Launcher *launcher = service->launcher; - _c_cleanup_(c_freep) char *object_path = NULL; const sd_bus_error *error; int r; - service->slot = sd_bus_slot_unref(service->slot); + service->slot_start_unit = sd_bus_slot_unref(service->slot_start_unit); error = sd_bus_message_get_error(message); if (!error) - /* unit started successfully */ + /* unit queued successfully */ return 1; /* @@ -265,23 +510,9 @@ } } - - /* unit failed, so reset pending activation requsets in the broker */ - r = asprintf(&object_path, "/org/bus1/DBus/Name/%s", service->id); - if (r < 0) - return error_origin(-errno); - - /* XXX: We should forward error-information to the activator. */ - r = sd_bus_call_method(service->launcher->bus_controller, - NULL, - object_path, - "org.bus1.DBus.Name", - "Reset", - NULL, - NULL, - ""); - if (r < 0) - return error_origin(r); + r = service_reset_activation(service); + if (r) + return error_trace(r); return 1; } @@ -291,7 +522,9 @@ _c_cleanup_(sd_bus_message_unrefp) sd_bus_message *method_call = NULL; int r; - service->slot = sd_bus_slot_unref(service->slot); + r = service_watch_unit(service, service->unit); + if (r) + return error_trace(r); r = sd_bus_message_new_method_call(launcher->bus_regular, &method_call, "org.freedesktop.systemd1", @@ -305,7 +538,7 @@ if (r < 0) return error_origin(r); - r = sd_bus_call_async(launcher->bus_regular, &service->slot, method_call, service_start_unit_handler, service, -1); + r = sd_bus_call_async(launcher->bus_regular, &service->slot_start_unit, method_call, service_start_unit_handler, service, -1); if (r < 0) return error_origin(r); @@ -319,8 +552,6 @@ const char *unique_name; int r; - service->slot = sd_bus_slot_unref(service->slot); - r = sd_bus_get_unique_name(launcher->bus_regular, &unique_name); if (r < 0) return error_origin(r); @@ -333,6 +564,10 @@ if (r < 0) return error_origin(-errno); + r = service_watch_unit(service, unit); + if (r) + return error_trace(r); + r = sd_bus_message_new_method_call(launcher->bus_regular, &method_call, "org.freedesktop.systemd1", "/org/freedesktop/systemd1", @@ -394,7 +629,7 @@ if (r < 0) return error_origin(r); - r = sd_bus_message_append(method_call, "b", true); + r = sd_bus_message_append(method_call, "b", false); if (r < 0) return error_origin(r); } @@ -446,6 +681,34 @@ if (r < 0) return error_origin(r); + r = sd_bus_message_open_container(method_call, 'r', "sv"); + if (r < 0) + return error_origin(r); + + { + r = sd_bus_message_append(method_call, "s", "CollectMode"); + if (r < 0) + return error_origin(r); + + r = sd_bus_message_open_container(method_call, 'v', "s"); + if (r < 0) + return error_origin(r); + + { + r = sd_bus_message_append(method_call, "s", "inactive-or-failed"); + if (r < 0) + return error_origin(r); + } + + r = sd_bus_message_close_container(method_call); + if (r < 0) + return error_origin(r); + } + + r = sd_bus_message_close_container(method_call); + if (r < 0) + return error_origin(r); + if (service->user) { /* * Ideally we would unconditionally pass the UID @@ -510,16 +773,54 @@ if (r < 0) return error_origin(r); - r = sd_bus_call_async(launcher->bus_regular, &service->slot, method_call, service_start_unit_handler, service, -1); + r = sd_bus_call_async(launcher->bus_regular, &service->slot_start_unit, method_call, service_start_unit_handler, service, -1); if (r < 0) return error_origin(r); return 0; } -int service_activate(Service *service) { +/** + * service_activate() - trigger a service activation + * @service: service to activate + * @serial: activation serial number + * + * This activates the specified service. Any previous activation is discarded + * silently. The new activation replaces a possible old one. + * + * An activation starts the systemd unit that the dbus-service-file configured. + * In case no unit was specified, a transient unit is created, which spawns the + * executable specified in the dbus-service-file. + * + * The launcher never tracks successfull activations. It is up to the broker to + * consider an activation successful, once the corresponding bus-name is + * claimed. Furthermore, the broker is free to consider an activation failed at + * any point in time, without notifying the launcher. There is no need to + * cancel the activation in the launcher. + * + * The role of the launcher is merely to start the right units on request, and + * track whenever those fail. If they fail, the activation is discarded and the + * failure is forwarded to the broker. However, it is not the job of the + * launcher to tell whether an activation succeeded. + * + * Long story short, this function triggers an activation and tracks the status + * of the activation until it fails. If it fails, the information is forwarded + * to the broker and the activation is discarded. If it does not fail, it + * continues tracking the activation until the entire service object is removed + * (see service_remove()). + * + * In all cases this does not mean that there is a matching activation object + * in the broker. The broker activation can have a completely different + * lifetime than this activation in the launcher. + * + * Returns: 0 on success, negative error code on failure. + */ +int service_activate(Service *service, uint64_t serial) { int r; + service_discard_activation(service); + service->last_serial = serial; + if (!strcmp(service->name, "org.freedesktop.systemd1")) { /* * systemd activation requests are silently ignored. @@ -534,15 +835,33 @@ if (service->unit) { r = service_start_unit(service); - if (r) - return error_trace(r); + if (error_trace(r)) + goto error; } else if (service->argc > 0) { r = service_start_transient_unit(service); - if (r) - return error_trace(r); + if (error_trace(r)) + goto error; + } else { + /* + * If no unit-file, nor any command-line is specified, we + * expect the service to self-activate. This is an extension + * over the reference-implementation, which refuses to load + * such service files. + * However, this is very handy for services like PID1, or other + * auto-start services, which we know will appear on the bus at + * some point, but don't need to be triggered. They can now + * provide service-files and be available on the bus right from + * the beginning, without requiring activation from us. + * Technically, you could achieve the same with `/bin/true` as + * command, but being explicit is always preferred. + */ } return 0; + +error: + service_discard_activation(service); + return error_trace(r); } int service_add(Service *service) { @@ -583,6 +902,8 @@ if (!service->running) return 0; + service_discard_activation(service); + r = asprintf(&object_path, "/org/bus1/DBus/Name/%s", service->id); if (r < 0) return error_origin(-ENOMEM); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/launch/service.h new/dbus-broker-27/src/launch/service.h --- old/dbus-broker-25/src/launch/service.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/launch/service.h 2021-02-08 13:49:11.695880200 +0100 @@ -17,7 +17,9 @@ bool not_found; bool running; bool reload_tag; - sd_bus_slot *slot; + sd_bus_slot *slot_watch_unit; + sd_bus_slot *slot_query_unit; + sd_bus_slot *slot_start_unit; CRBNode rb; CRBNode rb_by_name; char *path; @@ -30,6 +32,7 @@ uint64_t instance; uint64_t n_missing_unit; uint64_t n_masked_unit; + uint64_t last_serial; char id[]; }; @@ -54,5 +57,5 @@ int service_compare_by_name(CRBTree *t, void *k, CRBNode *n); int service_add(Service *service); -int service_activate(Service *service); +int service_activate(Service *service, uint64_t serial); int service_remove(Service *service); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/util/dispatch.c new/dbus-broker-27/src/util/dispatch.c --- old/dbus-broker-25/src/util/dispatch.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/util/dispatch.c 2021-02-04 15:01:53.783595800 +0100 @@ -165,7 +165,7 @@ * * Once you handled an event fully, you must clear it via dispatch_file_clear() * to tell the dispatcher that you should only be invoked for the event - * when the kernel signalls it again. + * when the kernel signals it again. */ void dispatch_file_select(DispatchFile *file, uint32_t mask) { c_assert(!(mask & ~file->kernel_mask)); diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/util/log.c new/dbus-broker-27/src/util/log.c --- old/dbus-broker-25/src/util/log.c 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/util/log.c 2021-02-04 15:01:53.786929100 +0100 @@ -87,8 +87,8 @@ #include "util/log.h" #include "util/syscall.h" -/* lets retrict log records to 256MiB */ -#define LOG_SIZE_MAX (256ULL * 1024ULL * 1024ULL) +/* lets retrict log records to 2MiB */ +#define LOG_SIZE_MAX (2ULL * 1024ULL * 1024ULL) /* warning that is sent on first dropped log message */ #define LOG_WARNING_DROPPED "<3>Log messages dropped\n" @@ -104,6 +104,8 @@ *log = (Log)LOG_NULL; log->mode = LOG_MODE_NONE; log->consumed = false; + log->map_size = LOG_SIZE_MAX; + /* NOTE: Other log_init_*() variants override these. */ } /** @@ -117,7 +119,8 @@ */ void log_init_stderr(Log *log, int stderr_fd) { c_assert(stderr_fd >= 0); - *log = (Log)LOG_NULL; + + log_init(log); log->log_fd = stderr_fd; log->mode = LOG_MODE_STDERR; log->consumed = false; @@ -133,7 +136,8 @@ */ void log_init_journal(Log *log, int journal_fd) { c_assert(journal_fd >= 0); - *log = (Log)LOG_NULL; + + log_init(log); log->log_fd = journal_fd; log->mode = LOG_MODE_JOURNAL; log->consumed = false; @@ -149,7 +153,8 @@ */ void log_init_journal_consume(Log *log, int journal_fd) { c_assert(journal_fd >= 0); - *log = (Log)LOG_NULL; + + log_init(log); log->log_fd = journal_fd; log->mode = LOG_MODE_JOURNAL; log->consumed = true; @@ -166,7 +171,7 @@ */ void log_deinit(Log *log) { if (log->map != MAP_FAILED) - munmap(log->map, LOG_SIZE_MAX); + munmap(log->map, log->map_size); c_close(log->mem_fd); if (log->consumed) c_close(log->log_fd); @@ -213,7 +218,7 @@ if (log->error) return false; - if (log->mem_fd >= 0) + if (log->map != MAP_FAILED) return true; c_assert(!log->offset); @@ -225,20 +230,35 @@ */ mem_fd = syscall_memfd_create("dbus-broker-log", 0x3); if (mem_fd < 0) { - log->error = error_origin(-errno); - return false; - } + mem_fd = -1; - r = ftruncate(mem_fd, LOG_SIZE_MAX); - if (r < 0) { - log->error = error_origin(-errno); - return false; - } + /* + * In case of EINVAL, memfd_create() is not available. We then + * use normal anonymous memory as backing. + */ - p = mmap(NULL, LOG_SIZE_MAX, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, 0); - if (p == MAP_FAILED) { - log->error = error_origin(-errno); - return false; + if (errno != EINVAL) { + log->error = error_origin(-errno); + return false; + } + + p = mmap(NULL, log->map_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (p == MAP_FAILED) { + log->error = error_origin(-errno); + return false; + } + } else { + r = ftruncate(mem_fd, log->map_size); + if (r < 0) { + log->error = error_origin(-errno); + return false; + } + + p = mmap(NULL, log->map_size, PROT_READ | PROT_WRITE, MAP_SHARED, mem_fd, 0); + if (p == MAP_FAILED) { + log->error = error_origin(-errno); + return false; + } } log->mem_fd = mem_fd; @@ -401,12 +421,12 @@ * debugging is enabled in some way. */ - if (log->lossy) + if (log->lossy || log->mem_fd < 0) goto out_drop; mfd = log->mem_fd; log->mem_fd = -1; - munmap(log->map, LOG_SIZE_MAX); + munmap(log->map, log->map_size); log->map = MAP_FAILED; r = ftruncate(mfd, log->offset); @@ -565,7 +585,7 @@ if (!n_data || !log_alloc(log)) return; - if (LOG_SIZE_MAX - log->offset < n_data) { + if (log->map_size - log->offset < n_data) { log->error = LOG_E_TRUNCATED; return; } @@ -590,10 +610,10 @@ return; r = vsnprintf(log->map + log->offset, - LOG_SIZE_MAX - log->offset, + log->map_size - log->offset, format, args); - if (r < 0 || r >= (ssize_t)(LOG_SIZE_MAX - log->offset)) { + if (r < 0 || r >= (ssize_t)(log->map_size - log->offset)) { log->error = LOG_E_TRUNCATED; return; } diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/src/util/log.h new/dbus-broker-27/src/util/log.h --- old/dbus-broker-25/src/util/log.h 2020-12-03 11:53:50.000000000 +0100 +++ new/dbus-broker-27/src/util/log.h 2021-02-04 15:01:53.786929100 +0100 @@ -36,6 +36,7 @@ int mem_fd; void *map; + size_t map_size; size_t offset; }; diff -urN '--exclude=CVS' '--exclude=.cvsignore' '--exclude=.svn' '--exclude=.svnignore' old/dbus-broker-25/subprojects/c-list/src/c-list.h new/dbus-broker-27/subprojects/c-list/src/c-list.h --- old/dbus-broker-25/subprojects/c-list/src/c-list.h 2020-04-16 12:28:38.000000000 +0200 +++ new/dbus-broker-27/subprojects/c-list/src/c-list.h 2021-02-04 16:14:18.166463100 +0100 @@ -87,10 +87,12 @@ * c_list_is_empty() - check whether a list is empty * @list: list to check, or NULL * + * This is the same as !c_list_is_linked(). + * * Return: True if @list is empty, false if not. */ static inline _Bool c_list_is_empty(const CList *list) { - return !list || !c_list_is_linked(list); + return !c_list_is_linked(list); } /**
