linux-kernel - Re: [PATCH v2 1/5] staging: lustre: llite: add support set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 15 May 2018 09:30:09 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     NeilBrown <neilb@...e.com>
Cc:     James Simmons <jsimmons@...radead.org>, devel@...verdev.osuosl.org,
        Andreas Dilger <andreas.dilger@...el.com>,
        Oleg Drokin <oleg.drokin@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lustre Development List <lustre-devel@...ts.lustre.org>,
        Dmitry Eremin <dmitry.eremin@...el.com>,
        "John L. Hammond" <john.hammond@...el.com>,
        James Simmons <uja.ornl@...oo.com>
Subject: Re: [PATCH v2 1/5] staging: lustre: llite: add support set_acl
 method in inode operations

On Tue, May 15, 2018 at 01:53:02PM +1000, NeilBrown wrote:
> On Mon, May 14 2018, James Simmons wrote:
> 
> > From: Dmitry Eremin <dmitry.eremin@...el.com>
> >
> > 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 <dmitry.eremin@...el.com>
> > Signed-off-by: John L. Hammond <john.hammond@...el.com>
> > Signed-off-by: James Simmons <uja.ornl@...oo.com>
> > 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 <bob.glossman@...el.com>
> > Reviewed-by: James Simmons <uja.ornl@...oo.com>
> > Reviewed-by: Andreas Dilger <andreas.dilger@...el.com>
> > Reviewed-by: Dmitry Eremin <dmitry.eremin@...el.com>
> > Reviewed-by: Oleg Drokin <oleg.drokin@...el.com>
> > Signed-off-by: James Simmons <jsimmons@...radead.org>
> > ---
> > 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