[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0006-fs-make-posix_acl_create-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131201/3aac7557/attachment.ksh>

[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Jan Kara]

On Sun 01-12-13 03:59:09, Christoph Hellwig wrote:
> Rename the current posix_acl_created to __posix_acl_create and add
> a fully featured helper to set up the ACLs on file creation that
> uses get_acl().
  Looks good, you can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/9p/acl.c               |    2 +-
>  fs/btrfs/acl.c            |    2 +-
>  fs/ext2/acl.c             |    2 +-
>  fs/ext3/acl.c             |    2 +-
>  fs/ext4/acl.c             |    2 +-
>  fs/f2fs/acl.c             |    2 +-
>  fs/generic_acl.c          |    2 +-
>  fs/gfs2/acl.c             |    2 +-
>  fs/hfsplus/posix_acl.c    |    2 +-
>  fs/jffs2/acl.c            |    2 +-
>  fs/jfs/acl.c              |    2 +-
>  fs/nfs/nfs3acl.c          |    2 +-
>  fs/ocfs2/acl.c            |    2 +-
>  fs/posix_acl.c            |   53 +++++++++++++++++++++++++++++++++++++++++++--
>  fs/reiserfs/xattr_acl.c   |    2 +-
>  fs/xfs/xfs_acl.c          |    4 ++--
>  include/linux/posix_acl.h |    6 ++++-
>  17 files changed, 72 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> index f5ce5c5..8482f2d 100644
> --- a/fs/9p/acl.c
> +++ b/fs/9p/acl.c
> @@ -200,7 +200,7 @@ int v9fs_acl_mode(struct inode *dir, umode_t *modep,
>  	if (acl) {
>  		if (S_ISDIR(mode))
>  			*dpacl = posix_acl_dup(acl);
> -		retval = posix_acl_create(&acl, GFP_NOFS, &mode);
> +		retval = __posix_acl_create(&acl, GFP_NOFS, &mode);
>  		if (retval < 0)
>  			return retval;
>  		if (retval > 0)
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 1af04ff..b56519d 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -222,7 +222,7 @@ int btrfs_init_acl(struct btrfs_trans_handle *trans,
>  			if (ret)
>  				goto failed;
>  		}
> -		ret = posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
> +		ret = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c
> index 7006ced..6e842a7 100644
> --- a/fs/ext2/acl.c
> +++ b/fs/ext2/acl.c
> @@ -268,7 +268,7 @@ ext2_init_acl(struct inode *inode, struct inode *dir)
>  			if (error)
>  				goto cleanup;
>  		}
> -		error = posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
> +		error = __posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
>  		if (error < 0)
>  			return error;
>  		if (error > 0) {
> diff --git a/fs/ext3/acl.c b/fs/ext3/acl.c
> index 6691a6c..4f3d8fa 100644
> --- a/fs/ext3/acl.c
> +++ b/fs/ext3/acl.c
> @@ -271,7 +271,7 @@ ext3_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
>  			if (error)
>  				goto cleanup;
>  		}
> -		error = posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
> +		error = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
>  		if (error < 0)
>  			return error;
>  
> diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
> index 2eebe02..f827f3b 100644
> --- a/fs/ext4/acl.c
> +++ b/fs/ext4/acl.c
> @@ -276,7 +276,7 @@ ext4_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
>  			if (error)
>  				goto cleanup;
>  		}
> -		error = posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
> +		error = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
>  		if (error < 0)
>  			return error;
>  
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 14c4df0..45e8430 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -285,7 +285,7 @@ int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage)
>  		if (error)
>  			goto cleanup;
>  	}
> -	error = posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
> +	error = __posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
>  	if (error < 0)
>  		return error;
>  	if (error > 0)
> diff --git a/fs/generic_acl.c b/fs/generic_acl.c
> index 46a5076..4357f39 100644
> --- a/fs/generic_acl.c
> +++ b/fs/generic_acl.c
> @@ -128,7 +128,7 @@ generic_acl_init(struct inode *inode, struct inode *dir)
>  	if (acl) {
>  		if (S_ISDIR(inode->i_mode))
>  			set_cached_acl(inode, ACL_TYPE_DEFAULT, acl);
> -		error = posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
> +		error = __posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
>  		if (error < 0)
>  			return error;
>  		if (error > 0)
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 3e200c7..e82e4ac 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -131,7 +131,7 @@ int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode)
>  			goto out;
>  	}
>  
> -	error = posix_acl_create(&acl, GFP_NOFS, &mode);
> +	error = __posix_acl_create(&acl, GFP_NOFS, &mode);
>  	if (error < 0)
>  		return error;
>  
> diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
> index cab5fd6..277942f 100644
> --- a/fs/hfsplus/posix_acl.c
> +++ b/fs/hfsplus/posix_acl.c
> @@ -137,7 +137,7 @@ int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
>  				goto init_acl_cleanup;
>  		}
>  
> -		err = posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
> +		err = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
>  		if (unlikely(err < 0))
>  			return err;
>  
> diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c
> index 5853969..4d6e31b 100644
> --- a/fs/jffs2/acl.c
> +++ b/fs/jffs2/acl.c
> @@ -295,7 +295,7 @@ int jffs2_init_acl_pre(struct inode *dir_i, struct inode *inode, umode_t *i_mode
>  		if (S_ISDIR(*i_mode))
>  			set_cached_acl(inode, ACL_TYPE_DEFAULT, acl);
>  
> -		rc = posix_acl_create(&acl, GFP_KERNEL, i_mode);
> +		rc = __posix_acl_create(&acl, GFP_KERNEL, i_mode);
>  		if (rc < 0)
>  			return rc;
>  		if (rc > 0)
> diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c
> index 9c0fca8..28d529a 100644
> --- a/fs/jfs/acl.c
> +++ b/fs/jfs/acl.c
> @@ -132,7 +132,7 @@ int jfs_init_acl(tid_t tid, struct inode *inode, struct inode *dir)
>  			if (rc)
>  				goto cleanup;
>  		}
> -		rc = posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
> +		rc = __posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
>  		if (rc < 0)
>  			goto cleanup; /* posix_acl_release(NULL) is no-op */
>  		if (rc > 0)
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index 4a1aafb..e859675 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -428,7 +428,7 @@ int nfs3_proc_set_default_acl(struct inode *dir, struct inode *inode,
>  	if (!dfacl)
>  		return 0;
>  	acl = posix_acl_dup(dfacl);
> -	error = posix_acl_create(&acl, GFP_KERNEL, &mode);
> +	error = __posix_acl_create(&acl, GFP_KERNEL, &mode);
>  	if (error < 0)
>  		goto out_release_dfacl;
>  	error = nfs3_proc_setacls(inode, acl, S_ISDIR(inode->i_mode) ?
> diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c
> index 73ccf0e..c0f9d2f 100644
> --- a/fs/ocfs2/acl.c
> +++ b/fs/ocfs2/acl.c
> @@ -401,7 +401,7 @@ int ocfs2_init_acl(handle_t *handle,
>  				goto cleanup;
>  		}
>  		mode = inode->i_mode;
> -		ret = posix_acl_create(&acl, GFP_NOFS, &mode);
> +		ret = __posix_acl_create(&acl, GFP_NOFS, &mode);
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 9f76aaa..38d6a49 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -384,7 +384,7 @@ static int __posix_acl_chmod_masq(struct posix_acl *acl, umode_t mode)
>  }
>  
>  int
> -posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
> +__posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
>  {
>  	struct posix_acl *clone = posix_acl_clone(*acl, gfp);
>  	int err = -ENOMEM;
> @@ -399,7 +399,7 @@ posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p)
>  	*acl = clone;
>  	return err;
>  }
> -EXPORT_SYMBOL(posix_acl_create);
> +EXPORT_SYMBOL(__posix_acl_create);
>  
>  int
>  __posix_acl_chmod(struct posix_acl **acl, gfp_t gfp, umode_t mode)
> @@ -443,6 +443,55 @@ posix_acl_chmod(struct inode *inode)
>  }
>  EXPORT_SYMBOL(posix_acl_chmod);
>  
> +int
> +posix_acl_create(struct inode *dir, umode_t *mode,
> +		struct posix_acl **default_acl, struct posix_acl **acl)
> +{
> +	struct posix_acl *p;
> +	int ret;
> +
> +	if (S_ISLNK(*mode) || !IS_POSIXACL(dir))
> +		goto no_acl;
> +
> +	p = get_acl(dir, ACL_TYPE_DEFAULT);
> +	if (IS_ERR(p))
> +		return PTR_ERR(p);
> +
> +	if (!p) {
> +		*mode &= ~current_umask();
> +		goto no_acl;
> +	}
> +
> +	*acl = posix_acl_clone(p, GFP_NOFS);
> +	if (!*acl)
> +		return -ENOMEM;
> +
> +	ret = posix_acl_create_masq(*acl, mode);
> +	if (ret < 0) {
> +		posix_acl_release(*acl);
> +		return -ENOMEM;
> +	}
> +
> +	if (ret == 0) {
> +		posix_acl_release(*acl);
> +		*acl = NULL;
> +	}
> +
> +	if (!S_ISDIR(*mode)) {
> +		posix_acl_release(p);
> +		*default_acl = NULL;
> +	} else {
> +		*default_acl = p;
> +	}
> +	return 0;
> +
> +no_acl:
> +	*default_acl = NULL;
> +	*acl = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(posix_acl_create);
> +
>  struct posix_acl *get_acl(struct inode *inode, int type)
>  {
>  	struct posix_acl *acl;
> diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c
> index ea4e443..d95c959 100644
> --- a/fs/reiserfs/xattr_acl.c
> +++ b/fs/reiserfs/xattr_acl.c
> @@ -378,7 +378,7 @@ reiserfs_inherit_default_acl(struct reiserfs_transaction_handle *th,
>  
>  		/* Now we reconcile the new ACL and the mode,
>  		   potentially modifying both */
> -		err = posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
> +		err = __posix_acl_create(&acl, GFP_NOFS, &inode->i_mode);
>  		if (err < 0)
>  			return err;
>  
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index 4eac105..057ae2d 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -297,12 +297,12 @@ xfs_inherit_acl(struct inode *inode, struct posix_acl *acl)
>  			goto out;
>  	}
>  
> -	error = posix_acl_create(&acl, GFP_KERNEL, &mode);
> +	error = __posix_acl_create(&acl, GFP_KERNEL, &mode);
>  	if (error < 0)
>  		return error;
>  
>  	/*
> -	 * If posix_acl_create returns a positive value we need to
> +	 * If __posix_acl_create returns a positive value we need to
>  	 * inherit a permission that can't be represented using the Unix
>  	 * mode bits and we actually need to set an ACL.
>  	 */
> diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
> index 8b64e78..9ec6b45 100644
> --- a/include/linux/posix_acl.h
> +++ b/include/linux/posix_acl.h
> @@ -88,14 +88,18 @@ extern int posix_acl_valid(const struct posix_acl *);
>  extern int posix_acl_permission(struct inode *, const struct posix_acl *, int);
>  extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t);
>  extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *);
> -extern int posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
> +extern int __posix_acl_create(struct posix_acl **, gfp_t, umode_t *);
>  extern int __posix_acl_chmod(struct posix_acl **, gfp_t, umode_t);
> +extern int posix_acl_prepare(struct inode *dir, struct inode *inode,
> +		umode_t *mode);
>  
>  extern struct posix_acl *get_posix_acl(struct inode *, int);
>  extern int set_posix_acl(struct inode *, int, struct posix_acl *);
>  
>  #ifdef CONFIG_FS_POSIX_ACL
>  extern int posix_acl_chmod(struct inode *);
> +extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **,
> +		struct posix_acl **);
>  
>  static inline struct posix_acl **acl_by_type(struct inode *inode, int type)
>  {
> -- 
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Cluster-devel] [PATCH 00/18] Consolidate Posix ACL implementation V2

