Re: [RFC] POSIX ACL kernel infrastructure

[Andreas Gruenbacher <agruen@suse.de>]

On Sunday 04 August 2002 16:33, Christoph Hellwig wrote:
> On Sun, Aug 04, 2002 at 04:14:47PM +0200, Andreas Gruenbacher wrote:
> > +MODULE_AUTHOR("Andreas Gruenbacher <a.gruenbacher@computer.org>");
> > +MODULE_DESCRIPTION("Generic Posix Access Control List (ACL)
> > Manipulation"); +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,0)
> > +MODULE_LICENSE("GPL");
> > +#endif
>
> MODULE_LICENSE was new in 2.4.9 or 2.4.10, but certainly not present in
> 2.4.1..  I don't think kernel version checks in core code are a good idea
> though.  especially if you aim for inclusion.

It was 2.4.10; I've canged that. I wanted to use identical code from 2.2 to 
2.4; the non-2.5 parts can be stripped off.

> > +posix_acl_t *
> > +posix_acl_alloc(int count)
> > +{
> > +	const size_t size = sizeof(posix_acl_t) +
> > +	                    count * sizeof(posix_acl_entry_t);
> > +	posix_acl_t *acl = kmalloc(size, GFP_KERNEL);
> > +	if (acl) {
> > +		atomic_set(&acl->a_refcount, 1);
> > +		acl->a_count = count;
> > +	}
> > +	return acl;
>
> are you sure this is never called from filesystem transactions?
> passing a gfp flag down seems like a good idea to me.

Probably, yes.

> > +/*
> > + * Duplicate an ACL handle.
> > + */
> > +posix_acl_t *
> > +posix_acl_dup(posix_acl_t *acl)
> > +{
> > +	if (acl)
> > +		atomic_inc(&acl->a_refcount);
> > +	return acl;
> > +}
>
> Make this an inline in a header?  can acl really be NULL?

Yes it can, meaning `there are only the file permission bits'.

> > +/*
> > + * Get the POSIX ACL of an inode.
> > + */
> > +posix_acl_t *
> > +get_posix_acl(struct inode *inode, int type)
> > +{
> > +	posix_acl_t *acl;
> > +
> > +	if (!inode->i_op || !inode->i_op->get_posix_acl)
> > +		return ERR_PTR(-ENOTSUP);
>
> inode->i_op is never NULL.

Changed.

> > +	down(&inode->i_sem);
> > +	lock_kernel();  /* goes away in 2.5.x */
>
> this patch _is_ for 2.5, isn't it?

Removed.

> > linux-2.5.30.patch/include/linux/fs.h
> > --- linux-2.5.30/include/linux/fs.h	Thu Aug  1 23:16:15 2002
> > +++ linux-2.5.30.patch/include/linux/fs.h	Sun Aug  4 13:29:31 2002
> > @@ -23,6 +23,7 @@
> >  #include <linux/string.h>
> >  #include <linux/radix-tree.h>
> >  #include <linux/bitops.h>
> > +#include <linux/posix_acl.h>
> >
> >  #include <asm/atomic.h>
> >
> > @@ -787,6 +788,8 @@
> >  	ssize_t (*getxattr) (struct dentry *, const char *, void *, size_t);
> >  	ssize_t (*listxattr) (struct dentry *, char *, size_t);
> >  	int (*removexattr) (struct dentry *, const char *);
> > +	posix_acl_t *(*get_posix_acl) (struct inode *, int);
> > +	int (*set_posix_acl) (struct inode *, int, posix_acl_t *);
>
> If you had followed Documentation/CodingSyle and use struct posix_acl
> instead of posix_acl_t we wouldn't have to bloat fs.h with yet another
> indirect header..

Using `struct posix_acl' everywhere is so much more messy. Is an exception 
justified here? The core kernel code has typedefs all over the place.

> Also what exactly are get_posix_acl/set_posix_acl for?  We have wrappers
> for them in fs/posix_acl.c, but even in your 2.4 patch only get_posix_acl
> is ever used.  Shouldn't we always set/get posix ACLs through the xattr
> inode operations?

>From user space, yes. The get_posix_acl operation is currently used in nfsd; 
going via the xattr operations would be too expensive here. The set_posix_acl 
operation is indeed not used so far; I think it makes sense to add it for 
completeness' sake.

>
> > +#ifdef __KERNEL__
>
> why the __KERNEL__?

Not needed anymore.

> > +/* pxacl.c */
>
> Shouldn't this be posix_acl.c?

Yes.

I have put up an updated version at 
<http://acl.bestbits.at/pre/2.5/linux-2.5.30-posix-acl-1.diff>.

Regards,
Andreas.

------------------------------------------------------------------
 Andreas Gruenbacher                                SuSE Linux AG
 mailto:agruen@suse.de                     Deutschherrnstr. 15-19
 http://www.suse.de/                   D-90429 Nuernberg, Germany