<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 2, 2015 at 1:54 PM, Lennart Poettering <span dir="ltr"><<a href="mailto:lennart@poettering.net" target="_blank">lennart@poettering.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, 22.03.15 13:36, Michael Olbrich (<a href="mailto:m.olbrich@pengutronix.de">m.olbrich@pengutronix.de</a>) wrote:<br>
<br>
Love this work!<br>
<span class=""><br>
>          init_autofs_dev_ioctl(&param);<br>
>          param.ioctlfd = ioctl_fd;<br>
> -        param.timeout.timeout = sec;<br>
> +<br>
> +        /* Convert to seconds, rounding up. */<br>
> +        param.timeout.timeout = usec / USEC_PER_SEC + (usec % USEC_PER_SEC > 0);<br>
<br>
</span>Please use "(usec + USEC_PER_SEC - 1) / USEC_PER_SEC".<br>
<span class=""><br>
><br>
> +int automount_update_mount(Automount *a, MountState old_state, MountState state)<br>
> +{<br>
<br>
</span>Please add opening bracket to the function declaration line before, like for all other<br>
constructs. We differ from kernel coding style in this regard.<br>
<span class=""><br>
> @@ -535,6 +585,100 @@ fail:<br>
>          automount_enter_dead(a, AUTOMOUNT_FAILURE_RESOURCES);<br>
>  }<br>
><br>
> +static int automount_start_expire(Automount *a);<br>
> +<br>
> +struct expire_data {<br>
> +        int dev_autofs_fd;<br>
> +        int ioctl_fd;<br>
> +};<br>
> +<br>
> +static inline void cleanup_expire(void *p) {<br>
> +        struct expire_data *data = *(struct expire_data**)p;<br>
> +<br>
> +        if (!data)<br>
> +                return;<br>
> +<br>
> +        safe_close(data->dev_autofs_fd);<br>
> +        safe_close(data->ioctl_fd);<br>
> +        free(data);<br>
> +}<br>
> +#define _cleanup_expire_ _cleanup_(cleanup_expire)<br>
<br>
</span>No need to define this. We only define this for commonly used<br>
structs. Simply use _cleanup_() directly...<br>
<span class=""><br>
> +<br>
> +static void *expire_thread(void *p) {<br>
> +        struct autofs_dev_ioctl param;<br>
> +        _cleanup_expire_ struct expire_data *data = (struct expire_data*)p;<br>
> +        int r;<br>
> +<br>
> +        assert(data->dev_autofs_fd >= 0);<br>
> +        assert(data->ioctl_fd >= 0);<br>
> +<br>
> +        init_autofs_dev_ioctl(&param);<br>
> +        param.ioctlfd = data->ioctl_fd;<br>
> +<br>
> +        do {<br>
> +                r = ioctl(data->dev_autofs_fd, AUTOFS_DEV_IOCTL_EXPIRE, &param);<br>
> +        } while (r == 0);<br>
> +<br>
> +        if (errno != EAGAIN)<br>
> +                log_error_errno(errno, "failed to expire automount:<br>
> %m");<br>
<br>
</span>Please uppercase the first word of the message.<br>
<br>
Also, given that we don't actually do anything about the failure, i<br>
think we should downgrade this to a warning, and clarify that we<br>
ignore this, by suffixing the message with ", ignoring" or so... Like so:<br>
<br>
        log_warning_errno(errno, "Failed to expire automount, ignoring: %m");<br>
<span class=""><br></span></blockquote><div><br></div><div>Isn't log_warning_errno(errno, "%m") identical to just log_warning("%m")?</div><div><br></div></div>-- <br><div class="gmail_signature"><div dir="ltr">Mantas Mikulėnas <<a href="mailto:grawity@gmail.com" target="_blank">grawity@gmail.com</a>></div></div>
</div></div>