[Christoph Hellwig]

This series consolidates the various cut'n'pasted Posix ACL implementations
into a single common one based on the ->get_acl method Linus added a while
ago and a new ->set_acl counterpart.

This remove ~1800 lines of code and provides a single place to implement
various nasty little gems of the semantics.

Unfortunately the 9p code is still left out - it implements the ACLs
in two very weird ways, one using the common code but on the client only,
and one pasing things straight through to the server.  We could easily
convert it to the new code on the write side if ->set_acl took a dentry,
but there's no cance to do that on the ->get_acl side.  Ideas how to
handle it welcome.

After that we'd be ready to never go into the fs for the ACL attributes
and branch straight to the ACL code below the syscall, repairing the
old API braindamage of overloading ACLs onto the xattrs.


Changes from V1:
 - check for symlinks in the ACL code and remove checks in the lower
   level functions.
 - remove get_acl instances for symlinks in a few filesystems
 - pass a umode_t mode argument to posix_acl_chmod to accomodate f2fs
 - various cosemtic bits from the reviews.

Note that I still haven't heard from ocfs2 folks, so the patch is left
unchanged.

[Cluster-devel] [PATCH 01/18] reiserfs: prefix ACL symbols with reiserfs_

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0001-reiserfs-prefix-ACL-symbols-with-reiserfs_.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/35841bc7/attachment.ksh>

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0002-fs-add-get_acl-helper.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2311bc1c/attachment.ksh>

