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

[Lennart Poettering]

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=.

> +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

-- 
Lennart Poettering, Red Hat