[systemd-devel] [PATCH 2/3] timer: timer can be a transient unit

[WaLyong Cho]

On 10/23/2014 12:42 AM, Lennart Poettering wrote:
> On Tue, 07.10.14 14:20, WaLyong Cho (walyong.cho at samsung.com) wrote:
> 
>> +        assert(message);
>> +
>> +        if (streq(name, "OnActiveSec") ||
>> +            streq(name, "OnBootSec") ||
>> +            streq(name, "OnStartupSec") ||
>> +            streq(name, "OnUnitActiveSec") ||
>> +            streq(name, "OnUnitInactiveSec")) {
> 
> I think it would be cool to use STR_IN_SET() here.
> 
Ok, I will do on next patch.

>> +
>> +                TimerValue *v;
>> +                TimerBase b = _TIMER_BASE_INVALID;
>> +                usec_t u = 0;
>> +                CalendarSpec *c = NULL;
>> +
>> +                b = timer_base_from_string(name);
>> +                if (b < 0)
>> +                        return 0;
>> +
>> +                r = sd_bus_message_read(message, "s", &str);
>> +                if (r < 0)
>> +                        return r;
>> +
>> +                if (mode != UNIT_CHECK) {
>> +                        if (b == TIMER_CALENDAR) {
> 
> Hmm, there's something wrong here, b can never be TIMER_CALENDAR as
> we never enter this if block for name == "OnCalendar".
> 
Yes, right. I will add "OnCalendar" to above condition check.

>> +
>> +        } else if (streq(name, "AccuracySec")) {
>> +
>> +                usec_t u = 0;
>> +
>> +                r = sd_bus_message_read(message, "s", &str);
>> +                if (r < 0)
>> +                        return r;
> 
> No. The accuracy should be of type "t", and be called
> "AccuracyUSec". There's a slight asymmetry between dbus interfaces and
> the config files here, as the bus interfaces always expose usec as
> uint64_t, while the config files default to sec as unit...
> 
Yes, right. I think we talked already about this. If systemd-run send
parsed value then this also should be not string.

>>  
>> +        /* If trigger unit is transient, make sure be gabage
>> +         * collected. That maybe have no_gc to wait until this timer
>> +         * is started. */
>> +        if (UNIT_TRIGGER(u)->transient)
>> +                UNIT_TRIGGER(u)->no_gc = false;
>> +
> 
> No, this should be unnecessary?
> 
> timer units should just follow the normal GC logic, and get GC'ed as
> soon as they are not running or referenced anymore.
> 
If we only use StartTransientUnit then NOT necessary. This was added for
keeping the generated transient service by NewTransientUnit what only
exist on my previous patch. Some more detail, on previous my patch,
there are two method calls. At the first, NewTransientUnit is sent for
transient service. And the second, StartTransientUnit is sent for
transient timer. The transient service was cleared by GC because there
is no jobs and no dependencies on there. The transient service has
dependency after the transient timer is loaded.

Now we will only use StartTransientUnit so maybe this kind of hack will
not be needed.

Thanks,

WaLyong

> Lennart
>