[Cluster-devel] [PATCH 03/18] fs: add a set_acl inode operation

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0003-fs-add-a-set_acl-inode-operation.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/12450327/attachment.ksh>

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0004-fs-add-generic-xattr_acl-handlers.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/91db43bc/attachment.ksh>

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0005-fs-make-posix_acl_chmod-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/176a07d3/attachment.ksh>

[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0006-fs-make-posix_acl_create-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/eb09193e/attachment.ksh>

[Cluster-devel] [PATCH 07/18] btrfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0007-btrfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/876d757d/attachment.ksh>

[Cluster-devel] [PATCH 08/18] ext2/3/4: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0008-ext2-3-4-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6d2a563e/attachment.ksh>

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0009-f2fs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2cec2935/attachment.ksh>

[Cluster-devel] [PATCH 10/18] hfsplus: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0010-hfsplus-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/d51bfb09/attachment.ksh>

[Cluster-devel] [PATCH 11/18] jffs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0011-jffs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/a3efba51/attachment.ksh>

[Cluster-devel] [PATCH 12/18] ocfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0012-ocfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/88e1778c/attachment.ksh>

[Cluster-devel] [PATCH 13/18] reiserfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0013-reiserfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6aa889e6/attachment.ksh>

[Cluster-devel] [PATCH 14/18] xfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0014-xfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/9a6a7104/attachment.ksh>

[Cluster-devel] [PATCH 15/18] jfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0015-jfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/c6ddfc3b/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0016-gfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/aafe34fb/attachment.ksh>

[Cluster-devel] [PATCH 17/18] nfs: use generic posix ACL infrastructure for v3 Posix ACLs

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0017-nfs-use-generic-posix-ACL-infrastructure-for-v3-Posi.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ce18263a/attachment.ksh>

[Cluster-devel] [PATCH 18/18] fs: remove generic_acl

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0018-fs-remove-generic_acl.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ff393c13/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Steven Whitehouse]

