[linux-cifs-client] Re: [PATCH] cifs: fix busy-file renames and refactor cifs_rename ...

[Jeff Layton]

On Mon, 22 Sep 2008 18:50:39 -0500
"Steve French" <smfrench at gmail.com> wrote:

> A few comments about patch
> http://git.samba.org/?p=jlayton/cifs.git;a=commitdiff;h=3a611c1fed0879ba3b672e221b4a8fcb20d52ef2
> 
> 1) The check for from_dentry->d_parent != to_dentry->d_parent in
> cifs_do_rename seems unnecessary since it is not required that a
> server reject a rename by handle outside of the current directory
> although some MS servers do reject this.
> 

Did we ever get an answer on this from Microsoft? Are there any servers
that we know of that do allow cross-directory moves via rename by
filehandle, but that don't allow renaming open files via path?

> 2) In cifs_do_rename, my preference would be to remove the goto and
> corresponding label (by reversing the if to "if (rc==0)
> CIFSSMBRenameOpenFile ,,,").  In general I prefer only using gotos
> when it simplifies the code
> 

Sounds reasonable, I'll respin it that way...

> 3) by reordering the FreeXid statement at the beginning of cifs_rename
> you lose the logging of the function return code on a common case
> (EXDEV) when the debug flags are turned on in /proc/fs/cifs/
> 
> -       if (pTcon != cifs_sb_target->tcon) {
> -               FreeXid(xid);
> -               return -EXDEV;

Ugh, I really dislike the way that these debug statements are
tangled up with the Xid allocation code, but I'll respin this
with that in mind.

Thanks for the review...

-- 
Jeff Layton <jlayton at redhat.com>