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

[Lennart Poettering]

On Sun, 22.03.15 13:36, Michael Olbrich (m.olbrich at pengutronix.de) wrote:

Love this work!

>          init_autofs_dev_ioctl(&param);
>          param.ioctlfd = ioctl_fd;
> -        param.timeout.timeout = sec;
> +
> +        /* Convert to seconds, rounding up. */
> +        param.timeout.timeout = usec / USEC_PER_SEC + (usec % USEC_PER_SEC > 0);

Please use "(usec + USEC_PER_SEC - 1) / USEC_PER_SEC".

>  
> +int automount_update_mount(Automount *a, MountState old_state, MountState state)
> +{

Please add opening bracket to the function declaration line before, like for all other
constructs. We differ from kernel coding style in this regard.

> @@ -535,6 +585,100 @@ fail:
>          automount_enter_dead(a, AUTOMOUNT_FAILURE_RESOURCES);
>  }
>  
> +static int automount_start_expire(Automount *a);
> +
> +struct expire_data {
> +        int dev_autofs_fd;
> +        int ioctl_fd;
> +};
> +
> +static inline void cleanup_expire(void *p) {
> +        struct expire_data *data = *(struct expire_data**)p;
> +
> +        if (!data)
> +                return;
> +
> +        safe_close(data->dev_autofs_fd);
> +        safe_close(data->ioctl_fd);
> +        free(data);
> +}
> +#define _cleanup_expire_ _cleanup_(cleanup_expire)

No need to define this. We only define this for commonly used
structs. Simply use _cleanup_() directly...

> +
> +static void *expire_thread(void *p) {
> +        struct autofs_dev_ioctl param;
> +        _cleanup_expire_ struct expire_data *data = (struct expire_data*)p;
> +        int r;
> +
> +        assert(data->dev_autofs_fd >= 0);
> +        assert(data->ioctl_fd >= 0);
> +
> +        init_autofs_dev_ioctl(&param);
> +        param.ioctlfd = data->ioctl_fd;
> +
> +        do {
> +                r = ioctl(data->dev_autofs_fd, AUTOFS_DEV_IOCTL_EXPIRE, &param);
> +        } while (r == 0);
> +
> +        if (errno != EAGAIN)
> +                log_error_errno(errno, "failed to expire automount:
> %m");

Please uppercase the first word of the message.

Also, given that we don't actually do anything about the failure, i
think we should downgrade this to a warning, and clarify that we
ignore this, by suffixing the message with ", ignoring" or so... Like so:

        log_warning_errno(errno, "Failed to expire automount, ignoring: %m");

> +
> +        return NULL;
> +}
> +
> +static int automount_dispatch_expire(sd_event_source *source, usec_t usec, void *userdata) {
> +        Automount *a = AUTOMOUNT(userdata);
> +        _cleanup_expire_ struct expire_data *data = NULL;
> +        int r;
> +
> +        assert(a);
> +        assert(source == a->expire_event_source);
> +
> +        data = malloc0(sizeof(struct expire_data));

Please use "new0(struct expire data, 1)" for this.

> +        if (!data)
> +                return -ENOMEM;
> +
> +        data->ioctl_fd = -1;
> +
> +        data->dev_autofs_fd = dup(UNIT(a)->manager->dev_autofs_fd);

In general I think we should avoid dup(), and also dup2(), and use
fcntl(fd, F_DUPFD_CLOEXEC, 3) instead. First, you want the O_CLOEXEC for
this. Secondly, it's good to avoid the risk the dupped fd becomes 0,
1, 2 in the unlikely case they are closed...

I also updated CODING_STYLE now to clarify this.

> +        if (data->dev_autofs_fd < 0)
> +                return data->dev_autofs_fd;

You probably want to return -errno here?

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

> +        case autofs_ptype_expire_direct:
> +                log_unit_debug(UNIT(a)->id, "Got direct umount request on %s", a->where);
> +
> +                sd_event_source_set_enabled(a->expire_event_source,
> SD_EVENT_OFF);

Maybe cast the result to (void) explicitly, to clarify that we
knowingly ignore the return value.

> +
> +                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().

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

> diff --git a/src/core/automount.h b/src/core/automount.h
> index 60f55223897e..2a50fef68d01 100644
> --- a/src/core/automount.h
> +++ b/src/core/automount.h
> @@ -47,6 +47,7 @@ struct Automount {
>          AutomountState state, deserialized_state;
>  
>          char *where;
> +        usec_t timeout_idle_usec;
>  
>          int pipe_fd;
>          sd_event_source *pipe_event_source;
> @@ -54,13 +55,16 @@ struct Automount {
>          dev_t dev_id;
>  
>          Set *tokens;
> +        Set *expire_tokens;
> +
> +        sd_event_source *expire_event_source;

Unless I am missing something then you never free this event source
when the automount unit is freed?

>  
> +static int write_idle_timeout(FILE *f, const char *where, const char *opts, char **filtered) {
> +        _cleanup_free_ char *timeout = NULL;
> +        usec_t u;
> +        int r;
> +
> +        r = fstab_filter_options(opts, "x-systemd.idle-timeout\0",
> +                                 NULL, &timeout, filtered);
> +        if (r <= 0)
> +                return r;
> +
> +        r = parse_sec(timeout, &u);
> +        if (r < 0) {
> +                log_warning("Failed to parse timeout for %s, ignoring: %s",
> +                            where, timeout);

We normally don't line break so eagerly.

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

Looks great otherwise!

Lennart

-- 
Lennart Poettering, Red Hat