Hi,

On Wed, 2013-12-11 at 02:42 -0800, Christoph Hellwig wrote:
> plain text document attachment
> (0016-gfs2-use-generic-posix-ACL-infrastructure.patch)
> This contains some major refactoring for the create path so that
> inodes are created with the right mode to start with instead of
> fixing it up later.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Steven Whitehouse <swhiteho@redhat.com>

A really nice clean up - this is a very useful step forward in
simplifying the create path. Thanks for sorting this out,

Steve.

> ---
>  fs/gfs2/acl.c   |  234 +++++++------------------------------------------------
>  fs/gfs2/acl.h   |    4 +-
>  fs/gfs2/inode.c |   33 ++++++--
>  fs/gfs2/xattr.c |    4 +-
>  4 files changed, 62 insertions(+), 213 deletions(-)
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index e82e4ac..ba94566 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -49,10 +49,6 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	if (!ip->i_eattr)
>  		return NULL;
>  
> -	acl = get_cached_acl(&ip->i_inode, type);
> -	if (acl != ACL_NOT_CACHED)
> -		return acl;
> -
>  	name = gfs2_acl_name(type);
>  	if (name == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -80,7 +76,7 @@ static int gfs2_set_mode(struct inode *inode, umode_t mode)
>  	return error;
>  }
>  
> -static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
> +int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
>  	int error;
>  	int len;
> @@ -88,219 +84,49 @@ static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
>  	const char *name = gfs2_acl_name(type);
>  
>  	BUG_ON(name == NULL);
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	if (len == 0)
> -		return 0;
> -	data = kmalloc(len, GFP_NOFS);
> -	if (data == NULL)
> -		return -ENOMEM;
> -	error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	if (error < 0)
> -		goto out;
> -	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> -	if (!error)
> -		set_cached_acl(inode, type, acl);
> -out:
> -	kfree(data);
> -	return error;
> -}
> -
> -int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode)
> -{
> -	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
> -	struct posix_acl *acl;
> -	umode_t mode = inode->i_mode;
> -	int error = 0;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return 0;
> -	if (S_ISLNK(inode->i_mode))
> -		return 0;
> -
> -	acl = gfs2_get_acl(&dip->i_inode, ACL_TYPE_DEFAULT);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl) {
> -		mode &= ~current_umask();
> -		return gfs2_set_mode(inode, mode);
> -	}
> -
> -	if (S_ISDIR(inode->i_mode)) {
> -		error = gfs2_acl_set(inode, ACL_TYPE_DEFAULT, acl);
> -		if (error)
> -			goto out;
> -	}
> -
> -	error = __posix_acl_create(&acl, GFP_NOFS, &mode);
> -	if (error < 0)
> -		return error;
>  
> -	if (error == 0)
> -		goto munge;
> -
> -	error = gfs2_acl_set(inode, ACL_TYPE_ACCESS, acl);
> -	if (error)
> -		goto out;
> -munge:
> -	error = gfs2_set_mode(inode, mode);
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr)
> -{
> -	struct inode *inode = &ip->i_inode;
> -	struct posix_acl *acl;
> -	char *data;
> -	unsigned int len;
> -	int error;
> -
> -	acl = gfs2_get_acl(&ip->i_inode, ACL_TYPE_ACCESS);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl)
> -		return gfs2_setattr_simple(inode, attr);
> -
> -	error = __posix_acl_chmod(&acl, GFP_NOFS, attr->ia_mode);
> -	if (error)
> -		return error;
> -
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	data = kmalloc(len, GFP_NOFS);
> -	error = -ENOMEM;
> -	if (data == NULL)
> -		goto out;
> -	posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	error = gfs2_xattr_acl_chmod(ip, attr, data);
> -	kfree(data);
> -	set_cached_acl(&ip->i_inode, ACL_TYPE_ACCESS, acl);
> -
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -static int gfs2_acl_type(const char *name)
> -{
> -	if (strcmp(name, GFS2_POSIX_ACL_ACCESS) == 0)
> -		return ACL_TYPE_ACCESS;
> -	if (strcmp(name, GFS2_POSIX_ACL_DEFAULT) == 0)
> -		return ACL_TYPE_DEFAULT;
> -	return -EINVAL;
> -}
> -
> -static int gfs2_xattr_system_get(struct dentry *dentry, const char *name,
> -				 void *buffer, size_t size, int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl;
> -	int type;
> -	int error;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -
> -	acl = gfs2_get_acl(inode, type);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (acl == NULL)
> -		return -ENODATA;
> -
> -	error = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> -	posix_acl_release(acl);
> -
> -	return error;
> -}
> -
> -static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
> -				 const void *value, size_t size, int flags,
> -				 int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl = NULL;
> -	int error = 0, type;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -	if (flags & XATTR_CREATE)
> -		return -EINVAL;
> -	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> -		return value ? -EACCES : 0;
> -	if (!uid_eq(current_fsuid(), inode->i_uid) && !capable(CAP_FOWNER))
> -		return -EPERM;
> -	if (S_ISLNK(inode->i_mode))
> -		return -EOPNOTSUPP;
> -
> -	if (!value)
> -		goto set_acl;
> -
> -	acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	if (!acl) {
> -		/*
> -		 * acl_set_file(3) may request that we set default ACLs with
> -		 * zero length -- defend (gracefully) against that here.
> -		 */
> -		goto out;
> -	}
> -	if (IS_ERR(acl)) {
> -		error = PTR_ERR(acl);
> -		goto out;
> -	}
> -
> -	error = posix_acl_valid(acl);
> -	if (error)
> -		goto out_release;
> -
> -	error = -EINVAL;
>  	if (acl->a_count > GFS2_ACL_MAX_ENTRIES)
> -		goto out_release;
> +		return -EINVAL;
>  
>  	if (type == ACL_TYPE_ACCESS) {
>  		umode_t mode = inode->i_mode;
> +
>  		error = posix_acl_equiv_mode(acl, &mode);
> +		if (error < 0)
> +			return error;
>  
> -		if (error <= 0) {
> -			posix_acl_release(acl);
> +		if (error == 0)
>  			acl = NULL;
>  
> -			if (error < 0)
> -				return error;
> -		}
> -
>  		error = gfs2_set_mode(inode, mode);
>  		if (error)
> -			goto out_release;
> +			return error;
>  	}
>  
> -set_acl:
> -	error = __gfs2_xattr_set(inode, name, value, size, 0, GFS2_EATYPE_SYS);
> -	if (!error) {
> -		if (acl)
> -			set_cached_acl(inode, type, acl);
> -		else
> -			forget_cached_acl(inode, type);
> +	if (acl) {
> +		len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> +		if (len == 0)
> +			return 0;
> +		data = kmalloc(len, GFP_NOFS);
> +		if (data == NULL)
> +			return -ENOMEM;
> +		error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> +		if (error < 0)
> +			goto out;
> +	} else {
> +		data = NULL;
> +		len = 0;
>  	}
> -out_release:
> -	posix_acl_release(acl);
> +
> +	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> +	if (error)
> +		goto out;
> +
> +	if (acl)
> +		set_cached_acl(inode, type, acl);
> +	else
> +		forget_cached_acl(inode, type);
>  out:
> +	kfree(data);
>  	return error;
>  }
> -
> -const struct xattr_handler gfs2_xattr_system_handler = {
> -	.prefix = XATTR_SYSTEM_PREFIX,
> -	.flags  = GFS2_EATYPE_SYS,
> -	.get    = gfs2_xattr_system_get,
> -	.set    = gfs2_xattr_system_set,
> -};
> -
> diff --git a/fs/gfs2/acl.h b/fs/gfs2/acl.h
> index 0da38dc..301260c 100644
> --- a/fs/gfs2/acl.h
> +++ b/fs/gfs2/acl.h
> @@ -17,8 +17,6 @@
>  #define GFS2_ACL_MAX_ENTRIES		25
>  
>  extern struct posix_acl *gfs2_get_acl(struct inode *inode, int type);
> -extern int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode);
> -extern int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr);
> -extern const struct xattr_handler gfs2_xattr_system_handler;
> +extern int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  
>  #endif /* __ACL_DOT_H__ */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 7119504..92b8959 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -552,6 +552,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  			     unsigned int size, int excl, int *opened)
>  {
>  	const struct qstr *name = &dentry->d_name;
> +	struct posix_acl *default_acl, *acl;
>  	struct gfs2_holder ghs[2];
>  	struct inode *inode = NULL;
>  	struct gfs2_inode *dip = GFS2_I(dir), *ip;
> @@ -611,10 +612,14 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	if (!inode)
>  		goto fail_gunlock;
>  
> +	error = posix_acl_create(dir, &mode, &default_acl, &acl);
> +	if (error)
> +		goto fail_free_vfs_inode;
> +
>  	ip = GFS2_I(inode);
>  	error = gfs2_rs_alloc(ip);
>  	if (error)
> -		goto fail_free_inode;
> +		goto fail_free_acls;
>  
>  	inode->i_mode = mode;
>  	set_nlink(inode, S_ISDIR(mode) ? 2 : 1);
> @@ -682,7 +687,16 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	gfs2_set_iop(inode);
>  	insert_inode_hash(inode);
>  
> -	error = gfs2_acl_create(dip, inode);
> +	if (default_acl) {
> +		error = gfs2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		posix_acl_release(default_acl);
> +	}
> +	if (acl) {
> +		if (!error)
> +			error = gfs2_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +		posix_acl_release(acl);
> +	}
> +
>  	if (error)
>  		goto fail_gunlock3;
>  
> @@ -716,6 +730,12 @@ fail_free_inode:
>  	if (ip->i_gl)
>  		gfs2_glock_put(ip->i_gl);
>  	gfs2_rs_delete(ip, NULL);
> +fail_free_acls:
> +	if (default_acl)
> +		posix_acl_release(default_acl);
> +	if (acl)
> +		posix_acl_release(acl);
> +fail_free_vfs_inode:
>  	free_inode_nonrcu(inode);
>  	inode = NULL;
>  fail_gunlock:
> @@ -1678,10 +1698,11 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  		error = gfs2_setattr_size(inode, attr->ia_size);
>  	else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>  		error = setattr_chown(inode, attr);
> -	else if ((attr->ia_valid & ATTR_MODE) && IS_POSIXACL(inode))
> -		error = gfs2_acl_chmod(ip, attr);
> -	else
> +	else {
>  		error = gfs2_setattr_simple(inode, attr);
> +		if (!error && attr->ia_valid & ATTR_MODE)
> +			error = posix_acl_chmod(inode, inode->i_mode);
> +	}
>  
>  out:
>  	if (!error)
> @@ -1841,6 +1862,7 @@ const struct inode_operations gfs2_file_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  };
>  
>  const struct inode_operations gfs2_dir_iops = {
> @@ -1862,6 +1884,7 @@ const struct inode_operations gfs2_dir_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  	.atomic_open = gfs2_atomic_open,
>  };
>  
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index 8c6a6f6..0b81f78 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -13,6 +13,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/xattr.h>
>  #include <linux/gfs2_ondisk.h>
> +#include <linux/posix_acl_xattr.h>
>  #include <asm/uaccess.h>
>  
>  #include "gfs2.h"
> @@ -1500,7 +1501,8 @@ static const struct xattr_handler gfs2_xattr_security_handler = {
>  const struct xattr_handler *gfs2_xattr_handlers[] = {
>  	&gfs2_xattr_user_handler,
>  	&gfs2_xattr_security_handler,
> -	&gfs2_xattr_system_handler,
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
>  	NULL,
>  };
>  

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Andreas Gruenbacher]

