Re: [PATCH 11/11] ext4: add cross rename support
From: J. Bruce Fields
Date: Fri Jan 17 2014 - 17:09:39 EST
On Fri, Jan 17, 2014 at 11:53:07PM +1300, Michael Kerrisk (man-pages) wrote:
> 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.
I agree that EINVAL makes sense for an invalid bit flag.
But renameat2() can also fail when the caller passes a perfectly valid
flags field but the paths resolve to a filesystem that doesn't support
the RENAME_EXCHANGE operation. EOPNOTSUPP looks more appropriate in
> (See the man pages for the *at() calls that have a 'flags" argument.)
Aren't those flags mostly handled in the vfs? In which case they work
everywhere, so there isn't the same distinction between "flag is
defined" and "behavior requested by flag is unsupported for the given
> > EINVAL Invalid combination of flags
> (This is okay.)
> Looks otherwise okay to me (and I agree with Bruce's comments).
> 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
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/