subject:"\[PATCH v2 1\/5\] staging\: lustre\: llite\: add support set

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

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

2018-05-14 Thread James Simmons

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
 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 a/drivers/staging/lustre/lustre/llite/llite_internal.h 
b/drivers/staging/lustre/lustre/llite/llite_internal.h
index 6504850..2280327 100644
--- a/drivers/staging/lustre/lustre/llite/llite_internal.h
+++ b/drivers/staging/lustre/lustre/llite/llite_internal.h
@@ -754,7 +754,11 @@ enum ldlm_mode ll_take_md_lock(struct inode *inode, __u64 
bits,
 int ll_md_real_close(struct inode *inode, fmode_t fmode);
 int ll_getattr(const struct path *path, struct kstat *stat,
   u32 request_mask, unsigned int flags);
+#ifdef CONFIG_FS_POSIX_ACL
 struct posix_acl *ll_get_acl(struct inode *inode, int type);
+int ll_set_acl(struct inode *inode, struct posix_acl *acl, int type);
+#endif /* CONFIG_FS_POSIX_ACL */
+
 int ll_migrate(struct inode *parent, struct file *file, int mdtidx,
   const char *name, int

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information