Re: [PATCH] cifs: add comments explaining cifs_new_fileinfo behavior

On Mon, 10 May 2010 20:00:05 +0530
Suresh Jayaraman <[email protected]> wrote:

> The comments make it clear the otherwise subtle behavior of cifs_new_fileinfo().
> 
> Signed-off-by: Suresh Jayaraman <[email protected]>
> --
>  fs/cifs/dir.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index d791d07..bd363df 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -129,6 +129,12 @@ cifs_bp_rename_retry:
>  	return full_path;
>  }
>  
> +/*
> + * When called with struct file pointer set to NULL, there is no way we could
> + * update file->private_data, but getting it stuck on openFileList provides a
> + * way to access it from cifs_fill_filedata and thereby set file->private_data
> + * from cifs_open.
> + */
>  struct cifsFileInfo *
>  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
>  		  struct file *file, struct vfsmount *mnt, unsigned int oflags)
> @@ -251,6 +257,10 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  		cifs_fattr_to_inode(*pinode, &fattr);
>  	}
>  
> +	/*
> +	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
> +	 * file->private_data.
> +	 */
>  	if (mnt)
>  		cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt, oflags);
>  
> @@ -466,8 +476,12 @@ cifs_create_set_dentry:
>  		/* mknod case - do not leave file open */
>  		CIFSSMBClose(xid, tcon, fileHandle);
>  	} else if (!(posix_create) && (newinode)) {
> -			cifs_new_fileinfo(newinode, fileHandle, NULL,
> -						nd->path.mnt, oflags);
> +		/*
> +		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> +		 * pointer to file->private_data.
> +		 */
> +		cifs_new_fileinfo(newinode, fileHandle, NULL, nd->path.mnt,
> +				  oflags);
>  	}
>  cifs_create_out:
>  	kfree(buf);
> _______________________________________________
> linux-cifs-client mailing list
> [email protected]
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 

The comments help, but this code has bothered me for some time. Is it
possible for the create to return success and for something else to
happen such that the cifs_open is never called? I'd imagine that it is,
and if so, then this this open file will be "leaked".

I think we should be using lookup_instantiate_filp here instead and
doing this similarly to the open on lookup code. There is some
precedent for this -- fuse_create_open does this and it gets called via
the .create operation.

-- 
Jeff Layton <[email protected]>