Re: [PATCH 06/13] cifs: add new cifs_iget function and convert unix codepath to use it

From: Christoph Hellwig <hch@infradead.org>
To: Jeff Layton <jlayton@redhat.com>
Cc: linux-cifs-client@lists.samba.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 06/13] cifs: add new cifs_iget function and convert unix codepath to use it
Date: Mon, 11 May 2009 17:06:05 -0400	[thread overview]
Message-ID: <20090511210605.GE5751@infradead.org> (raw)
In-Reply-To: <1242073472-7100-7-git-send-email-jlayton@redhat.com>

On Mon, May 11, 2009 at 04:24:25PM -0400, Jeff Layton wrote:
> Add a new cifs_iget function that uses iget5_locked to identify inodes.
> This will compare inodes based on the uniqueid value in a cifs_fattr
> struct.
> 
> Rather than filling out an already-created inode, have
> cifs_get_inode_info_unix instead fill out cifs_fattr and hand that off
> to cifs_iget. cifs_iget can then properly look for hardlinked inodes.
> 
> With this, we should then have proper hardlink detection and can
> eventually get rid of some nasty CIFS-specific hacks for handing them.
> 
> Fixing the unix readdir codepath will be done in a later patch.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

> +	fattr = cifs_unix_basic_to_fattr(presp_data, cifs_sb);
> +	if (!fattr) {
> +		rc = -ENOMEM;
> +		goto posix_open_ret;
> +	}

The cifs_fattr is probably small enough to be stored on-stack in
many places.  And a calling convention where allocations if needed
are done by the caller and cifs_unix_basic_to_fattr just fills the
buffer is probably nicer either way.

>  /*
> + * Allocate a cifs_fattr struct and fill it with fake inode info.
> + *
> + * Needed to setup cifs_fattr data for the directory which is the
> + * junction to the new submount (ie to setup the fake directory
> + * which represents a DFS referral).
> + *
> + * Returns pointer to new cifs_fattr or NULL on error.
>   */

> +static struct cifs_fattr *
> +cifs_create_fake_fattr(struct super_block *sb)

Same comment about caling conventions applies here, too.

Also wou;dn't something like cifs_create_dfsmount_fattr be a better
name?