Re: [PATCH] sanitize xattr handler prototypes

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Wed, 2009-09-23 at 14:41 +0200, Christoph Hellwig wrote:
> On Thu, Jun 25, 2009 at 07:35:43PM +0200, Christoph Hellwig wrote:
> > Add a flags argument to strcut xattr_handler and pass it to all xattr
> > handler methods.  This allows using the same methods for multiple
> > handlers, e.g. for the ACL methods which perform exactly the same action
> > for the access and default ACLs, just using a different underlying
> > attribute.  With a little more groundwork it'll also allow sharing the
> > methods for the regular user/trusted/secure handlers in extN, ocfs2 and
> > jffs2 like it's already done for xfs in this patch.
> > 
> > Also change the inode argument to the handlers to a dentry to allow
> > using the handlers mechnism for filesystems that require it later,
> > e.g. cifs.
> 
> Updated for current mainline which saw gfs2 converted to the xattr
> handlers infrastructure:
> 
Wrt the GFS2 bits, they look good, but with one query:

> Index: linux-2.6/fs/gfs2/acl.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/acl.c	2009-09-22 23:48:35.905254352 -0300
> +++ linux-2.6/fs/gfs2/acl.c	2009-09-22 23:48:40.801005886 -0300
> @@ -217,8 +217,8 @@ int gfs2_acl_create(struct gfs2_inode *d
>  	acl = clone;
>  
>  	if (S_ISDIR(ip->i_inode.i_mode)) {
> -		error = gfs2_xattr_set(&ip->i_inode, GFS2_EATYPE_SYS,
> -				       GFS2_POSIX_ACL_DEFAULT, data, len, 0);
> +		error = __gfs2_xattr_set(ip, GFS2_POSIX_ACL_DEFAULT, data,
> +					 len, 0, GFS2_EATYPE_SYS);
>  		if (error)
>  			goto out;
>  	}
> @@ -230,8 +230,8 @@ int gfs2_acl_create(struct gfs2_inode *d
>  		goto munge;
>  
>  	posix_acl_to_xattr(acl, data, len);
> -	error = gfs2_xattr_set(&ip->i_inode, GFS2_EATYPE_SYS,
> -			       GFS2_POSIX_ACL_ACCESS, data, len, 0);
> +	error = __gfs2_xattr_set(ip, GFS2_POSIX_ACL_ACCESS, data, len, 0,
> +				 GFS2_EATYPE_SYS);
>  	if (error)
>  		goto out;
>  munge:
> Index: linux-2.6/fs/gfs2/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/inode.c	2009-09-22 23:48:35.913254045 -0300
> +++ linux-2.6/fs/gfs2/inode.c	2009-09-22 23:48:40.804006338 -0300
> @@ -801,7 +801,7 @@ static int gfs2_security_init(struct gfs
>  		return err;
>  	}
>  
> -	err = gfs2_xattr_set(&ip->i_inode, GFS2_EATYPE_SECURITY, name, value, len, 0);
> +	err = __gfs2_xattr_set(ip, name, value, len, 0, GFS2_EATYPE_SECURITY);
>  	kfree(value);
>  	kfree(name);
>  
> Index: linux-2.6/fs/gfs2/xattr.c
> ===================================================================
> --- linux-2.6.orig/fs/gfs2/xattr.c	2009-09-22 23:48:35.918278918 -0300
> +++ linux-2.6/fs/gfs2/xattr.c	2009-09-22 23:48:40.809005649 -0300
> @@ -537,18 +537,17 @@ int gfs2_ea_get_copy(struct gfs2_inode *
>  /**
>   * gfs2_xattr_get - Get a GFS2 extended attribute
>   * @inode: The inode
> - * @type: The type of extended attribute
>   * @name: The name of the extended attribute
>   * @buffer: The buffer to write the result into
>   * @size: The size of the buffer
> + * @type: The type of extended attribute
>   *
>   * Returns: actual size of data on success, -errno on error
>   */
> -
> -int gfs2_xattr_get(struct inode *inode, int type, const char *name,
> -		   void *buffer, size_t size)
> +static int gfs2_xattr_get(struct dentry *dentry, const char *name,
> +		void *buffer, size_t size, int type)
>  {
> -	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_inode *ip = GFS2_I(dentry->d_inode);
>  	struct gfs2_ea_location el;
>  	int error;
>  
> @@ -1089,7 +1088,7 @@ static int ea_remove_stuffed(struct gfs2
>  
>  /**
>   * gfs2_xattr_remove - Remove a GFS2 extended attribute
> - * @inode: The inode
> + * @ip: The inode
>   * @type: The type of the extended attribute
>   * @name: The name of the extended attribute
>   *
> @@ -1100,9 +1099,8 @@ static int ea_remove_stuffed(struct gfs2
>   * Returns: 0, or errno on failure
>   */
>  
> -static int gfs2_xattr_remove(struct inode *inode, int type, const char *name)
> +static int gfs2_xattr_remove(struct gfs2_inode *ip, int type, const char *name)
>  {
> -	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct gfs2_ea_location el;
>  	int error;
>  
> @@ -1126,35 +1124,32 @@ static int gfs2_xattr_remove(struct inod
>  }
>  
>  /**
> - * gfs2_xattr_set - Set (or remove) a GFS2 extended attribute
> - * @inode: The inode
> - * @type: The type of the extended attribute
> + * __gfs2_xattr_set - Set (or remove) a GFS2 extended attribute
> + * @ip: The inode
>   * @name: The name of the extended attribute
>   * @value: The value of the extended attribute (NULL for remove)
>   * @size: The size of the @value argument
>   * @flags: Create or Replace
> + * @type: The type of the extended attribute
>   *
>   * See gfs2_xattr_remove() for details of the removal of xattrs.
>   *
>   * Returns: 0 or errno on failure
>   */
>  
> -int gfs2_xattr_set(struct inode *inode, int type, const char *name,
> -		   const void *value, size_t size, int flags)
> +int __gfs2_xattr_set(struct gfs2_inode *ip, const char *name,
> +		   const void *value, size_t size, int flags, int type)
>  {
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct gfs2_inode *ip = GFS2_I(inode);
> +	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct gfs2_ea_location el;
>  	unsigned int namel = strlen(name);
>  	int error;
>  
> -	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> -		return -EPERM;
We need to leave these checks here because the checks made in the VFS
are done before the glock is held, so they might yield the wrong
answers. I think we should check again here just to be on the safe side.
Otherwise:

Acked-by: Steven Whitehouse <swhiteho@redhat.com>

Steve.