[systemd-devel] [PATCH v6 3/3] run: introduce timer support option
WaLyong Cho
walyong.cho at samsung.com
Mon Dec 8 08:13:12 PST 2014
On 12/09/2014 12:38 AM, Lennart Poettering wrote:
> On Tue, 09.12.14 00:03, WaLyong Cho (walyong.cho at 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
>
More information about the systemd-devel
mailing list