[systemd-devel] [PATCH] automount: add expire support

[Michael Olbrich]

On Thu, Apr 02, 2015 at 12:54:00PM +0200, Lennart Poettering wrote:
> On Sun, 22.03.15 13:36, Michael Olbrich (m.olbrich at pengutronix.de) wrote:
> 
> Love this work!

Thanks.

[...]
> > +
> > +        if (a->expire_event_source) {
> > +                r = sd_event_source_set_time(a->expire_event_source, now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC);
> > +                if (r < 0)
> > +                        return r;
> > +
> > +                return sd_event_source_set_enabled(a->expire_event_source, SD_EVENT_ONESHOT);
> > +        }
> > +
> > +        return sd_event_add_time(
> > +                        UNIT(a)->manager->event,
> > +                        &a->expire_event_source,
> > +                        CLOCK_MONOTONIC,
> > +                        now(CLOCK_MONOTONIC) + 5 * USEC_PER_SEC, 0,
> > +                        automount_dispatch_expire, a);
> > +}
> 
> Maybe the 5 * USEC_PER_SEC should become a #define somewhere at the
> top of the file? Given that this is used more than once it might be
> advisable to use the same definition.

I've been thinking about this again and I changed this. Checking every 5
Seconds is rather often for longer timeouts. I've changed it to
MAX(a->timeout_idle_usec/10, USEC_PER_SEC). I think that's a good
compromise.

> > +
> > +                r = set_ensure_allocated(&a->expire_tokens, NULL);
> > +                if (r < 0) {
> > +                        log_unit_error(UNIT(a)->id, "Failed to
> > allocate token set.");
> 
> Sounds like a job for log_oom().

It's basically a copy of the code right above it, so it should match. I
think that should be a separate patch.

> > +                        goto fail;
> > +                }
> > +
> > +                r = set_put(a->expire_tokens, UINT_TO_PTR(packet.v5_packet.wait_queue_token));
> > +                if (r < 0) {
> > +                        log_unit_error_errno(UNIT(a)->id, r, "Failed to remember token: %m");
> > +                        goto fail;
> > +                }
> > +                r = manager_add_job(UNIT(a)->manager, JOB_STOP, UNIT_TRIGGER(UNIT(a)),
> > +                                    JOB_REPLACE, true, &error,
> > NULL);
> 
> We normally don't like break this eagerly.

Actually we do. In this file at least. The manager_add_job for JOB_START
has the same line break :-). But I don't care either way, so I changed it.

> > +                return 0;
> > +        }
> > +
> > +        fprintf(f,
> > +                "TimeoutIdleSec=" USEC_FMT "\n",
> > +                u / USEC_PER_SEC);
> > +
> > +        return 0;
> 
> Why not format that using format_timespan() here, so that the accuracy
> is not lost? (We'll lose it when passing it to the kernel ultimately,
> but we shouldn't lose it any earlier. In particular since you round up
> when passing it to the kernel, but are rounding down here... Hence,
> let's keep this simple, and always pass our native accuracy along
> until the last point where we really have to convert it.

Sounds reasonable, so I changed it. I basically copied this from
the x-systemd.device-timeout implementation, so you might want to change
that as well.

I've changed everything else as requested. New patch follows.

Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |