Re: [systemd-devel] [PATCH] Propagate reload from RELOADING=1 notifications

2015-02-02 Thread Lennart Poettering 
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

2015-02-02 Thread Lennart Poettering 
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

2015-02-02 Thread Lennart Poettering 
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

2015-01-01 Thread Zbigniew Jędrzejewski-Szmek 
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

2015-01-01 Thread Jouke Witteveen 
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

2014-12-30 Thread Jouke Witteveen 
---

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