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

[Lennart Poettering]

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?

> +++ 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.

> +        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...

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...

> +
> +        /* 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? 

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?

> +
> +        /* 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...

> +
> +        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...

Looks good otherwise,

Lennart

-- 
Lennart Poettering, Red Hat