Re: [RFC] POSIX ACL kernel infrastructure

On Sunday 04 August 2002 16:33, Christoph Hellwig wrote:

On Sun, Aug 04, 2002 at 04:14:47PM +0200, Andreas Gruenbacher wrote:

quoted

+MODULE_AUTHOR("Andreas Gruenbacher [off-list ref]");
+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.

quoted

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

quoted

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

quoted

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

quoted

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

this patch _is_ for 2.5, isn't it?

Removed.

quoted

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.

quoted

+#ifdef __KERNEL__

why the __KERNEL__?

Not needed anymore.

quoted

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