[linux-cifs-client] Re: [PATCH] cifs: fix renaming one hardlink on top of another

[Jeff Layton]

On Mon, 3 Nov 2008 09:12:53 -0600
"Steve French" <smfrench at gmail.com> wrote:

> This fix looks right ... unfortunately the old code (2.6.27 and
> earlier) had a similar bug, it looks like it returned EEXIST for this
> case.
> 
> What are you using to test this - the mv command does not obey posix
> semantics (does not call rename in vfs) and I am not convinced that
> the rename command does either.
> 

I noticed it because the nfsidem test in connectathon failed (the
rename() failed with -EEXIST).

I don't think the older code failed this in most cases though. As long
as the last CIFSSMBUnixQPathInfo call worked, then we'll return 0 on
the old code in this case.

The reason this broke now is that we changed cifs_rename to use a tmprc
variable to hold the return code of the CIFSSMBUnixQPathInfo call in
order to preserve the value of "rc" while we're checking for a hardlink.


> On Mon, Nov 3, 2008 at 8:07 AM, Jeff Layton <jlayton at redhat.com> wrote:
> > POSIX says that renaming one hardlink on top of another to the same
> > inode is a no-op. We had the logic mostly right, but forgot to clear
> > the return code.
> >
> > Signed-off-by: Jeff Layton <jlayton at redhat.com>
> > ---
> >  fs/cifs/inode.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index d54fa8a..ff8c68d 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -1361,9 +1361,11 @@ int cifs_rename(struct inode *source_dir, struct dentry *source_dentry,
> >                                        CIFS_MOUNT_MAP_SPECIAL_CHR);
> >
> >                if (tmprc == 0 && (info_buf_source->UniqueId ==
> > -                                  info_buf_target->UniqueId))
> > +                                  info_buf_target->UniqueId)) {
> >                        /* same file, POSIX says that this is a noop */
> > +                       rc = 0;
> >                        goto cifs_rename_exit;
> > +               }
> >        } /* else ... BB we could add the same check for Windows by
> >                     checking the UniqueId via FILE_INTERNAL_INFO */
> >
> > --
> > 1.5.5.1
> >
> >
> 
> 
> 


-- 
Jeff Layton <jlayton at redhat.com>