[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).
> 
> 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=.
> 
Rework is almost done. Now I'm testing. But the AccuracySec= and
WakeSystem= options are hard to deal with --property= option. Currently
the --property= option of systemd-run is designed for service or scope
unit. If that option is specified then given string is parsed again by
bus_append_unit_property_assignment(). We can AccuracySec=/WakeSystem=
option to there. But we have to split given properties(by --property
option) according to which unit can have that properties.

So I'd like to keep those two option as separated systemd-run options.

If you agree, I will send a patch soon.

WaLyong

>> +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. 
> 
> (I'll leave the rest unreviewed for now, please rework this first to
> make use of the fourth StartTransientUnit() bus call argument!)
> 
> Thanks,
> 
> Lennart
>