Christoph,

> +struct posix_acl *get_acl(struct inode *inode, int type)
> +{
> +	struct posix_acl *acl;
> +
> +	acl = get_cached_acl(inode, type);
> +	if (acl != ACL_NOT_CACHED)
> +		return acl;
> +
> +	if (!IS_POSIXACL(inode))
> +		return NULL;
> +
> +	/*
> +	 * A filesystem can force a ACL callback by just never filling the
> +	 * ACL cache. But normally you'd fill the cache either at inode
> +	 * instantiation time, or on the first ->get_acl call.
> +	 *
> +	 * If the filesystem doesn't have a get_acl() function at all, we'll
> +	 * just create the negative cache entry.
> +	 */
> +	if (!inode->i_op->get_acl) {
> +		set_cached_acl(inode, type, NULL);
> +		return ERR_PTR(-EAGAIN);

The function should return NULL here.

Andreas

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Andreas Gruenbacher]

Christoph,

> +int
> +posix_acl_chmod(struct inode *inode)
> +{
> +	struct posix_acl *acl;
> +	int ret = 0;
> +
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Andreas

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Andreas Gruenbacher]

Christoph,

> +static int
> +posix_acl_xattr_set(struct dentry *dentry, const char *name,
> +		const void *value, size_t size, int flags, int type)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct posix_acl *acl = NULL;
> +	int ret;
> +
> +	if (!IS_POSIXACL(inode))
> +		return -EOPNOTSUPP;
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Sama here, I would remove the S_ISLNK() check.

Andreas

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Andreas Gruenbacher]

