[systemd-devel] [PATCH] util: add rename_noreplace

[Alban Crequy]

Comments inline:

On Tue, Mar 10, 2015 at 4:25 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Tue, 10.03.15 16:16, Alban Crequy (alban.crequy at gmail.com) wrote:
>
>> -        if (renameat2(AT_FDCWD, t, AT_FDCWD, to, replace ? 0 : RENAME_NOREPLACE) < 0) {
>> -                unlink_noerrno(t);
>> -                return -errno;
>> +        if (replace) {
>> +                if (renameat(AT_FDCWD, t, AT_FDCWD, to) < 0) {
>> +                        unlink_noerrno(t);
>> +                        return -errno;
>> +                }
>> +        } else {
>> +                if (rename_noreplace(AT_FDCWD, t, AT_FDCWD, to) < 0) {
>> +                        unlink_noerrno(t);
>> +                        return -errno;
>> +                }
>>          }
>
> Maybe shorten this by first assigning this to a temporary variable
> "r", and then only have one error if check for both cases?

Ok.

>> +++ b/src/shared/util.c
>> @@ -8118,3 +8118,37 @@ void cmsg_close_all(struct msghdr *mh) {
>>                  if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_RIGHTS)
>>                          close_many((int*) CMSG_DATA(cmsg), (cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int));
>>  }
>> +
>> +int rename_noreplace(int olddirfd, const char *oldpath, int newdirfd, const char *newpath)
>> +{
>
> Opening { for functions on the same line as the function name please, see CODING_STYLE.

Ok.

>> +        struct stat buf;
>> +        int ret;
>> +
>> +        ret = renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE);
>> +
>> +        /* Even though renameat2() exists since Linux 3.15, btrfs added
>> +         * support for it later. If it is not implemented, fallback to another
>> +         * method. */
>> +        if (ret != -1 || errno != EINVAL)
>> +                return ret;
>
> Hmm, not that it would matter much, but in systemd we so far mostly
> check for < 0 rather then == -1 when looking for errors in syscalls...

Ok. I'm changing for ret == 0 here though since we are checking for success.

> Also, I think it would be a good idea to return "-errno" here, even
> though the underlying syscalls don't do that. However, the "return
> -errno" thing is pretty ubiquitously used in systemd, and at least as
> useful as return the kernel's original -1 here...

Callers print errno with log_error_errno(%m) so it seems easier to
keep the same return values as renameat2.

Or do you want to give the caller the choice between using the return
value or errno?

>> +        /* The link()/unlink() fallback does not work on directories. But
>> +         * renameat() without RENAME_NOREPLACE gives the same semantics on
>> +         * directories, except when newpath is an *empty* directory. This is
>> +         * good enough. */
>> +        ret = fstatat(olddirfd, oldpath, &buf, 0);
>> +        if (ret == 0 && S_ISDIR(buf.st_mode))
>> +                return renameat(olddirfd, oldpath, newdirfd,
>> newpath);
>
> Hmm, the fstatat() should use AT_SYMLINK_NOFOLLOW, no?

Agree.

> I figure we should explicitly return with an error on stuff that is
> neither S_ISDIR, nor S_ISREG, since neither renameat() nor link()
> would work on it, right?

I was expecting linkat() to catch the errors. I tried to create hard
links on socket, fifo and device files and it works fine. I don't mind
either way.

>> +
>> +        /* If it is not a directory, use the link()/unlink() fallback. */
>> +        ret = linkat(olddirfd, oldpath, newdirfd, newpath, 0);
>> +        if (ret == -1)
>> +                return ret;
>
> The usual < 0 and return -errno applies here...

Ok for < 0.

For -errno, cf question above.

>> +
>> +        ret = unlinkat(olddirfd, oldpath, 0);
>> +        if (ret != 0) {
>> +                if (unlinkat(newdirfd, newpath, 0) != 0)
>> +                        log_error("Failed to recover from
>> unlinkat() failure.");
>
> In systemd we try to enforce a regime: either functions log errors
> themsevles, or they don't. But they should not log some errors and
> others not, like it's done here...
>
> Given that this is a low-level library-like call I think it should
> never log on its own here. Instead, just eat up the second error...

Good reason. I am removing the log.

> Looks good otherwise,

Thanks for the review!

Alban