Hi Miklos,
A few comments below, including one piece in the code that really must be fixed.
On 01/16/2014 11:54 PM, Miklos Szeredi wrote:
>> On Wed, Jan 15, 2014 at 7:23 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
>>> Do you have a man page update somewhere for the two new flags?
>
> Here's the updated man page (and attached the patch)
>
> Michael, could you please review the interface?
>
> I forgot to CC you when posing the patch series. I can resend it if you want,
> or you can fetch the latest version of the cross-rename series from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git cross-rename
[...]
> renameat2() has an additional flags argument. renameat2() call with a
> zero flags argument is equivalent to renameat().
>
> The flags argument is a bitfield consisting of zero or more of the fol-
> lowing constants defined in <linux/fs.h>:
>
> RENAME_NOREPLACE
> Don't overwrite the target of the rename. Return an error if
> the target would be overwritten.
>
> RENAME_EXCHANGE
> Atomically exchange the source and destination. Both must exist
> but may be of a different type (e.g. one a non-empty directory
> and the other a symbolic link).
Somewhere here it would be good to explain the consequences if
(flags & (RENAME_NOREPLACE | RENAME_EXCHANGE)) ==
(RENAME_NOREPLACE | RENAME_EXCHANGE)
Okay -- it's EINVAL, but here the man page text should say something like
"these two flags can't be specified together", right?
> RETURN VALUE
> On success, renameat() and renameat2() return 0. On error, -1 is
> returned and errno is set to indicate the error.
>
> ERRORS
> The same errors that occur for rename(2) can also occur for renameat()
> and renameat2(). The following additional errors can occur for
> renameat() and renameat2():
>
> EBADF olddirfd or newdirfd is not a valid file descriptor.
>
> ENOTDIR
> oldpath is relative and olddirfd is a file descriptor referring
> to a file other than a directory; or similar for newpath and
> newdirfd
>
> The following additional errors are defined for renameat2():
>
> EOPNOTSUPP
> The filesystem does not support a flag in flags
This is not the usual error for an invalid bit flag. Please make it EINVAL.
(See the man pages for the *at() calls that have a 'flags" argument.)
> EINVAL Invalid combination of flags
(This is okay.)
Looks otherwise okay to me (and I agree with Bruce's comments).
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