Christoph,

gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
fs/gfs2/inode.c, from a long time ago, which should be removed
as well.

Andreas

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:06:09PM +0100, Andreas Gruenbacher wrote:
> > +	/*
> > +	 * A filesystem can force a ACL callback by just never filling the
> > +	 * ACL cache. But normally you'd fill the cache either at inode
> > +	 * instantiation time, or on the first ->get_acl call.
> > +	 *
> > +	 * If the filesystem doesn't have a get_acl() function at all, we'll
> > +	 * just create the negative cache entry.
> > +	 */
> > +	if (!inode->i_op->get_acl) {
> > +		set_cached_acl(inode, type, NULL);
> > +		return ERR_PTR(-EAGAIN);
> 
> The function should return NULL here.

Indeed.  EAGAIN is the convention check_acl() in fs/namei.c uses,
but it will return that automatically if we just return NULL here.

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:07:20PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> > +int
> > +posix_acl_chmod(struct inode *inode)
> > +{
> > +	struct posix_acl *acl;
> > +	int ret = 0;
> > +
> > +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> > +		return -EOPNOTSUPP;
> 
> Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Yeah, will simplify it.  Same for the other places where we have both
checks.

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:08:38PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
> fs/gfs2/inode.c, from a long time ago, which should be removed
> as well.

Ok, will fix.