Re: [systemd-devel] [PATCH v6 3/3] run: introduce timer support option

[Lennart Poettering]

On Tue, 09.12.14 01:13, WaLyong Cho (walyong@samsung.com) wrote:
> > What's the rationale for checking the unit state here? I mean, PID 1
> > should fail anyway in this case, do we realy need to check this name
> > in advance? I mean, such a check is necessarily racy, since the unit
> > might appear while between our client-side check and the actual
> > execution of the service, no?
> 
> I had plan to generate transient timer unit for already existing
> service. 

This might make sense to add one day, but I'd really leave that out
for now.

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 v6 3/3] run: introduce timer support option

[WaLyong Cho]

On 12/09/2014 12:38 AM, Lennart Poettering wrote:
> On Tue, 09.12.14 00:03, WaLyong Cho (walyong@samsung.com) wrote:
> 
>>  } else {
>>  log_error("Unknown assignment %s.", assignment);
>>  return -EINVAL;
>> diff --git a/src/run/run.c b/src/run/run.c
>> index 85eb052..e145784 100644
>> --- a/src/run/run.c
>> +++ b/src/run/run.c
>> @@ -30,6 +30,7 @@
>>  #include "env-util.h"
>>  #include "path-util.h"
>>  #include "bus-error.h"
>> +#include "calendarspec.h"
>>  
>>  static bool arg_scope = false;
>>  static bool arg_remain_after_exit = false;
>> @@ -47,27 +48,45 @@ static int arg_nice = 0;
>>  static bool arg_nice_set = false;
>>  static char **arg_environment = NULL;
>>  static char **arg_property = NULL;
>> +static bool with_timer = false;
> 
> Hmm, maybe we should turn this boolean into a function? I mean, static
> variables are something we should keep at a minimum. We do them for
> command line arguments, since they are kinda global anyway, but I
> think we should otherwise avoid them, especially if they are
> effectively redundant anyway.
> 
> Hence, my suggestions would be to turn this into a call:
> 
> static bool with_timer(void) {
> return arg_on_active || arg_on_boot || arg_on_startup || 
> arg_on_unit_active || arg_on_unit_inactive || arg_on_calendar;
> }
> 
> Or so?

Yes, it should be.
> 
>> +static int start_transient_service(
>> +sd_bus *bus,
>> +char **argv,
>> +sd_bus_error *error) {
>> +
>> +_cleanup_bus_message_unref_ sd_bus_message *m = NULL;
>> +_cleanup_free_ char *service = NULL;
>> +char *state = NULL;
>> +int r;
>> +
>> +assert(bus);
>> +assert(argv);
>> +
>> +if (arg_unit) {
>> +service = unit_name_mangle_with_suffix(arg_unit, 
>> MANGLE_NOGLOB, ".service");
>> +if (!service)
>> +return log_oom();
>> +
>> +if (get_unit_state_by_name(bus, service, &state) == 0) {
>> +log_error("Unit %s is already exist with %s 
>> state.", service, state);
>> +return -EEXIST;
>> +}
> 
> What's the rationale for checking the unit state here? I mean, PID 1
> should fail anyway in this case, do we realy need to check this name
> in advance? I mean, such a check is necessarily racy, since the unit
> might appear while between our client-side check and the actual
> execution of the service, no?

I had plan to generate transient timer unit for already existing
service. To do that I needed get_unit_state_by_name() and I used that
also here.

with_aux = get_unit_state_by_name(bus, service, &state)
< 0 ? true : false;

Above is to determine that. Honestly, some of codes are removed during
rebase. I will rework for that.

Anyway, as you said, it is role of server side not client. I will remove
on here.

WaLyong

> 
> Lennart
> 
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v6 3/3] run: introduce timer support option

[Lennart Poettering]

On Tue, 09.12.14 00:03, WaLyong Cho (walyong@samsung.com) wrote:

>  } else {
>  log_error("Unknown assignment %s.", assignment);
>  return -EINVAL;
> diff --git a/src/run/run.c b/src/run/run.c
> index 85eb052..e145784 100644
> --- a/src/run/run.c
> +++ b/src/run/run.c
> @@ -30,6 +30,7 @@
>  #include "env-util.h"
>  #include "path-util.h"
>  #include "bus-error.h"
> +#include "calendarspec.h"
>  
>  static bool arg_scope = false;
>  static bool arg_remain_after_exit = false;
> @@ -47,27 +48,45 @@ static int arg_nice = 0;
>  static bool arg_nice_set = false;
>  static char **arg_environment = NULL;
>  static char **arg_property = NULL;
> +static bool with_timer = false;

Hmm, maybe we should turn this boolean into a function? I mean, static
variables are something we should keep at a minimum. We do them for
command line arguments, since they are kinda global anyway, but I
think we should otherwise avoid them, especially if they are
effectively redundant anyway.

Hence, my suggestions would be to turn this into a call:

static bool with_timer(void) {
return arg_on_active || arg_on_boot || arg_on_startup || 
arg_on_unit_active || arg_on_unit_inactive || arg_on_calendar;
}

Or so?

> +static int start_transient_service(
> +sd_bus *bus,
> +char **argv,
> +sd_bus_error *error) {
> +
> +_cleanup_bus_message_unref_ sd_bus_message *m = NULL;
> +_cleanup_free_ char *service = NULL;
> +char *state = NULL;
> +int r;
> +
> +assert(bus);
> +assert(argv);
> +
> +if (arg_unit) {
> +service = unit_name_mangle_with_suffix(arg_unit, 
> MANGLE_NOGLOB, ".service");
> +if (!service)
> +return log_oom();
> +
> +if (get_unit_state_by_name(bus, service, &state) == 0) {
> +log_error("Unit %s is already exist with %s state.", 
> service, state);
> +return -EEXIST;
> +}

What's the rationale for checking the unit state here? I mean, PID 1
should fail anyway in this case, do we realy need to check this name
in advance? I mean, such a check is necessarily racy, since the unit
might appear while between our client-side check and the actual
execution of the service, no?

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH v6 3/3] run: introduce timer support option

[WaLyong Cho]

Support timer options --on-active=, --on-boot=, --on-startup=,
--on-unit-active=, --on-unit-inactive=, --on-calendar=. Each options
corresponding with OnActiveSec=, OnBootSec=, OnStartupSec=,
OnUnitActiveSec=, OnUnitInactiveSec=, OnCalendar= of timer
respectively. And OnCalendar= and WakeSystem= supported by
--timer-property= option like --property= of systemd-run.
---
 man/systemd-run.xml  |  64 
 src/libsystemd/sd-bus/bus-util.c |  14 +-
 src/run/run.c| 619 +++
 3 files changed, 580 insertions(+), 117 deletions(-)

diff --git a/man/systemd-run.xml b/man/systemd-run.xml
index 28a9878..30e7783 100644
--- a/man/systemd-run.xml
+++ b/man/systemd-run.xml
@@ -210,6 +210,54 @@ along with systemd; If not, see 
.
   
   
 
+  
+--on-active=
+--on-boot=
+--on-startup=
+--on-unit-active=
+--on-unit-inactive=
+
+Defines monotonic timers relative to different
+starting points. Also see OnActiveSec=,
+OnBootSec=,
+OnStartupSec=,
+OnUnitActiveSec= and
+OnUnitInactiveSec= in
+
systemd.timer5.
 This
+option has no effect in conjunction with
+--scope.
+
+  
+
+  
+--on-calendar=
+
+Defines realtime (i.e. wallclock) timers with
+calendar event expressions. Also see
+OnCalendar= in
+
systemd.timer5.
 This
+option has no effect in conjunction with
+--scope.
+
+  
+
+  
+--timer-property=
+
+Sets a timer unit property for the timer unit
+that is created. It is similar with
+--property but only for created timer
+unit. This option only has effect in conjunction with
+--on-active=, --on-boot=,
+--on-startup=,
+--on-unit-active=,
+--on-unit-inactive=,
+--on-calendar=. This takes an assignment in
+the same format as
+
systemctl1's
+set-property command. 
+  
+
   
   
 
@@ -250,6 +298,21 @@ Sep 08 07:37:21 bupkis env[19948]: 
BOOT_IMAGE=/vmlinuz-3.11.0-0.rc5.git6.2.fc20.
 property.
 
 # systemd-run -p BlockIOWeight=10 updatedb
+
+The following command will touch a file after 10 seconds.
+
+# date; systemd-run --on-active=30 
--timer-property=AccuracySec=100ms /bin/touch /tmp/foo
+Mon Dec  8 20:44:24 KST 2014
+Running as unit run-71.timer.
+Will run as unit run-71.service.
+# journalctl -b -u run-73.timer
+-- Logs begin at Fri 2014-12-05 19:09:21 KST, end at Mon 2014-12-08 20:44:54 
KST. --
+Dec 08 20:44:38 container systemd[1]: Starting /bin/touch /tmp/foo.
+Dec 08 20:44:38 container systemd[1]: Started /bin/touch /tmp/foo.
+# journalctl -b -u run-73.service
+-- Logs begin at Fri 2014-12-05 19:09:21 KST, end at Mon 2014-12-08 20:44:54 
KST. --
+Dec 08 20:44:48 container systemd[1]: Starting /bin/touch /tmp/foo...
+Dec 08 20:44:48 container systemd[1]: Started /bin/touch 
/tmp/foo.
   
 
   
@@ -263,6 +326,7 @@ Sep 08 07:37:21 bupkis env[19948]: 
BOOT_IMAGE=/vmlinuz-3.11.0-0.rc5.git6.2.fc20.
   
systemd.slice5,
   
systemd.exec5,
   
systemd.resource-control5,
+  
systemd.timer5,
   
machinectl1
 
   
diff --git a/src/libsystemd/sd-bus/bus-util.c b/src/libsystemd/sd-bus/bus-util.c
index bdaa449..0f1a89c 100644
--- a/src/libsystemd/sd-bus/bus-util.c
+++ b/src/libsystemd/sd-bus/bus-util.c
@@ -1372,7 +1372,8 @@ int bus_append_unit_property_assignment(sd_bus_message 
*m, const char *assignmen
 
 if (STR_IN_SET(field,
"CPUAccounting", "MemoryAccounting", 
"BlockIOAccounting",
-   "SendSIGHUP", "SendSIGKILL")) {
+   "SendSIGHUP", "SendSIGKILL",
+   "WakeSystem")) {
 
 r = parse_boolean(eq);
 if (r < 0) {
@@ -1533,6 +1534,17 @@ int bus_append_unit_property_assignment(sd_bus_message 
*m, const char *assignmen
 
 r = sd_bus_message_append(m, "v", "i", sig);
 
+} else if (streq(field, "AccuracySec")) {
+usec_t u;
+
+r = parse_sec(eq, &u);
+if (r < 0) {
+log_error("Failed to parse %s value %s", field, eq);
+return -EINVAL;
+}
+
+r = sd_bus_message_append(m, "v", "t", u);
+
 } else {
 log_error("Unknown assignment %s.", assignment);
 return -EINVAL;
diff --git a/src/run/run.c b/src/run/run.c
index 85eb052..e145784 100644
--- a/src/run/run.c
+++ b/src/run/run.c
@@ -30,6 +30,7 @@
 #include "env-util.h"
 #include "path-util.h"
 #include "bus-error.h"
+#include "calendarspec.h"
 
 static bool arg_scope = false;
 static bool arg_remain_after_exit = false;
@@ -47,27 +48,45 @@ static int arg_nice = 0;
 static bool arg_nice_set = false;
 static char **arg_environment = NULL;