Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Tue, 30.12.14 20:22, Jouke Witteveen (j.wittev...@gmail.com) wrote: Hmm, what happened to this patch? Did you ever resend it with the commit msg fix Zbigniew suggested? I don't see it in the mail archives? --- This fixes #87251 src/core/manager.c | 42 ++ src/core/manager.h | 1 + src/core/service.c | 7 +++ 3 files changed, 50 insertions(+) diff --git a/src/core/manager.c b/src/core/manager.c index 9705e64..11cca17 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1226,6 +1226,48 @@ int manager_add_job_by_name(Manager *m, JobType type, const char *name, JobMode return manager_add_job(m, type, unit, mode, override, e, _ret); } +int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool override, sd_bus_error *e) { +int r; +Transaction *tr; +Iterator i; +Unit *dep; + + Spurious newline... Otherwise looks great! Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Thu, 01.01.15 19:15, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: On Tue, Dec 30, 2014 at 08:22:27PM +0100, Jouke Witteveen wrote: --- This fixes #87251 This is actually important information that should be included in the commit message (i.e. above not below ---). We usually include the full url, since we also use distribution bug trackers and having the full url makes things easier to click. In this case: https://bugs.freedesktop.org/show_bug.cgi?id=87251 static void service_enter_reload_by_notify(Service *s) { +_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; +int r; + assert(s); if (s-timeout_start_usec 0) service_arm_timer(s, s-timeout_start_usec); +r = manager_propagate_reload(UNIT(s)-manager, UNIT(s), JOB_REPLACE, false, error); +if(r 0) +log_unit_warning(UNIT(s)-id, %s failed to schedule propagation of reload: %s, UNIT(s)-id, bus_error_message(error, -r)); + Let's say that a.service has PropagateReloadsTo=b.service, and a.service provides the RELOADING=1 notification during a reload. What happens if a reload is requested with 'systemctl reload a', and systemd schedules a reload of a and b. Is it possible for b to be reloaded a second time as a result of notification of a? This should not happen, have you verified that this will not happen? THis is a real issue I fear. If service units use an asynchronous command in ExecReload= (such as one that just sends a SIGHUP signal to the daemon and terminates), then the daemon might send its RELOADING=1 later than the ExecReload= finishes. This would then trigger a second reload in b.service... This problem goes away if the people use synchronous commands in ExecReload= however, that wait for the reload to complete. In that case the RELOADING=1 from the service would be queued in s-notify_state, and be unset again when the daemon reports READY=1 again. And only after that the ExecReload= process would exit, and hence no second reload be propagated. (this is race-free because we always process notification messages before SIGCHLD). Now I wonder what to do with this. I think the change would be welcome, but I wonder if we can simply require people who use PropagateReloadsTo= to define synchronous ExecReload= processes or accept double reloads to be forwarded... I leaning towards merging the patch, but this would require an explanation in the man page of sd_notify() and in systemd.unit near the description of PropagateReloadsTo=. Jouke, could you resend this patch with such an addition? Thanks, Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Tue, 06.01.15 16:33, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: Isn't that just bad behavior? Sending a RELOADING=1 notification after a reload is initiated? I guess if both service_enter_reload() and service_enter_reload_by_notify() are called it is justified to propagate two reloads. Before testing it might be nice to know what we want :-). Hm. Looking at the code, sending periodic RELOADING=1 notifications can be used to sidestep the timeouts. This does not look OK, but maybe I'm misreading the code. I think you are reading the code right. Not sure though whether that should be considered a bug though... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Tue, Dec 30, 2014 at 08:22:27PM +0100, Jouke Witteveen wrote: --- This fixes #87251 This is actually important information that should be included in the commit message (i.e. above not below ---). We usually include the full url, since we also use distribution bug trackers and having the full url makes things easier to click. In this case: https://bugs.freedesktop.org/show_bug.cgi?id=87251 static void service_enter_reload_by_notify(Service *s) { +_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; +int r; + assert(s); if (s-timeout_start_usec 0) service_arm_timer(s, s-timeout_start_usec); +r = manager_propagate_reload(UNIT(s)-manager, UNIT(s), JOB_REPLACE, false, error); +if(r 0) +log_unit_warning(UNIT(s)-id, %s failed to schedule propagation of reload: %s, UNIT(s)-id, bus_error_message(error, -r)); + Let's say that a.service has PropagateReloadsTo=b.service, and a.service provides the RELOADING=1 notification during a reload. What happens if a reload is requested with 'systemctl reload a', and systemd schedules a reload of a and b. Is it possible for b to be reloaded a second time as a result of notification of a? This should not happen, have you verified that this will not happen? Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
On Thu, Jan 1, 2015 at 7:15 PM, Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl wrote: On Tue, Dec 30, 2014 at 08:22:27PM +0100, Jouke Witteveen wrote: --- This fixes #87251 This is actually important information that should be included in the commit message (i.e. above not below ---). We usually include the full url, since we also use distribution bug trackers and having the full url makes things easier to click. In this case: https://bugs.freedesktop.org/show_bug.cgi?id=87251 Will do. For consistency it is also better to not abort propagation if one unit fails to reload, so I have a second version ready already. static void service_enter_reload_by_notify(Service *s) { +_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; +int r; + assert(s); if (s-timeout_start_usec 0) service_arm_timer(s, s-timeout_start_usec); +r = manager_propagate_reload(UNIT(s)-manager, UNIT(s), JOB_REPLACE, false, error); +if(r 0) +log_unit_warning(UNIT(s)-id, %s failed to schedule propagation of reload: %s, UNIT(s)-id, bus_error_message(error, -r)); + Let's say that a.service has PropagateReloadsTo=b.service, and a.service provides the RELOADING=1 notification during a reload. What happens if a reload is requested with 'systemctl reload a', and systemd schedules a reload of a and b. Is it possible for b to be reloaded a second time as a result of notification of a? This should not happen, have you verified that this will not happen? Isn't that just bad behavior? Sending a RELOADING=1 notification after a reload is initiated? I guess if both service_enter_reload() and service_enter_reload_by_notify() are called it is justified to propagate two reloads. Before testing it might be nice to know what we want :-). Regards, - Jouke ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications
--- This fixes #87251 src/core/manager.c | 42 ++ src/core/manager.h | 1 + src/core/service.c | 7 +++ 3 files changed, 50 insertions(+) diff --git a/src/core/manager.c b/src/core/manager.c index 9705e64..11cca17 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1226,6 +1226,48 @@ int manager_add_job_by_name(Manager *m, JobType type, const char *name, JobMode return manager_add_job(m, type, unit, mode, override, e, _ret); } +int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool override, sd_bus_error *e) { +int r; +Transaction *tr; +Iterator i; +Unit *dep; + + +assert(m); +assert(unit); +assert(mode _JOB_MODE_MAX); + +tr = transaction_new(mode == JOB_REPLACE_IRREVERSIBLY); +if (!tr) +return -ENOMEM; + +SET_FOREACH(dep, unit-dependencies[UNIT_PROPAGATES_RELOAD_TO], i) { +r = transaction_add_job_and_dependencies(tr, JOB_RELOAD, dep, NULL, false, override, false, false, mode == JOB_IGNORE_DEPENDENCIES, e); +if (r 0) { +log_unit_warning(dep-id, + Cannot add dependency reload job for unit %s, ignoring: %s, + dep-id, bus_error_message(e, r)); + +if (e) +sd_bus_error_free(e); + +goto tr_abort; +} +} + +r = transaction_activate(tr, m, mode, e); +if (r 0) +goto tr_abort; + +transaction_free(tr); +return 0; + +tr_abort: +transaction_abort(tr); +transaction_free(tr); +return r; +} + Job *manager_get_job(Manager *m, uint32_t id) { assert(m); diff --git a/src/core/manager.h b/src/core/manager.h index ab75f90..bc11f87 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -316,6 +316,7 @@ int manager_load_unit_from_dbus_path(Manager *m, const char *s, sd_bus_error *e, int manager_add_job(Manager *m, JobType type, Unit *unit, JobMode mode, bool force, sd_bus_error *e, Job **_ret); int manager_add_job_by_name(Manager *m, JobType type, const char *name, JobMode mode, bool force, sd_bus_error *e, Job **_ret); +int manager_propagate_reload(Manager *m, Unit *unit, JobMode mode, bool force, sd_bus_error *e); void manager_dump_units(Manager *s, FILE *f, const char *prefix); void manager_dump_jobs(Manager *s, FILE *f, const char *prefix); diff --git a/src/core/service.c b/src/core/service.c index bfbe959..71c7bf6 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1496,11 +1496,18 @@ fail: } static void service_enter_reload_by_notify(Service *s) { +_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; +int r; + assert(s); if (s-timeout_start_usec 0) service_arm_timer(s, s-timeout_start_usec); +r = manager_propagate_reload(UNIT(s)-manager, UNIT(s), JOB_REPLACE, false, error); +if(r 0) +log_unit_warning(UNIT(s)-id, %s failed to schedule propagation of reload: %s, UNIT(s)-id, bus_error_message(error, -r)); + service_set_state(s, SERVICE_RELOAD); } -- 2.2.1 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel