From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [RFC] POSIX ACL kernel infrastructure
Date: Sun, 4 Aug 2002 15:33:49 +0100
Sender: linux-fsdevel-owner@vger.kernel.org
Message-ID: <20020804153349.A28109@infradead.org>
References: <200208041546.35234.agruen@suse.de> <20020804150407.A27593@infradead.org> <200208041614.47152.agruen@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Linux-FSDevel <linux-fsdevel@vger.kernel.org>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
To: Andreas Gruenbacher <agruen@suse.de>
Content-Disposition: inline
In-Reply-To: <200208041614.47152.agruen@suse.de>; from agruen@suse.de on Sun, Aug 04, 2002 at 04:14:47PM +0200
List-Id: linux-fsdevel.vger.kernel.org

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.

> +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 filsystem transactions?
passing a gfp flag down seems like a good idea to me.

> +/*
> + * 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?

> +/*
> + * 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.

> +	down(&inode->i_sem);
> +	lock_kernel();  /* goes away in 2.5.x */

this patch _is_ for 2.5, isn't it?

> 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 osix_acl
instead of posix_acl_t we wouldn't have to bloat fs.h with yet another
indirect header..

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?
 
> +#ifdef __KERNEL__

why the __KERNEL__?  

> +/* pxacl.c */

Shouldn't this be posix_acl.c?