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

[Mantas Mikulėnas]

On Thu, Apr 2, 2015 at 1:54 PM, Lennart Poettering <lenn...@poettering.net>
wrote:

> On Sun, 22.03.15 13:36, Michael Olbrich (m.olbr...@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");
>
>
Isn't log_warning_errno(errno, "%m") identical to just log_warning("%m")?

-- 
Mantas Mikulėnas <graw...@gmail.com>

_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel