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

[WaLyong Cho]

On 10/23/2014 12:56 AM, Lennart Poettering wrote:
> On Tue, 07.10.14 14:20, WaLyong Cho (walyong.cho at samsung.com) wrote:
>
>> If systemd-run is called with timer option, then systemd-run call
>> NewTransientUnit with service unit. And also call StartTransientUnit
>> with timer unit which has same name with the service. So actually, two
>> method call is coming and two transient unit is generated. One is
>> service and the other is timer.
>>
>> Supported timer options are --after=, --after-boot=, --after-startup=,
>> --after-active=, --after-inactive=, --calendar=, --accuracy= and
>> --wake-system. Each option corresponding with OnActiveSec=,
>> OnBootSec=, OnStartupSec=, OnUnitActiveSec=, OnUnitInactiveSec=,
>> AccuracySec= and AccuracySec= of timer respectively.
>
> Hmm, I'd really like to stay close to the underlying props here in
> naming, and call the options "--on-boot=", "--on-inactive=" and so on
> (the -sec suffix we can probably drop).
>
I'd really tought and worried about naming of the options. At the first 
time, I'd made similar named option with timer unit. But I tought the 
command line options should be more meaningful or easy to understand. So 
I changed like this. But if you say so, I don't want to against to your 
opinion. I will change those.

> The AccuracySec= and WakeSystem= stuff I think we don't need to cover
> with a command line argument of its own, we can cover that with
> --property=.
>
Ah, good, I didn't know that. I will do like that.

>> +static bool with_timer = false;
>> +static char *arg_after = NULL;
>> +static char *arg_after_boot = NULL;
>> +static char *arg_after_startup = NULL;
>> +static char *arg_after_active = NULL;
>> +static char *arg_after_inactive = NULL;
>> +static char *arg_calendar = NULL;
>> +static char *arg_accuracy = NULL;
>> +static bool arg_wake_system = false;
>
> I'd just do one arg_on_timer usec_t variable to store most of the
> --on-xyz= values in. Plus one arg_on_calendar to hold the calendar
> expression. This should be much simpler than keeping one variable
> around for each.
>
>> +                case ARG_AFTER:
>> +                        r = parse_sec(optarg, &u);
>> +                        if (r < 0) {
>> +                                log_error("Failed to parse timer value: %s", optarg);
>> +                                return r;
>> +                        }
>> +                        arg_after = optarg;
>> +                        with_timer = true;
>> +                        break;
>
> This stuff really should not store the string, but the parsed value.
>
Yeah, a couple of parse_sec doesn't make sense.
How about calendar_spec_from_string? calendar_spec_from_string also be 
called twice on run and dbus-timer. I'm not sure the parsed calendar 
spec is able to be sent on current StartTransientUnit() interface. But 
even if possible, just string is obiously easy to send and receive. How 
do you think?

> (I'll leave the rest unreviewed for now, please rework this first to
> make use of the fourth StartTransientUnit() bus call argument!)
>
If you reply on above question, then I will send patch again soon.

Thanks,

WaLyong

> Thanks,
>
> Lennart
>