[systemd-devel] [systemd-commits] 3 commits - Makefile.am src/core src/fstab-generator units/initrd-cleanup.service units/initrd-fs-pre.target units/initrd-fs.target units/initrd-parse-etc.service units/initrd-switch-root.target

[Lennart Poettering]

On Mon, 04.03.13 10:43, Harald Hoyer (harald at kemper.freedesktop.org) wrote:

> +                if (initrd) {
> +                        char _cleanup_free_ *mu = NULL, *name = NULL;
> +                        /* Skip generation, if unit already exists */
> +                        name = unit_name_from_path(where, ".mount");
> +                        if (!name)
> +                                return log_oom();
> +                        mu = strjoin(arg_dest, "/", name, NULL);
> +                        if (!mu)
> +                                return log_oom();
> +
> +                        k = access(mu, R_OK);
> +                        if (k == 0)
> +                                continue;
> +                }

Ahum. I can't say I like invocations to access() like this one. There
are very few cases where access() is the right syscall to invoke, and I
am pretty sure this is not one of them, due to its inherent
raciness. There might be other generators running in parallel to this
one, so the existance of the mount unit might have already changed by
the time you then go on and actually create the unit file. So, your
test here doesn't work and is pointless.

Please folks, be very careful with access()! Uses like this are broken,
you want to rely on O_EXCL (or fopen's "x") instead, to ensure the file
is only created if it doesn't exist yet, and that is atomic.

Quite frankly, the majority of the uses of access()es I have seen in my
life are misuses, people really should keep the atomicity of things in mind. 

>  static int parse_new_root_from_proc_cmdline(void) {
>          char *w, *state;
> -        _cleanup_free_ char *what = NULL, *type = NULL, *opts = NULL, *line = NULL;
> +        _cleanup_free_ char *what = NULL, *type = NULL, *opts = NULL, *line = NULL, *mu = NULL;
>          int r;
>          size_t l;
>  
> +        /* Skip generation, if sysroot.mount already exists */
> +        mu = strjoin(arg_dest, "/", "sysroot.mount", NULL);
> +        if (!mu)
> +                return log_oom();
> +
> +        r = access(mu, R_OK);
> +        if (r == 0)
> +            return 0;
> +

Same here. Also: indentation is borked.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.