[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

[Harald Hoyer]

Am 05.03.2013 02:05, schrieb 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
> 

Sorry... reverted anyway