Re: [PATCH 11/11] ext4: add cross rename support

[J. Bruce Fields]

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

> (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
objects".

--b.

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