On Tue, 10.03.15 16:16, Alban Crequy (alban.cre...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel