Comments inline: On Tue, Mar 10, 2015 at 4:25 PM, Lennart Poettering <lenn...@poettering.net> wrote: > 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?
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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel