Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations

2018-05-15 Thread James Simmons 

> On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> > +#ifdef CONFIG_FS_POSIX_ACL
> >  struct posix_acl *ll_get_acl(struct inode *inode, int type)
> >  {
> > struct ll_inode_info *lli = ll_i2info(inode);
> > @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, 
> > int type)
> > return acl;
> >  }
> >  
> > +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> > +{
> > +   struct ll_sb_info *sbi = ll_i2sbi(inode);
> > +   struct ptlrpc_request *req = NULL;
> > +   const char *name = NULL;
> > +   size_t value_size = 0;
> > +   char *value = NULL;
> > +   int rc;
> 
> "rc" needs to be initialized to zero.  It's disapppointing that GCC
> doesn't catch this.

Thanks Dan. Will fix.
 
> > +
> > +   switch (type) {
> > +   case ACL_TYPE_ACCESS:
> > +   name = XATTR_NAME_POSIX_ACL_ACCESS;
> > +   if (acl)
> > +   rc = posix_acl_update_mode(inode, >i_mode, );
> > +   break;
> > +
> > +   case ACL_TYPE_DEFAULT:
> > +   name = XATTR_NAME_POSIX_ACL_DEFAULT;
> > +   if (!S_ISDIR(inode->i_mode))
> > +   rc = acl ? -EACCES : 0;
> > +   break;
> > +
> > +   default:
> > +   rc = -EINVAL;
> > +   break;
> > +   }
> > +   if (rc)
> > +   return rc;
> 
> Otherwise rc can be uninitialized here.
> 
> regards,
> dan carpenter
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations

2018-05-15 Thread Dan Carpenter 
On Mon, May 14, 2018 at 10:16:59PM -0400, James Simmons wrote:
> +#ifdef CONFIG_FS_POSIX_ACL
>  struct posix_acl *ll_get_acl(struct inode *inode, int type)
>  {
>   struct ll_inode_info *lli = ll_i2info(inode);
> @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int 
> type)
>   return acl;
>  }
>  
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> + struct ll_sb_info *sbi = ll_i2sbi(inode);
> + struct ptlrpc_request *req = NULL;
> + const char *name = NULL;
> + size_t value_size = 0;
> + char *value = NULL;
> + int rc;

"rc" needs to be initialized to zero.  It's disapppointing that GCC
doesn't catch this.

> +
> + switch (type) {
> + case ACL_TYPE_ACCESS:
> + name = XATTR_NAME_POSIX_ACL_ACCESS;
> + if (acl)
> + rc = posix_acl_update_mode(inode, >i_mode, );
> + break;
> +
> + case ACL_TYPE_DEFAULT:
> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> + if (!S_ISDIR(inode->i_mode))
> + rc = acl ? -EACCES : 0;
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> + if (rc)
> + return rc;

Otherwise rc can be uninitialized here.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations

2018-05-15 Thread Greg Kroah-Hartman 
On Tue, May 15, 2018 at 01:53:02PM +1000, NeilBrown wrote:
> On Mon, May 14 2018, James Simmons wrote:
> 
> > From: Dmitry Eremin 
> >
> > Linux kernel v3.14 adds set_acl method to inode operations.
> > This patch adds support to Lustre for proper acl management.
> >
> > Signed-off-by: Dmitry Eremin 
> > Signed-off-by: John L. Hammond 
> > Signed-off-by: James Simmons 
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> > Reviewed-on: https://review.whamcloud.com/25965
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> > Reviewed-on: https://review.whamcloud.com/31588
> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> > Reviewed-on: https://review.whamcloud.com/32045
> > Reviewed-by: Bob Glossman 
> > Reviewed-by: James Simmons 
> > Reviewed-by: Andreas Dilger 
> > Reviewed-by: Dmitry Eremin 
> > Reviewed-by: Oleg Drokin 
> > Signed-off-by: James Simmons 
> > ---
> > Changelog:
> >
> > v1) Initial patch ported to staging tree
> > v2) Fixed up goto handling and avoid BUG() when calling
> > forget_cached_acl()with invalid type as pointed out by Dan Carpenter
> >
> >  drivers/staging/lustre/lustre/llite/file.c | 62 
> > ++
> >  .../staging/lustre/lustre/llite/llite_internal.h   |  4 ++
> >  drivers/staging/lustre/lustre/llite/namei.c| 10 +++-
> >  3 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c 
> > b/drivers/staging/lustre/lustre/llite/file.c
> > index 0026fde..64a5698 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct 
> > fiemap_extent_info *fieinfo,
> > return rc;
> >  }
> >  
> > +#ifdef CONFIG_FS_POSIX_ACL
> 
> Using #ifdef in  .c files is generally discouraged.
> The "standard" approach here is:
> - put the acl code in a separate file (acl.c)
> - optionally include it via the Make file
>  lustre-$(CONFIG_FS_POSIX_ACL) += acl.o
> 
> - in the header where ll_get_acl and ll_set_acl are declared have
>  #ifdef CONFIG_FS_POSIX_ACL
>declare the functions
>  #else
>  #define ll_get_acl NULL
>  #define ll_set_acl NULL
>  #endif
> 
> Now as this is staging and that is (presumably) an upstream patch
> lightly improved it is probably fine to include the patch as-is,
> but in that case we will probably want to fix it up later.

Let's get it right the first time if at all possible please.

I'll drop this series from my queue and wait for the next version of it.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl method in inode operations

2018-05-14 Thread NeilBrown 
On Mon, May 14 2018, James Simmons wrote:

> From: Dmitry Eremin 
>
> Linux kernel v3.14 adds set_acl method to inode operations.
> This patch adds support to Lustre for proper acl management.
>
> Signed-off-by: Dmitry Eremin 
> Signed-off-by: John L. Hammond 
> Signed-off-by: James Simmons 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-9183
> Reviewed-on: https://review.whamcloud.com/25965
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10541
> Reviewed-on: https://review.whamcloud.com/31588
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-10926
> Reviewed-on: https://review.whamcloud.com/32045
> Reviewed-by: Bob Glossman 
> Reviewed-by: James Simmons 
> Reviewed-by: Andreas Dilger 
> Reviewed-by: Dmitry Eremin 
> Reviewed-by: Oleg Drokin 
> Signed-off-by: James Simmons 
> ---
> Changelog:
>
> v1) Initial patch ported to staging tree
> v2) Fixed up goto handling and avoid BUG() when calling
> forget_cached_acl()with invalid type as pointed out by Dan Carpenter
>
>  drivers/staging/lustre/lustre/llite/file.c | 62 
> ++
>  .../staging/lustre/lustre/llite/llite_internal.h   |  4 ++
>  drivers/staging/lustre/lustre/llite/namei.c| 10 +++-
>  3 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/file.c 
> b/drivers/staging/lustre/lustre/llite/file.c
> index 0026fde..64a5698 100644
> --- a/drivers/staging/lustre/lustre/llite/file.c
> +++ b/drivers/staging/lustre/lustre/llite/file.c
> @@ -3030,6 +3030,7 @@ static int ll_fiemap(struct inode *inode, struct 
> fiemap_extent_info *fieinfo,
>   return rc;
>  }
>  
> +#ifdef CONFIG_FS_POSIX_ACL

Using #ifdef in  .c files is generally discouraged.
The "standard" approach here is:
- put the acl code in a separate file (acl.c)
- optionally include it via the Make file
 lustre-$(CONFIG_FS_POSIX_ACL) += acl.o

- in the header where ll_get_acl and ll_set_acl are declared have
 #ifdef CONFIG_FS_POSIX_ACL
   declare the functions
 #else
 #define ll_get_acl NULL
 #define ll_set_acl NULL
 #endif

Now as this is staging and that is (presumably) an upstream patch
lightly improved it is probably fine to include the patch as-is,
but in that case we will probably want to fix it up later.

Thanks,
NeilBrown

>  struct posix_acl *ll_get_acl(struct inode *inode, int type)
>  {
>   struct ll_inode_info *lli = ll_i2info(inode);
> @@ -3043,6 +3044,64 @@ struct posix_acl *ll_get_acl(struct inode *inode, int 
> type)
>   return acl;
>  }
>  
> +int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> + struct ll_sb_info *sbi = ll_i2sbi(inode);
> + struct ptlrpc_request *req = NULL;
> + const char *name = NULL;
> + size_t value_size = 0;
> + char *value = NULL;
> + int rc;
> +
> + switch (type) {
> + case ACL_TYPE_ACCESS:
> + name = XATTR_NAME_POSIX_ACL_ACCESS;
> + if (acl)
> + rc = posix_acl_update_mode(inode, >i_mode, );
> + break;
> +
> + case ACL_TYPE_DEFAULT:
> + name = XATTR_NAME_POSIX_ACL_DEFAULT;
> + if (!S_ISDIR(inode->i_mode))
> + rc = acl ? -EACCES : 0;
> + break;
> +
> + default:
> + rc = -EINVAL;
> + break;
> + }
> + if (rc)
> + return rc;
> +
> + if (acl) {
> + value_size = posix_acl_xattr_size(acl->a_count);
> + value = kmalloc(value_size, GFP_NOFS);
> + if (!value) {
> + rc = -ENOMEM;
> + goto out;
> + }
> +
> + rc = posix_acl_to_xattr(_user_ns, acl, value, value_size);
> + if (rc < 0)
> + goto out_value;
> + }
> +
> + rc = md_setxattr(sbi->ll_md_exp, ll_inode2fid(inode),
> +  value ? OBD_MD_FLXATTR : OBD_MD_FLXATTRRM,
> +  name, value, value_size, 0, 0, 0, );
> +
> + ptlrpc_req_finished(req);
> +out_value:
> + kfree(value);
> +out:
> + if (rc)
> + forget_cached_acl(inode, type);
> + else
> + set_cached_acl(inode, type, acl);
> + return rc;
> +}
> +#endif /* CONFIG_FS_POSIX_ACL */
> +
>  int ll_inode_permission(struct inode *inode, int mask)
>  {
>   struct ll_sb_info *sbi;
> @@ -3164,7 +3223,10 @@ int ll_inode_permission(struct inode *inode, int mask)
>   .permission = ll_inode_permission,
>   .listxattr  = ll_listxattr,
>   .fiemap = ll_fiemap,
> +#ifdef CONFIG_FS_POSIX_ACL
>   .get_acl= ll_get_acl,
> + .set_acl= ll_set_acl,
> +#endif
>  };
>  
>  /* dynamic ioctl number support routines */
> diff --git