[PATCH] use generic_*xattr routines

[PATCH] use generic_*xattr routines

[Christoph Hellwig]

Use the generic set, get and removexattr methods and supply the s_xattr
array with fine-grained handlers.  All XFS/Linux highlevel attr handling is
rewritten from scratch and placed into fs/xfs/linux-2.6/xfs_xattr.c so
that it's separated from the generic low-level code.

The code size reduction is not as big as I had hoped, but it's still a
worthwile cleanup.

I didn't managed to get rid of struct attrnames yet, as it's still used
to hack the Linux string prefixes into the output inside the lowest
level xattr code.  I have some plans to clean that bit up aswell, but
that will have to wait for a while.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/Makefile
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/Makefile	2008-04-30 08:24:31.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/Makefile	2008-04-30 08:43:49.000000000 +0200
@@ -97,6 +97,7 @@ xfs-y				+= $(addprefix $(XFS_LINUX)/, \
 				   xfs_lrw.o \
 				   xfs_super.o \
 				   xfs_vnode.o \
+				   xfs_xattr.o \
 				   xfs_ksyms.o) 
 
 # Objects in support/
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-04-30 08:35:43.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-04-30 08:43:49.000000000 +0200
@@ -282,7 +282,7 @@ xfs_vn_mknod(
 	struct xfs_inode *ip = NULL;
 	xfs_acl_t	*default_acl = NULL;
 	struct xfs_name	name;
-	attrexists_t	test_default_acl = _ACL_DEFAULT_EXISTS;
+	int (*test_default_acl)(struct inode *) = _ACL_DEFAULT_EXISTS;
 	int		error;
 
 	/*
@@ -735,98 +735,6 @@ xfs_vn_truncate(
 	WARN_ON(error);
 }
 
-STATIC int
-xfs_vn_setxattr(
-	struct dentry	*dentry,
-	const char	*name,
-	const void	*data,
-	size_t		size,
-	int		flags)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	/* Convert Linux syscall to XFS internal ATTR flags */
-	if (flags & XATTR_CREATE)
-		xflags |= ATTR_CREATE;
-	if (flags & XATTR_REPLACE)
-		xflags |= ATTR_REPLACE;
-	xflags |= namesp->attr_flag;
-	return namesp->attr_set(vp, attr, (void *)data, size, xflags);
-}
-
-STATIC ssize_t
-xfs_vn_getxattr(
-	struct dentry	*dentry,
-	const char	*name,
-	void		*data,
-	size_t		size)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	/* Convert Linux syscall to XFS internal ATTR flags */
-	if (!size) {
-		xflags |= ATTR_KERNOVAL;
-		data = NULL;
-	}
-	xflags |= namesp->attr_flag;
-	return namesp->attr_get(vp, attr, (void *)data, size, xflags);
-}
-
-STATIC ssize_t
-xfs_vn_listxattr(
-	struct dentry		*dentry,
-	char			*data,
-	size_t			size)
-{
-	bhv_vnode_t		*vp = vn_from_inode(dentry->d_inode);
-	int			error, xflags = ATTR_KERNAMELS;
-	ssize_t			result;
-
-	if (!size)
-		xflags |= ATTR_KERNOVAL;
-	xflags |= capable(CAP_SYS_ADMIN) ? ATTR_KERNFULLS : ATTR_KERNORMALS;
-
-	error = attr_generic_list(vp, data, size, xflags, &result);
-	if (error < 0)
-		return error;
-	return result;
-}
-
-STATIC int
-xfs_vn_removexattr(
-	struct dentry	*dentry,
-	const char	*name)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	xflags |= namesp->attr_flag;
-	return namesp->attr_remove(vp, attr, xflags);
-}
-
 STATIC long
 xfs_vn_fallocate(
 	struct inode	*inode,
@@ -874,10 +782,10 @@ const struct inode_operations xfs_inode_
 	.truncate		= xfs_vn_truncate,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 	.fallocate		= xfs_vn_fallocate,
 };
 
@@ -894,10 +802,10 @@ const struct inode_operations xfs_dir_in
 	.permission		= xfs_vn_permission,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 };
 
 const struct inode_operations xfs_symlink_inode_operations = {
@@ -907,8 +815,8 @@ const struct inode_operations xfs_symlin
 	.permission		= xfs_vn_permission,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 };
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c	2008-04-30 12:55:33.000000000 +0200
@@ -0,0 +1,266 @@
+/*
+ * Copyright (C) 2008 Christoph Hellwig.
+ *	Released under GPL v2.
+ */
+
+#include "xfs.h"
+#include "xfs_attr.h"
+#include "xfs_acl.h"
+#include "xfs_vnodeops.h"
+
+#include <linux/posix_acl_xattr.h>
+#include <linux/xattr.h>
+
+
+/*
+ * ACL handling.  Should eventually be moved into xfs_acl.c
+ */
+
+static int
+xfs_decode_acl(const char *name)
+{
+	if (strcmp(name, "posix_acl_access") == 0)
+		return _ACL_TYPE_ACCESS;
+	else if (strcmp(name, "posix_acl_default") == 0)
+		return _ACL_TYPE_DEFAULT;
+	return -EINVAL;
+}
+
+static int
+xfs_xattr_system_get(struct inode *inode, const char *name,
+		void *buffer, size_t size)
+{
+	int acl;
+
+	acl = xfs_decode_acl(name);
+	if (acl < 0)
+		return acl;
+
+	return xfs_acl_vget(inode, buffer, size, acl);
+}
+
+static int
+xfs_xattr_system_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	int error, acl;
+
+	acl = xfs_decode_acl(name);
+	if (acl < 0)
+		return acl;
+	if (flags & XATTR_CREATE)
+		return -EINVAL;
+
+	if (!value)
+		return xfs_acl_vremove(inode, acl);
+
+	error = xfs_acl_vset(inode, (void *)value, size, acl);
+	if (!error)
+		vn_revalidate(inode);
+	return error;
+}
+
+static struct xattr_handler xfs_xattr_system_handler = {
+	.prefix	= XATTR_SYSTEM_PREFIX,
+	.get	= xfs_xattr_system_get,
+	.set	= xfs_xattr_system_set,
+};
+
+
+/*
+ * Real xattr handling.  The only difference between the namespaces is
+ * a flag passed to the low-level attr code.
+ */
+
+static int
+__xfs_xattr_get(struct inode *inode, const char *name,
+		void *value, size_t size, int xflags)
+{
+	struct xfs_inode *ip = XFS_I(inode);
+	int error, asize = size;
+
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+
+	/* Convert Linux syscall to XFS internal ATTR flags */
+	if (!size) {
+		xflags |= ATTR_KERNOVAL;
+		value = NULL;
+	}
+
+	error = -xfs_attr_get(ip, name, value, &asize, xflags);
+	if (error)
+		return error;
+	return asize;
+}
+
+static int
+__xfs_xattr_set(struct inode *inode, const char *name, const void *value,
+		size_t size, int flags, int xflags)
+{
+	struct xfs_inode *ip = XFS_I(inode);
+
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+
+	/* Convert Linux syscall to XFS internal ATTR flags */
+	if (flags & XATTR_CREATE)
+		xflags |= ATTR_CREATE;
+	if (flags & XATTR_REPLACE)
+		xflags |= ATTR_REPLACE;
+
+	if (!value)
+		return -xfs_attr_remove(ip, name, xflags);
+	return -xfs_attr_set(ip, name, (void *)value, size, xflags);
+}
+
+static int
+xfs_xattr_user_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, 0);
+}
+
+static int
+xfs_xattr_user_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, 0);
+}
+
+struct attrnames attr_user = {
+	.attr_name	= "user.",
+	.attr_namelen	= sizeof("user.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_user_handler = {
+	.prefix	= XATTR_USER_PREFIX,
+	.get	= xfs_xattr_user_get,
+	.set	= xfs_xattr_user_set,
+};
+
+
+static int
+xfs_xattr_trusted_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, ATTR_ROOT);
+}
+
+static int
+xfs_xattr_trusted_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_ROOT);
+}
+
+struct attrnames attr_trusted = {
+	.attr_name	= "trusted.",
+	.attr_namelen	= sizeof("trusted.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_trusted_handler = {
+	.prefix	= XATTR_TRUSTED_PREFIX,
+	.get	= xfs_xattr_trusted_get,
+	.set	= xfs_xattr_trusted_set,
+};
+
+
+static int
+xfs_xattr_secure_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, ATTR_SECURE);
+}
+
+static int
+xfs_xattr_secure_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_SECURE);
+}
+
+struct attrnames attr_secure = {
+	.attr_name	= "security.",
+	.attr_namelen	= sizeof("security.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_security_handler = {
+	.prefix	= XATTR_SECURITY_PREFIX,
+	.get	= xfs_xattr_secure_get,
+	.set	= xfs_xattr_secure_set,
+};
+
+
+struct xattr_handler *xfs_xattr_handlers[] = {
+	&xfs_xattr_user_handler,
+	&xfs_xattr_trusted_handler,
+	&xfs_xattr_security_handler,
+	&xfs_xattr_system_handler,
+	NULL
+};
+
+static int
+list_one_attr(const char *name, const size_t len, void *data,
+		size_t size, ssize_t *result)
+{
+	char *p = data + *result;
+
+	*result += len;
+	if (!size)
+		return 0;
+	if (*result > size)
+		return -ERANGE;
+
+	strcpy(p, name);
+	p += len;
+	return 0;
+}
+
+ssize_t
+xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
+{
+	struct inode		*inode = dentry->d_inode;
+	struct xfs_inode	*ip = XFS_I(inode);
+	attrlist_cursor_kern_t	cursor = { 0 };
+	int			error, xflags;
+	ssize_t			result;
+
+	xflags = ATTR_KERNAMELS;
+	if (!size)
+		xflags |= ATTR_KERNOVAL;
+
+	if (capable(CAP_SYS_ADMIN))
+		xflags |= ATTR_KERNFULLS;
+	else
+		xflags |= ATTR_KERNORMALS;
+
+
+	/*
+	 * First read the regular on-disk attributes.
+	 */
+	result = -xfs_attr_list(ip, data, size, xflags, &cursor);
+	if (result < 0)
+		return result;
+
+	/*
+	 * Then add the two synthetic ACL attributes.
+	 */
+	if (xfs_acl_vhasacl_access(inode)) {
+		error = list_one_attr(POSIX_ACL_XATTR_ACCESS,
+				strlen(POSIX_ACL_XATTR_ACCESS) + 1,
+				data, size, &result);
+		if (error)
+			return error;
+	}
+
+	if (xfs_acl_vhasacl_default(inode)) {
+		error = list_one_attr(POSIX_ACL_XATTR_DEFAULT,
+				strlen(POSIX_ACL_XATTR_DEFAULT) + 1,
+				data, size, &result);
+		if (error)
+			return error;
+	}
+
+	return result;
+}
Index: linux-2.6-xfs/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_attr.c	2008-04-30 08:35:43.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_attr.c	2008-04-30 08:43:49.000000000 +0200
@@ -57,11 +57,6 @@
  * Provide the external interfaces to manage attribute lists.
  */
 
-#define ATTR_SYSCOUNT	2
-static struct attrnames posix_acl_access;
-static struct attrnames posix_acl_default;
-static struct attrnames *attr_system_names[ATTR_SYSCOUNT];
-
 /*========================================================================
  * Function prototypes for the kernel.
  *========================================================================*/
@@ -2435,270 +2430,3 @@ xfs_attr_trace_enter(int type, char *whe
 					 (void *)a14, (void *)a15);
 }
 #endif	/* XFS_ATTR_TRACE */
-
-
-/*========================================================================
- * System (pseudo) namespace attribute interface routines.
- *========================================================================*/
-
-STATIC int
-posix_acl_access_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vset(vp, data, size, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return xfs_acl_vremove(vp, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vget(vp, data, size, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_exists(
-	bhv_vnode_t *vp)
-{
-	return xfs_acl_vhasacl_access(vp);
-}
-
-STATIC int
-posix_acl_default_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vset(vp, data, size, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vget(vp, data, size, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return xfs_acl_vremove(vp, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_exists(
-	bhv_vnode_t *vp)
-{
-	return xfs_acl_vhasacl_default(vp);
-}
-
-static struct attrnames posix_acl_access = {
-	.attr_name	= "posix_acl_access",
-	.attr_namelen	= sizeof("posix_acl_access") - 1,
-	.attr_get	= posix_acl_access_get,
-	.attr_set	= posix_acl_access_set,
-	.attr_remove	= posix_acl_access_remove,
-	.attr_exists	= posix_acl_access_exists,
-};
-
-static struct attrnames posix_acl_default = {
-	.attr_name	= "posix_acl_default",
-	.attr_namelen	= sizeof("posix_acl_default") - 1,
-	.attr_get	= posix_acl_default_get,
-	.attr_set	= posix_acl_default_set,
-	.attr_remove	= posix_acl_default_remove,
-	.attr_exists	= posix_acl_default_exists,
-};
-
-static struct attrnames *attr_system_names[] =
-	{ &posix_acl_access, &posix_acl_default };
-
-
-/*========================================================================
- * Namespace-prefix-style attribute name interface routines.
- *========================================================================*/
-
-STATIC int
-attr_generic_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return -xfs_attr_set(xfs_vtoi(vp), name, data, size, xflags);
-}
-
-STATIC int
-attr_generic_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	int	error, asize = size;
-
-	error = xfs_attr_get(xfs_vtoi(vp), name, data, &asize, xflags);
-	if (!error)
-		return asize;
-	return -error;
-}
-
-STATIC int
-attr_generic_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return -xfs_attr_remove(xfs_vtoi(vp), name, xflags);
-}
-
-STATIC int
-attr_generic_listadd(
-	attrnames_t		*prefix,
-	attrnames_t		*namesp,
-	void			*data,
-	size_t			size,
-	ssize_t			*result)
-{
-	char			*p = data + *result;
-
-	*result += prefix->attr_namelen;
-	*result += namesp->attr_namelen + 1;
-	if (!size)
-		return 0;
-	if (*result > size)
-		return -ERANGE;
-	strcpy(p, prefix->attr_name);
-	p += prefix->attr_namelen;
-	strcpy(p, namesp->attr_name);
-	p += namesp->attr_namelen + 1;
-	return 0;
-}
-
-STATIC int
-attr_system_list(
-	bhv_vnode_t		*vp,
-	void			*data,
-	size_t			size,
-	ssize_t			*result)
-{
-	attrnames_t		*namesp;
-	int			i, error = 0;
-
-	for (i = 0; i < ATTR_SYSCOUNT; i++) {
-		namesp = attr_system_names[i];
-		if (!namesp->attr_exists || !namesp->attr_exists(vp))
-			continue;
-		error = attr_generic_listadd(&attr_system, namesp,
-						data, size, result);
-		if (error)
-			break;
-	}
-	return error;
-}
-
-int
-attr_generic_list(
-	bhv_vnode_t *vp, void *data, size_t size, int xflags, ssize_t *result)
-{
-	attrlist_cursor_kern_t	cursor = { 0 };
-	int			error;
-
-	error = xfs_attr_list(xfs_vtoi(vp), data, size, xflags, &cursor);
-	if (error > 0)
-		return -error;
-	*result = -error;
-	return attr_system_list(vp, data, size, result);
-}
-
-attrnames_t *
-attr_lookup_namespace(
-	char			*name,
-	struct attrnames	**names,
-	int			nnames)
-{
-	int			i;
-
-	for (i = 0; i < nnames; i++)
-		if (!strncmp(name, names[i]->attr_name, names[i]->attr_namelen))
-			return names[i];
-	return NULL;
-}
-
-STATIC int
-attr_system_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	attrnames_t	*namesp;
-	int		error;
-
-	if (xflags & ATTR_CREATE)
-		return -EINVAL;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	error = namesp->attr_set(vp, name, data, size, xflags);
-	if (!error)
-		error = vn_revalidate(vp);
-	return error;
-}
-
-STATIC int
-attr_system_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	attrnames_t	*namesp;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	return namesp->attr_get(vp, name, data, size, xflags);
-}
-
-STATIC int
-attr_system_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	attrnames_t	*namesp;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	return namesp->attr_remove(vp, name, xflags);
-}
-
-struct attrnames attr_system = {
-	.attr_name	= "system.",
-	.attr_namelen	= sizeof("system.") - 1,
-	.attr_flag	= ATTR_SYSTEM,
-	.attr_get	= attr_system_get,
-	.attr_set	= attr_system_set,
-	.attr_remove	= attr_system_remove,
-};
-
-struct attrnames attr_trusted = {
-	.attr_name	= "trusted.",
-	.attr_namelen	= sizeof("trusted.") - 1,
-	.attr_flag	= ATTR_ROOT,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames attr_secure = {
-	.attr_name	= "security.",
-	.attr_namelen	= sizeof("security.") - 1,
-	.attr_flag	= ATTR_SECURE,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames attr_user = {
-	.attr_name	= "user.",
-	.attr_namelen	= sizeof("user.") - 1,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames *attr_namespaces[] =
-	{ &attr_system, &attr_trusted, &attr_secure, &attr_user };
Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h	2008-04-30 08:35:43.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_attr.h	2008-04-30 08:43:49.000000000 +0200
@@ -38,30 +38,14 @@
 struct cred;
 struct xfs_attr_list_context;
 
-typedef int (*attrset_t)(bhv_vnode_t *, char *, void *, size_t, int);
-typedef int (*attrget_t)(bhv_vnode_t *, char *, void *, size_t, int);
-typedef int (*attrremove_t)(bhv_vnode_t *, char *, int);
-typedef int (*attrexists_t)(bhv_vnode_t *);
-
 typedef struct attrnames {
 	char *		attr_name;
 	unsigned int	attr_namelen;
-	unsigned int	attr_flag;
-	attrget_t	attr_get;
-	attrset_t	attr_set;
-	attrremove_t	attr_remove;
-	attrexists_t	attr_exists;
 } attrnames_t;
 
-#define ATTR_NAMECOUNT	4
 extern struct attrnames attr_user;
 extern struct attrnames attr_secure;
-extern struct attrnames attr_system;
 extern struct attrnames attr_trusted;
-extern struct attrnames *attr_namespaces[ATTR_NAMECOUNT];
-
-extern attrnames_t *attr_lookup_namespace(char *, attrnames_t **, int);
-extern int attr_generic_list(bhv_vnode_t *, void *, size_t, int, ssize_t *);
 
 #define ATTR_DONTFOLLOW	0x0001	/* -- unused, from IRIX -- */
 #define ATTR_ROOT	0x0002	/* use attrs in root (trusted) namespace */
@@ -69,7 +53,6 @@ extern int attr_generic_list(bhv_vnode_t
 #define ATTR_SECURE	0x0008	/* use attrs in security namespace */
 #define ATTR_CREATE	0x0010	/* pure create: fail if attr already exists */
 #define ATTR_REPLACE	0x0020	/* pure set: fail if attr does not exist */
-#define ATTR_SYSTEM	0x0100	/* use attrs in system (pseudo) namespace */
 
 #define ATTR_KERNACCESS	0x0400	/* [kernel] iaccess, inode held io-locked */
 #define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.h	2008-04-30 08:24:31.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h	2008-04-30 08:43:49.000000000 +0200
@@ -26,6 +26,7 @@ extern const struct file_operations xfs_
 extern const struct file_operations xfs_dir_file_operations;
 extern const struct file_operations xfs_invis_file_operations;
 
+extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
 
 struct xfs_inode;
 extern void xfs_ichgtime(struct xfs_inode *, int);
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2008-04-30 08:24:32.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c	2008-04-30 08:43:49.000000000 +0200
@@ -1328,6 +1328,7 @@ xfs_fs_fill_super(
 		goto fail_vfsop;
 
 	sb_min_blocksize(sb, BBSIZE);
+	sb->s_xattr = xfs_xattr_handlers;
 	sb->s_export_op = &xfs_export_operations;
 	sb->s_qcop = &xfs_quotactl_operations;
 	sb->s_op = &xfs_super_operations;
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.h	2008-04-30 08:24:31.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h	2008-04-30 08:43:49.000000000 +0200
@@ -84,6 +84,7 @@ extern void xfs_blkdev_put(struct block_
 extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
 
 extern const struct export_operations xfs_export_operations;
+extern struct xattr_handler *xfs_xattr_handlers[];
 
 #define XFS_M(sb)		((struct xfs_mount *)((sb)->s_fs_info))
 

Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig]

On Wed, Apr 30, 2008 at 01:22:17PM +0200, Christoph Hellwig wrote:
> Use the generic set, get and removexattr methods and supply the s_xattr
> array with fine-grained handlers.  All XFS/Linux highlevel attr handling is
> rewritten from scratch and placed into fs/xfs/linux-2.6/xfs_xattr.c so
> that it's separated from the generic low-level code.
> 
> The code size reduction is not as big as I had hoped, but it's still a
> worthwile cleanup.
> 
> I didn't managed to get rid of struct attrnames yet, as it's still used
> to hack the Linux string prefixes into the output inside the lowest
> level xattr code.  I have some plans to clean that bit up aswell, but
> that will have to wait for a while.

Updated on top of the case-insensitive filename changes:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6-xfs/fs/xfs/Makefile
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/Makefile	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/Makefile	2008-05-21 10:06:38.000000000 +0200
@@ -97,6 +97,7 @@ xfs-y				+= $(addprefix $(XFS_LINUX)/, \
 				   xfs_lrw.o \
 				   xfs_super.o \
 				   xfs_vnode.o \
+				   xfs_xattr.o \
 				   xfs_ksyms.o) 
 
 # Objects in support/
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 10:14:54.000000000 +0200
@@ -282,7 +282,7 @@ xfs_vn_mknod(
 	struct xfs_inode *ip = NULL;
 	xfs_acl_t	*default_acl = NULL;
 	struct xfs_name	name;
-	attrexists_t	test_default_acl = _ACL_DEFAULT_EXISTS;
+	int (*test_default_acl)(struct inode *) = _ACL_DEFAULT_EXISTS;
 	int		error;
 
 	/*
@@ -771,98 +771,6 @@ xfs_vn_truncate(
 	WARN_ON(error);
 }
 
-STATIC int
-xfs_vn_setxattr(
-	struct dentry	*dentry,
-	const char	*name,
-	const void	*data,
-	size_t		size,
-	int		flags)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	/* Convert Linux syscall to XFS internal ATTR flags */
-	if (flags & XATTR_CREATE)
-		xflags |= ATTR_CREATE;
-	if (flags & XATTR_REPLACE)
-		xflags |= ATTR_REPLACE;
-	xflags |= namesp->attr_flag;
-	return namesp->attr_set(vp, attr, (void *)data, size, xflags);
-}
-
-STATIC ssize_t
-xfs_vn_getxattr(
-	struct dentry	*dentry,
-	const char	*name,
-	void		*data,
-	size_t		size)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	/* Convert Linux syscall to XFS internal ATTR flags */
-	if (!size) {
-		xflags |= ATTR_KERNOVAL;
-		data = NULL;
-	}
-	xflags |= namesp->attr_flag;
-	return namesp->attr_get(vp, attr, (void *)data, size, xflags);
-}
-
-STATIC ssize_t
-xfs_vn_listxattr(
-	struct dentry		*dentry,
-	char			*data,
-	size_t			size)
-{
-	bhv_vnode_t		*vp = vn_from_inode(dentry->d_inode);
-	int			error, xflags = ATTR_KERNAMELS;
-	ssize_t			result;
-
-	if (!size)
-		xflags |= ATTR_KERNOVAL;
-	xflags |= capable(CAP_SYS_ADMIN) ? ATTR_KERNFULLS : ATTR_KERNORMALS;
-
-	error = attr_generic_list(vp, data, size, xflags, &result);
-	if (error < 0)
-		return error;
-	return result;
-}
-
-STATIC int
-xfs_vn_removexattr(
-	struct dentry	*dentry,
-	const char	*name)
-{
-	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
-	char		*attr = (char *)name;
-	attrnames_t	*namesp;
-	int		xflags = 0;
-
-	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	attr += namesp->attr_namelen;
-
-	xflags |= namesp->attr_flag;
-	return namesp->attr_remove(vp, attr, xflags);
-}
-
 STATIC long
 xfs_vn_fallocate(
 	struct inode	*inode,
@@ -910,10 +818,10 @@ const struct inode_operations xfs_inode_
 	.truncate		= xfs_vn_truncate,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 	.fallocate		= xfs_vn_fallocate,
 };
 
@@ -930,10 +838,10 @@ const struct inode_operations xfs_dir_in
 	.permission		= xfs_vn_permission,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 };
 
 const struct inode_operations xfs_dir_ci_inode_operations = {
@@ -949,10 +857,10 @@ const struct inode_operations xfs_dir_ci
 	.permission		= xfs_vn_permission,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 };
 
 const struct inode_operations xfs_symlink_inode_operations = {
@@ -962,8 +870,8 @@ const struct inode_operations xfs_symlin
 	.permission		= xfs_vn_permission,
 	.getattr		= xfs_vn_getattr,
 	.setattr		= xfs_vn_setattr,
-	.setxattr		= xfs_vn_setxattr,
-	.getxattr		= xfs_vn_getxattr,
+	.setxattr		= generic_setxattr,
+	.getxattr		= generic_getxattr,
+	.removexattr		= generic_removexattr,
 	.listxattr		= xfs_vn_listxattr,
-	.removexattr		= xfs_vn_removexattr,
 };
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c	2008-05-21 10:06:38.000000000 +0200
@@ -0,0 +1,266 @@
+/*
+ * Copyright (C) 2008 Christoph Hellwig.
+ *	Released under GPL v2.
+ */
+
+#include "xfs.h"
+#include "xfs_attr.h"
+#include "xfs_acl.h"
+#include "xfs_vnodeops.h"
+
+#include <linux/posix_acl_xattr.h>
+#include <linux/xattr.h>
+
+
+/*
+ * ACL handling.  Should eventually be moved into xfs_acl.c
+ */
+
+static int
+xfs_decode_acl(const char *name)
+{
+	if (strcmp(name, "posix_acl_access") == 0)
+		return _ACL_TYPE_ACCESS;
+	else if (strcmp(name, "posix_acl_default") == 0)
+		return _ACL_TYPE_DEFAULT;
+	return -EINVAL;
+}
+
+static int
+xfs_xattr_system_get(struct inode *inode, const char *name,
+		void *buffer, size_t size)
+{
+	int acl;
+
+	acl = xfs_decode_acl(name);
+	if (acl < 0)
+		return acl;
+
+	return xfs_acl_vget(inode, buffer, size, acl);
+}
+
+static int
+xfs_xattr_system_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	int error, acl;
+
+	acl = xfs_decode_acl(name);
+	if (acl < 0)
+		return acl;
+	if (flags & XATTR_CREATE)
+		return -EINVAL;
+
+	if (!value)
+		return xfs_acl_vremove(inode, acl);
+
+	error = xfs_acl_vset(inode, (void *)value, size, acl);
+	if (!error)
+		vn_revalidate(inode);
+	return error;
+}
+
+static struct xattr_handler xfs_xattr_system_handler = {
+	.prefix	= XATTR_SYSTEM_PREFIX,
+	.get	= xfs_xattr_system_get,
+	.set	= xfs_xattr_system_set,
+};
+
+
+/*
+ * Real xattr handling.  The only difference between the namespaces is
+ * a flag passed to the low-level attr code.
+ */
+
+static int
+__xfs_xattr_get(struct inode *inode, const char *name,
+		void *value, size_t size, int xflags)
+{
+	struct xfs_inode *ip = XFS_I(inode);
+	int error, asize = size;
+
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+
+	/* Convert Linux syscall to XFS internal ATTR flags */
+	if (!size) {
+		xflags |= ATTR_KERNOVAL;
+		value = NULL;
+	}
+
+	error = -xfs_attr_get(ip, name, value, &asize, xflags);
+	if (error)
+		return error;
+	return asize;
+}
+
+static int
+__xfs_xattr_set(struct inode *inode, const char *name, const void *value,
+		size_t size, int flags, int xflags)
+{
+	struct xfs_inode *ip = XFS_I(inode);
+
+	if (strcmp(name, "") == 0)
+		return -EINVAL;
+
+	/* Convert Linux syscall to XFS internal ATTR flags */
+	if (flags & XATTR_CREATE)
+		xflags |= ATTR_CREATE;
+	if (flags & XATTR_REPLACE)
+		xflags |= ATTR_REPLACE;
+
+	if (!value)
+		return -xfs_attr_remove(ip, name, xflags);
+	return -xfs_attr_set(ip, name, (void *)value, size, xflags);
+}
+
+static int
+xfs_xattr_user_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, 0);
+}
+
+static int
+xfs_xattr_user_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, 0);
+}
+
+struct attrnames attr_user = {
+	.attr_name	= "user.",
+	.attr_namelen	= sizeof("user.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_user_handler = {
+	.prefix	= XATTR_USER_PREFIX,
+	.get	= xfs_xattr_user_get,
+	.set	= xfs_xattr_user_set,
+};
+
+
+static int
+xfs_xattr_trusted_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, ATTR_ROOT);
+}
+
+static int
+xfs_xattr_trusted_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_ROOT);
+}
+
+struct attrnames attr_trusted = {
+	.attr_name	= "trusted.",
+	.attr_namelen	= sizeof("trusted.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_trusted_handler = {
+	.prefix	= XATTR_TRUSTED_PREFIX,
+	.get	= xfs_xattr_trusted_get,
+	.set	= xfs_xattr_trusted_set,
+};
+
+
+static int
+xfs_xattr_secure_get(struct inode *inode, const char *name,
+		void *value, size_t size)
+{
+	return __xfs_xattr_get(inode, name, value, size, ATTR_SECURE);
+}
+
+static int
+xfs_xattr_secure_set(struct inode *inode, const char *name,
+		const void *value, size_t size, int flags)
+{
+	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_SECURE);
+}
+
+struct attrnames attr_secure = {
+	.attr_name	= "security.",
+	.attr_namelen	= sizeof("security.") - 1,
+};
+
+static struct xattr_handler xfs_xattr_security_handler = {
+	.prefix	= XATTR_SECURITY_PREFIX,
+	.get	= xfs_xattr_secure_get,
+	.set	= xfs_xattr_secure_set,
+};
+
+
+struct xattr_handler *xfs_xattr_handlers[] = {
+	&xfs_xattr_user_handler,
+	&xfs_xattr_trusted_handler,
+	&xfs_xattr_security_handler,
+	&xfs_xattr_system_handler,
+	NULL
+};
+
+static int
+list_one_attr(const char *name, const size_t len, void *data,
+		size_t size, ssize_t *result)
+{
+	char *p = data + *result;
+
+	*result += len;
+	if (!size)
+		return 0;
+	if (*result > size)
+		return -ERANGE;
+
+	strcpy(p, name);
+	p += len;
+	return 0;
+}
+
+ssize_t
+xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
+{
+	struct inode		*inode = dentry->d_inode;
+	struct xfs_inode	*ip = XFS_I(inode);
+	attrlist_cursor_kern_t	cursor = { 0 };
+	int			error, xflags;
+	ssize_t			result;
+
+	xflags = ATTR_KERNAMELS;
+	if (!size)
+		xflags |= ATTR_KERNOVAL;
+
+	if (capable(CAP_SYS_ADMIN))
+		xflags |= ATTR_KERNFULLS;
+	else
+		xflags |= ATTR_KERNORMALS;
+
+
+	/*
+	 * First read the regular on-disk attributes.
+	 */
+	result = -xfs_attr_list(ip, data, size, xflags, &cursor);
+	if (result < 0)
+		return result;
+
+	/*
+	 * Then add the two synthetic ACL attributes.
+	 */
+	if (xfs_acl_vhasacl_access(inode)) {
+		error = list_one_attr(POSIX_ACL_XATTR_ACCESS,
+				strlen(POSIX_ACL_XATTR_ACCESS) + 1,
+				data, size, &result);
+		if (error)
+			return error;
+	}
+
+	if (xfs_acl_vhasacl_default(inode)) {
+		error = list_one_attr(POSIX_ACL_XATTR_DEFAULT,
+				strlen(POSIX_ACL_XATTR_DEFAULT) + 1,
+				data, size, &result);
+		if (error)
+			return error;
+	}
+
+	return result;
+}
Index: linux-2.6-xfs/fs/xfs/xfs_attr.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_attr.c	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_attr.c	2008-05-21 10:06:38.000000000 +0200
@@ -57,11 +57,6 @@
  * Provide the external interfaces to manage attribute lists.
  */
 
-#define ATTR_SYSCOUNT	2
-static struct attrnames posix_acl_access;
-static struct attrnames posix_acl_default;
-static struct attrnames *attr_system_names[ATTR_SYSCOUNT];
-
 /*========================================================================
  * Function prototypes for the kernel.
  *========================================================================*/
@@ -2378,270 +2373,3 @@ xfs_attr_trace_enter(int type, char *whe
 		(void *)a13, (void *)a14, (void *)a15);
 }
 #endif	/* XFS_ATTR_TRACE */
-
-
-/*========================================================================
- * System (pseudo) namespace attribute interface routines.
- *========================================================================*/
-
-STATIC int
-posix_acl_access_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vset(vp, data, size, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return xfs_acl_vremove(vp, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vget(vp, data, size, _ACL_TYPE_ACCESS);
-}
-
-STATIC int
-posix_acl_access_exists(
-	bhv_vnode_t *vp)
-{
-	return xfs_acl_vhasacl_access(vp);
-}
-
-STATIC int
-posix_acl_default_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vset(vp, data, size, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return xfs_acl_vget(vp, data, size, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return xfs_acl_vremove(vp, _ACL_TYPE_DEFAULT);
-}
-
-STATIC int
-posix_acl_default_exists(
-	bhv_vnode_t *vp)
-{
-	return xfs_acl_vhasacl_default(vp);
-}
-
-static struct attrnames posix_acl_access = {
-	.attr_name	= "posix_acl_access",
-	.attr_namelen	= sizeof("posix_acl_access") - 1,
-	.attr_get	= posix_acl_access_get,
-	.attr_set	= posix_acl_access_set,
-	.attr_remove	= posix_acl_access_remove,
-	.attr_exists	= posix_acl_access_exists,
-};
-
-static struct attrnames posix_acl_default = {
-	.attr_name	= "posix_acl_default",
-	.attr_namelen	= sizeof("posix_acl_default") - 1,
-	.attr_get	= posix_acl_default_get,
-	.attr_set	= posix_acl_default_set,
-	.attr_remove	= posix_acl_default_remove,
-	.attr_exists	= posix_acl_default_exists,
-};
-
-static struct attrnames *attr_system_names[] =
-	{ &posix_acl_access, &posix_acl_default };
-
-
-/*========================================================================
- * Namespace-prefix-style attribute name interface routines.
- *========================================================================*/
-
-STATIC int
-attr_generic_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	return -xfs_attr_set(xfs_vtoi(vp), name, data, size, xflags);
-}
-
-STATIC int
-attr_generic_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	int	error, asize = size;
-
-	error = xfs_attr_get(xfs_vtoi(vp), name, data, &asize, xflags);
-	if (!error)
-		return asize;
-	return -error;
-}
-
-STATIC int
-attr_generic_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	return -xfs_attr_remove(xfs_vtoi(vp), name, xflags);
-}
-
-STATIC int
-attr_generic_listadd(
-	attrnames_t		*prefix,
-	attrnames_t		*namesp,
-	void			*data,
-	size_t			size,
-	ssize_t			*result)
-{
-	char			*p = data + *result;
-
-	*result += prefix->attr_namelen;
-	*result += namesp->attr_namelen + 1;
-	if (!size)
-		return 0;
-	if (*result > size)
-		return -ERANGE;
-	strcpy(p, prefix->attr_name);
-	p += prefix->attr_namelen;
-	strcpy(p, namesp->attr_name);
-	p += namesp->attr_namelen + 1;
-	return 0;
-}
-
-STATIC int
-attr_system_list(
-	bhv_vnode_t		*vp,
-	void			*data,
-	size_t			size,
-	ssize_t			*result)
-{
-	attrnames_t		*namesp;
-	int			i, error = 0;
-
-	for (i = 0; i < ATTR_SYSCOUNT; i++) {
-		namesp = attr_system_names[i];
-		if (!namesp->attr_exists || !namesp->attr_exists(vp))
-			continue;
-		error = attr_generic_listadd(&attr_system, namesp,
-						data, size, result);
-		if (error)
-			break;
-	}
-	return error;
-}
-
-int
-attr_generic_list(
-	bhv_vnode_t *vp, void *data, size_t size, int xflags, ssize_t *result)
-{
-	attrlist_cursor_kern_t	cursor = { 0 };
-	int			error;
-
-	error = xfs_attr_list(xfs_vtoi(vp), data, size, xflags, &cursor);
-	if (error > 0)
-		return -error;
-	*result = -error;
-	return attr_system_list(vp, data, size, result);
-}
-
-attrnames_t *
-attr_lookup_namespace(
-	char			*name,
-	struct attrnames	**names,
-	int			nnames)
-{
-	int			i;
-
-	for (i = 0; i < nnames; i++)
-		if (!strncmp(name, names[i]->attr_name, names[i]->attr_namelen))
-			return names[i];
-	return NULL;
-}
-
-STATIC int
-attr_system_set(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	attrnames_t	*namesp;
-	int		error;
-
-	if (xflags & ATTR_CREATE)
-		return -EINVAL;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	error = namesp->attr_set(vp, name, data, size, xflags);
-	if (!error)
-		error = vn_revalidate(vp);
-	return error;
-}
-
-STATIC int
-attr_system_get(
-	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
-{
-	attrnames_t	*namesp;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	return namesp->attr_get(vp, name, data, size, xflags);
-}
-
-STATIC int
-attr_system_remove(
-	bhv_vnode_t *vp, char *name, int xflags)
-{
-	attrnames_t	*namesp;
-
-	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
-	if (!namesp)
-		return -EOPNOTSUPP;
-	return namesp->attr_remove(vp, name, xflags);
-}
-
-struct attrnames attr_system = {
-	.attr_name	= "system.",
-	.attr_namelen	= sizeof("system.") - 1,
-	.attr_flag	= ATTR_SYSTEM,
-	.attr_get	= attr_system_get,
-	.attr_set	= attr_system_set,
-	.attr_remove	= attr_system_remove,
-};
-
-struct attrnames attr_trusted = {
-	.attr_name	= "trusted.",
-	.attr_namelen	= sizeof("trusted.") - 1,
-	.attr_flag	= ATTR_ROOT,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames attr_secure = {
-	.attr_name	= "security.",
-	.attr_namelen	= sizeof("security.") - 1,
-	.attr_flag	= ATTR_SECURE,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames attr_user = {
-	.attr_name	= "user.",
-	.attr_namelen	= sizeof("user.") - 1,
-	.attr_get	= attr_generic_get,
-	.attr_set	= attr_generic_set,
-	.attr_remove	= attr_generic_remove,
-};
-
-struct attrnames *attr_namespaces[] =
-	{ &attr_system, &attr_trusted, &attr_secure, &attr_user };
Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_attr.h	2008-05-21 10:06:38.000000000 +0200
@@ -38,30 +38,14 @@
 struct cred;
 struct xfs_attr_list_context;
 
-typedef int (*attrset_t)(bhv_vnode_t *, char *, void *, size_t, int);
-typedef int (*attrget_t)(bhv_vnode_t *, char *, void *, size_t, int);
-typedef int (*attrremove_t)(bhv_vnode_t *, char *, int);
-typedef int (*attrexists_t)(bhv_vnode_t *);
-
 typedef struct attrnames {
 	char *		attr_name;
 	unsigned int	attr_namelen;
-	unsigned int	attr_flag;
-	attrget_t	attr_get;
-	attrset_t	attr_set;
-	attrremove_t	attr_remove;
-	attrexists_t	attr_exists;
 } attrnames_t;
 
-#define ATTR_NAMECOUNT	4
 extern struct attrnames attr_user;
 extern struct attrnames attr_secure;
-extern struct attrnames attr_system;
 extern struct attrnames attr_trusted;
-extern struct attrnames *attr_namespaces[ATTR_NAMECOUNT];
-
-extern attrnames_t *attr_lookup_namespace(char *, attrnames_t **, int);
-extern int attr_generic_list(bhv_vnode_t *, void *, size_t, int, ssize_t *);
 
 #define ATTR_DONTFOLLOW	0x0001	/* -- unused, from IRIX -- */
 #define ATTR_ROOT	0x0002	/* use attrs in root (trusted) namespace */
@@ -69,7 +53,6 @@ extern int attr_generic_list(bhv_vnode_t
 #define ATTR_SECURE	0x0008	/* use attrs in security namespace */
 #define ATTR_CREATE	0x0010	/* pure create: fail if attr already exists */
 #define ATTR_REPLACE	0x0020	/* pure set: fail if attr does not exist */
-#define ATTR_SYSTEM	0x0100	/* use attrs in system (pseudo) namespace */
 
 #define ATTR_KERNACCESS	0x0400	/* [kernel] iaccess, inode held io-locked */
 #define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 10:14:20.000000000 +0200
@@ -27,6 +27,7 @@ extern const struct file_operations xfs_
 extern const struct file_operations xfs_dir_file_operations;
 extern const struct file_operations xfs_invis_file_operations;
 
+extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
 
 struct xfs_inode;
 extern void xfs_ichgtime(struct xfs_inode *, int);
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 10:14:20.000000000 +0200
@@ -1767,6 +1767,7 @@ xfs_fs_fill_super(
 		goto out_free_mp;
 
 	sb_min_blocksize(sb, BBSIZE);
+	sb->s_xattr = xfs_xattr_handlers;
 	sb->s_export_op = &xfs_export_operations;
 	sb->s_qcop = &xfs_quotactl_operations;
 	sb->s_op = &xfs_super_operations;
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 09:59:53.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 10:14:20.000000000 +0200
@@ -81,6 +81,7 @@ extern void xfs_flush_device(struct xfs_
 extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
 
 extern const struct export_operations xfs_export_operations;
+extern struct xattr_handler *xfs_xattr_handlers[];
 
 #define XFS_M(sb)		((struct xfs_mount *)((sb)->s_fs_info))
 

Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin]

Looking at this now and will reply tomorrow.

--Tim

Christoph Hellwig wrote:
> On Wed, Apr 30, 2008 at 01:22:17PM +0200, Christoph Hellwig wrote:
>> Use the generic set, get and removexattr methods and supply the s_xattr
>> array with fine-grained handlers.  All XFS/Linux highlevel attr handling is
>> rewritten from scratch and placed into fs/xfs/linux-2.6/xfs_xattr.c so
>> that it's separated from the generic low-level code.
>>
>> The code size reduction is not as big as I had hoped, but it's still a
>> worthwile cleanup.
>>
>> I didn't managed to get rid of struct attrnames yet, as it's still used
>> to hack the Linux string prefixes into the output inside the lowest
>> level xattr code.  I have some plans to clean that bit up aswell, but
>> that will have to wait for a while.
> 
> Updated on top of the case-insensitive filename changes:
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6-xfs/fs/xfs/Makefile
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/Makefile	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/Makefile	2008-05-21 10:06:38.000000000 +0200
> @@ -97,6 +97,7 @@ xfs-y				+= $(addprefix $(XFS_LINUX)/, \
>  				   xfs_lrw.o \
>  				   xfs_super.o \
>  				   xfs_vnode.o \
> +				   xfs_xattr.o \
>  				   xfs_ksyms.o) 
>  
>  # Objects in support/
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 10:14:54.000000000 +0200
> @@ -282,7 +282,7 @@ xfs_vn_mknod(
>  	struct xfs_inode *ip = NULL;
>  	xfs_acl_t	*default_acl = NULL;
>  	struct xfs_name	name;
> -	attrexists_t	test_default_acl = _ACL_DEFAULT_EXISTS;
> +	int (*test_default_acl)(struct inode *) = _ACL_DEFAULT_EXISTS;
>  	int		error;
>  
>  	/*
> @@ -771,98 +771,6 @@ xfs_vn_truncate(
>  	WARN_ON(error);
>  }
>  
> -STATIC int
> -xfs_vn_setxattr(
> -	struct dentry	*dentry,
> -	const char	*name,
> -	const void	*data,
> -	size_t		size,
> -	int		flags)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	/* Convert Linux syscall to XFS internal ATTR flags */
> -	if (flags & XATTR_CREATE)
> -		xflags |= ATTR_CREATE;
> -	if (flags & XATTR_REPLACE)
> -		xflags |= ATTR_REPLACE;
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_set(vp, attr, (void *)data, size, xflags);
> -}
> -
> -STATIC ssize_t
> -xfs_vn_getxattr(
> -	struct dentry	*dentry,
> -	const char	*name,
> -	void		*data,
> -	size_t		size)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	/* Convert Linux syscall to XFS internal ATTR flags */
> -	if (!size) {
> -		xflags |= ATTR_KERNOVAL;
> -		data = NULL;
> -	}
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_get(vp, attr, (void *)data, size, xflags);
> -}
> -
> -STATIC ssize_t
> -xfs_vn_listxattr(
> -	struct dentry		*dentry,
> -	char			*data,
> -	size_t			size)
> -{
> -	bhv_vnode_t		*vp = vn_from_inode(dentry->d_inode);
> -	int			error, xflags = ATTR_KERNAMELS;
> -	ssize_t			result;
> -
> -	if (!size)
> -		xflags |= ATTR_KERNOVAL;
> -	xflags |= capable(CAP_SYS_ADMIN) ? ATTR_KERNFULLS : ATTR_KERNORMALS;
> -
> -	error = attr_generic_list(vp, data, size, xflags, &result);
> -	if (error < 0)
> -		return error;
> -	return result;
> -}
> -
> -STATIC int
> -xfs_vn_removexattr(
> -	struct dentry	*dentry,
> -	const char	*name)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_remove(vp, attr, xflags);
> -}
> -
>  STATIC long
>  xfs_vn_fallocate(
>  	struct inode	*inode,
> @@ -910,10 +818,10 @@ const struct inode_operations xfs_inode_
>  	.truncate		= xfs_vn_truncate,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  	.fallocate		= xfs_vn_fallocate,
>  };
>  
> @@ -930,10 +838,10 @@ const struct inode_operations xfs_dir_in
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };
>  
>  const struct inode_operations xfs_dir_ci_inode_operations = {
> @@ -949,10 +857,10 @@ const struct inode_operations xfs_dir_ci
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };
>  
>  const struct inode_operations xfs_symlink_inode_operations = {
> @@ -962,8 +870,8 @@ const struct inode_operations xfs_symlin
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c	2008-05-21 10:06:38.000000000 +0200
> @@ -0,0 +1,266 @@
> +/*
> + * Copyright (C) 2008 Christoph Hellwig.
> + *	Released under GPL v2.
> + */
> +
> +#include "xfs.h"
> +#include "xfs_attr.h"
> +#include "xfs_acl.h"
> +#include "xfs_vnodeops.h"
> +
> +#include <linux/posix_acl_xattr.h>
> +#include <linux/xattr.h>
> +
> +
> +/*
> + * ACL handling.  Should eventually be moved into xfs_acl.c
> + */
> +
> +static int
> +xfs_decode_acl(const char *name)
> +{
> +	if (strcmp(name, "posix_acl_access") == 0)
> +		return _ACL_TYPE_ACCESS;
> +	else if (strcmp(name, "posix_acl_default") == 0)
> +		return _ACL_TYPE_DEFAULT;
> +	return -EINVAL;
> +}
> +
> +static int
> +xfs_xattr_system_get(struct inode *inode, const char *name,
> +		void *buffer, size_t size)
> +{
> +	int acl;
> +
> +	acl = xfs_decode_acl(name);
> +	if (acl < 0)
> +		return acl;
> +
> +	return xfs_acl_vget(inode, buffer, size, acl);
> +}
> +
> +static int
> +xfs_xattr_system_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	int error, acl;
> +
> +	acl = xfs_decode_acl(name);
> +	if (acl < 0)
> +		return acl;
> +	if (flags & XATTR_CREATE)
> +		return -EINVAL;
> +
> +	if (!value)
> +		return xfs_acl_vremove(inode, acl);
> +
> +	error = xfs_acl_vset(inode, (void *)value, size, acl);
> +	if (!error)
> +		vn_revalidate(inode);
> +	return error;
> +}
> +
> +static struct xattr_handler xfs_xattr_system_handler = {
> +	.prefix	= XATTR_SYSTEM_PREFIX,
> +	.get	= xfs_xattr_system_get,
> +	.set	= xfs_xattr_system_set,
> +};
> +
> +
> +/*
> + * Real xattr handling.  The only difference between the namespaces is
> + * a flag passed to the low-level attr code.
> + */
> +
> +static int
> +__xfs_xattr_get(struct inode *inode, const char *name,
> +		void *value, size_t size, int xflags)
> +{
> +	struct xfs_inode *ip = XFS_I(inode);
> +	int error, asize = size;
> +
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;
> +
> +	/* Convert Linux syscall to XFS internal ATTR flags */
> +	if (!size) {
> +		xflags |= ATTR_KERNOVAL;
> +		value = NULL;
> +	}
> +
> +	error = -xfs_attr_get(ip, name, value, &asize, xflags);
> +	if (error)
> +		return error;
> +	return asize;
> +}
> +
> +static int
> +__xfs_xattr_set(struct inode *inode, const char *name, const void *value,
> +		size_t size, int flags, int xflags)
> +{
> +	struct xfs_inode *ip = XFS_I(inode);
> +
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;
> +
> +	/* Convert Linux syscall to XFS internal ATTR flags */
> +	if (flags & XATTR_CREATE)
> +		xflags |= ATTR_CREATE;
> +	if (flags & XATTR_REPLACE)
> +		xflags |= ATTR_REPLACE;
> +
> +	if (!value)
> +		return -xfs_attr_remove(ip, name, xflags);
> +	return -xfs_attr_set(ip, name, (void *)value, size, xflags);
> +}
> +
> +static int
> +xfs_xattr_user_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, 0);
> +}
> +
> +static int
> +xfs_xattr_user_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, 0);
> +}
> +
> +struct attrnames attr_user = {
> +	.attr_name	= "user.",
> +	.attr_namelen	= sizeof("user.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_user_handler = {
> +	.prefix	= XATTR_USER_PREFIX,
> +	.get	= xfs_xattr_user_get,
> +	.set	= xfs_xattr_user_set,
> +};
> +
> +
> +static int
> +xfs_xattr_trusted_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, ATTR_ROOT);
> +}
> +
> +static int
> +xfs_xattr_trusted_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_ROOT);
> +}
> +
> +struct attrnames attr_trusted = {
> +	.attr_name	= "trusted.",
> +	.attr_namelen	= sizeof("trusted.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_trusted_handler = {
> +	.prefix	= XATTR_TRUSTED_PREFIX,
> +	.get	= xfs_xattr_trusted_get,
> +	.set	= xfs_xattr_trusted_set,
> +};
> +
> +
> +static int
> +xfs_xattr_secure_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, ATTR_SECURE);
> +}
> +
> +static int
> +xfs_xattr_secure_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_SECURE);
> +}
> +
> +struct attrnames attr_secure = {
> +	.attr_name	= "security.",
> +	.attr_namelen	= sizeof("security.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_security_handler = {
> +	.prefix	= XATTR_SECURITY_PREFIX,
> +	.get	= xfs_xattr_secure_get,
> +	.set	= xfs_xattr_secure_set,
> +};
> +
> +
> +struct xattr_handler *xfs_xattr_handlers[] = {
> +	&xfs_xattr_user_handler,
> +	&xfs_xattr_trusted_handler,
> +	&xfs_xattr_security_handler,
> +	&xfs_xattr_system_handler,
> +	NULL
> +};
> +
> +static int
> +list_one_attr(const char *name, const size_t len, void *data,
> +		size_t size, ssize_t *result)
> +{
> +	char *p = data + *result;
> +
> +	*result += len;
> +	if (!size)
> +		return 0;
> +	if (*result > size)
> +		return -ERANGE;
> +
> +	strcpy(p, name);
> +	p += len;
> +	return 0;
> +}
> +
> +ssize_t
> +xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
> +{
> +	struct inode		*inode = dentry->d_inode;
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	attrlist_cursor_kern_t	cursor = { 0 };
> +	int			error, xflags;
> +	ssize_t			result;
> +
> +	xflags = ATTR_KERNAMELS;
> +	if (!size)
> +		xflags |= ATTR_KERNOVAL;
> +
> +	if (capable(CAP_SYS_ADMIN))
> +		xflags |= ATTR_KERNFULLS;
> +	else
> +		xflags |= ATTR_KERNORMALS;
> +
> +
> +	/*
> +	 * First read the regular on-disk attributes.
> +	 */
> +	result = -xfs_attr_list(ip, data, size, xflags, &cursor);
> +	if (result < 0)
> +		return result;
> +
> +	/*
> +	 * Then add the two synthetic ACL attributes.
> +	 */
> +	if (xfs_acl_vhasacl_access(inode)) {
> +		error = list_one_attr(POSIX_ACL_XATTR_ACCESS,
> +				strlen(POSIX_ACL_XATTR_ACCESS) + 1,
> +				data, size, &result);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (xfs_acl_vhasacl_default(inode)) {
> +		error = list_one_attr(POSIX_ACL_XATTR_DEFAULT,
> +				strlen(POSIX_ACL_XATTR_DEFAULT) + 1,
> +				data, size, &result);
> +		if (error)
> +			return error;
> +	}
> +
> +	return result;
> +}
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.c	2008-05-21 10:06:38.000000000 +0200
> @@ -57,11 +57,6 @@
>   * Provide the external interfaces to manage attribute lists.
>   */
>  
> -#define ATTR_SYSCOUNT	2
> -static struct attrnames posix_acl_access;
> -static struct attrnames posix_acl_default;
> -static struct attrnames *attr_system_names[ATTR_SYSCOUNT];
> -
>  /*========================================================================
>   * Function prototypes for the kernel.
>   *========================================================================*/
> @@ -2378,270 +2373,3 @@ xfs_attr_trace_enter(int type, char *whe
>  		(void *)a13, (void *)a14, (void *)a15);
>  }
>  #endif	/* XFS_ATTR_TRACE */
> -
> -
> -/*========================================================================
> - * System (pseudo) namespace attribute interface routines.
> - *========================================================================*/
> -
> -STATIC int
> -posix_acl_access_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vset(vp, data, size, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return xfs_acl_vremove(vp, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vget(vp, data, size, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_exists(
> -	bhv_vnode_t *vp)
> -{
> -	return xfs_acl_vhasacl_access(vp);
> -}
> -
> -STATIC int
> -posix_acl_default_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vset(vp, data, size, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vget(vp, data, size, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return xfs_acl_vremove(vp, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_exists(
> -	bhv_vnode_t *vp)
> -{
> -	return xfs_acl_vhasacl_default(vp);
> -}
> -
> -static struct attrnames posix_acl_access = {
> -	.attr_name	= "posix_acl_access",
> -	.attr_namelen	= sizeof("posix_acl_access") - 1,
> -	.attr_get	= posix_acl_access_get,
> -	.attr_set	= posix_acl_access_set,
> -	.attr_remove	= posix_acl_access_remove,
> -	.attr_exists	= posix_acl_access_exists,
> -};
> -
> -static struct attrnames posix_acl_default = {
> -	.attr_name	= "posix_acl_default",
> -	.attr_namelen	= sizeof("posix_acl_default") - 1,
> -	.attr_get	= posix_acl_default_get,
> -	.attr_set	= posix_acl_default_set,
> -	.attr_remove	= posix_acl_default_remove,
> -	.attr_exists	= posix_acl_default_exists,
> -};
> -
> -static struct attrnames *attr_system_names[] =
> -	{ &posix_acl_access, &posix_acl_default };
> -
> -
> -/*========================================================================
> - * Namespace-prefix-style attribute name interface routines.
> - *========================================================================*/
> -
> -STATIC int
> -attr_generic_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return -xfs_attr_set(xfs_vtoi(vp), name, data, size, xflags);
> -}
> -
> -STATIC int
> -attr_generic_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	int	error, asize = size;
> -
> -	error = xfs_attr_get(xfs_vtoi(vp), name, data, &asize, xflags);
> -	if (!error)
> -		return asize;
> -	return -error;
> -}
> -
> -STATIC int
> -attr_generic_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return -xfs_attr_remove(xfs_vtoi(vp), name, xflags);
> -}
> -
> -STATIC int
> -attr_generic_listadd(
> -	attrnames_t		*prefix,
> -	attrnames_t		*namesp,
> -	void			*data,
> -	size_t			size,
> -	ssize_t			*result)
> -{
> -	char			*p = data + *result;
> -
> -	*result += prefix->attr_namelen;
> -	*result += namesp->attr_namelen + 1;
> -	if (!size)
> -		return 0;
> -	if (*result > size)
> -		return -ERANGE;
> -	strcpy(p, prefix->attr_name);
> -	p += prefix->attr_namelen;
> -	strcpy(p, namesp->attr_name);
> -	p += namesp->attr_namelen + 1;
> -	return 0;
> -}
> -
> -STATIC int
> -attr_system_list(
> -	bhv_vnode_t		*vp,
> -	void			*data,
> -	size_t			size,
> -	ssize_t			*result)
> -{
> -	attrnames_t		*namesp;
> -	int			i, error = 0;
> -
> -	for (i = 0; i < ATTR_SYSCOUNT; i++) {
> -		namesp = attr_system_names[i];
> -		if (!namesp->attr_exists || !namesp->attr_exists(vp))
> -			continue;
> -		error = attr_generic_listadd(&attr_system, namesp,
> -						data, size, result);
> -		if (error)
> -			break;
> -	}
> -	return error;
> -}
> -
> -int
> -attr_generic_list(
> -	bhv_vnode_t *vp, void *data, size_t size, int xflags, ssize_t *result)
> -{
> -	attrlist_cursor_kern_t	cursor = { 0 };
> -	int			error;
> -
> -	error = xfs_attr_list(xfs_vtoi(vp), data, size, xflags, &cursor);
> -	if (error > 0)
> -		return -error;
> -	*result = -error;
> -	return attr_system_list(vp, data, size, result);
> -}
> -
> -attrnames_t *
> -attr_lookup_namespace(
> -	char			*name,
> -	struct attrnames	**names,
> -	int			nnames)
> -{
> -	int			i;
> -
> -	for (i = 0; i < nnames; i++)
> -		if (!strncmp(name, names[i]->attr_name, names[i]->attr_namelen))
> -			return names[i];
> -	return NULL;
> -}
> -
> -STATIC int
> -attr_system_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	attrnames_t	*namesp;
> -	int		error;
> -
> -	if (xflags & ATTR_CREATE)
> -		return -EINVAL;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	error = namesp->attr_set(vp, name, data, size, xflags);
> -	if (!error)
> -		error = vn_revalidate(vp);
> -	return error;
> -}
> -
> -STATIC int
> -attr_system_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	attrnames_t	*namesp;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	return namesp->attr_get(vp, name, data, size, xflags);
> -}
> -
> -STATIC int
> -attr_system_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	attrnames_t	*namesp;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	return namesp->attr_remove(vp, name, xflags);
> -}
> -
> -struct attrnames attr_system = {
> -	.attr_name	= "system.",
> -	.attr_namelen	= sizeof("system.") - 1,
> -	.attr_flag	= ATTR_SYSTEM,
> -	.attr_get	= attr_system_get,
> -	.attr_set	= attr_system_set,
> -	.attr_remove	= attr_system_remove,
> -};
> -
> -struct attrnames attr_trusted = {
> -	.attr_name	= "trusted.",
> -	.attr_namelen	= sizeof("trusted.") - 1,
> -	.attr_flag	= ATTR_ROOT,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames attr_secure = {
> -	.attr_name	= "security.",
> -	.attr_namelen	= sizeof("security.") - 1,
> -	.attr_flag	= ATTR_SECURE,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames attr_user = {
> -	.attr_name	= "user.",
> -	.attr_namelen	= sizeof("user.") - 1,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames *attr_namespaces[] =
> -	{ &attr_system, &attr_trusted, &attr_secure, &attr_user };
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.h	2008-05-21 10:06:38.000000000 +0200
> @@ -38,30 +38,14 @@
>  struct cred;
>  struct xfs_attr_list_context;
>  
> -typedef int (*attrset_t)(bhv_vnode_t *, char *, void *, size_t, int);
> -typedef int (*attrget_t)(bhv_vnode_t *, char *, void *, size_t, int);
> -typedef int (*attrremove_t)(bhv_vnode_t *, char *, int);
> -typedef int (*attrexists_t)(bhv_vnode_t *);
> -
>  typedef struct attrnames {
>  	char *		attr_name;
>  	unsigned int	attr_namelen;
> -	unsigned int	attr_flag;
> -	attrget_t	attr_get;
> -	attrset_t	attr_set;
> -	attrremove_t	attr_remove;
> -	attrexists_t	attr_exists;
>  } attrnames_t;
>  
> -#define ATTR_NAMECOUNT	4
>  extern struct attrnames attr_user;
>  extern struct attrnames attr_secure;
> -extern struct attrnames attr_system;
>  extern struct attrnames attr_trusted;
> -extern struct attrnames *attr_namespaces[ATTR_NAMECOUNT];
> -
> -extern attrnames_t *attr_lookup_namespace(char *, attrnames_t **, int);
> -extern int attr_generic_list(bhv_vnode_t *, void *, size_t, int, ssize_t *);
>  
>  #define ATTR_DONTFOLLOW	0x0001	/* -- unused, from IRIX -- */
>  #define ATTR_ROOT	0x0002	/* use attrs in root (trusted) namespace */
> @@ -69,7 +53,6 @@ extern int attr_generic_list(bhv_vnode_t
>  #define ATTR_SECURE	0x0008	/* use attrs in security namespace */
>  #define ATTR_CREATE	0x0010	/* pure create: fail if attr already exists */
>  #define ATTR_REPLACE	0x0020	/* pure set: fail if attr does not exist */
> -#define ATTR_SYSTEM	0x0100	/* use attrs in system (pseudo) namespace */
>  
>  #define ATTR_KERNACCESS	0x0400	/* [kernel] iaccess, inode held io-locked */
>  #define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 10:14:20.000000000 +0200
> @@ -27,6 +27,7 @@ extern const struct file_operations xfs_
>  extern const struct file_operations xfs_dir_file_operations;
>  extern const struct file_operations xfs_invis_file_operations;
>  
> +extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
>  
>  struct xfs_inode;
>  extern void xfs_ichgtime(struct xfs_inode *, int);
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 10:14:20.000000000 +0200
> @@ -1767,6 +1767,7 @@ xfs_fs_fill_super(
>  		goto out_free_mp;
>  
>  	sb_min_blocksize(sb, BBSIZE);
> +	sb->s_xattr = xfs_xattr_handlers;
>  	sb->s_export_op = &xfs_export_operations;
>  	sb->s_qcop = &xfs_quotactl_operations;
>  	sb->s_op = &xfs_super_operations;
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 10:14:20.000000000 +0200
> @@ -81,6 +81,7 @@ extern void xfs_flush_device(struct xfs_
>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>  
>  extern const struct export_operations xfs_export_operations;
> +extern struct xattr_handler *xfs_xattr_handlers[];
>  
>  #define XFS_M(sb)		((struct xfs_mount *)((sb)->s_fs_info))
>  
> 

Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin]

Hi Christoph,

Looks reasonable to me.

In list_one_attr(), which looks based on attr_generic_listadd(),
it does a final:
> +	p += len;
which seems useless.

An aside, I noticed in passing (which was in existing code),
how we call vn_revalidate
in xfs_xattr_system_set(). I presume this is because we
call xfs_acl_setmode() in xfs_acl_vset() when we want to sync
the mode bits to the ACL.
If this is the case, then I think it would have been clearer
to put vn_revalidate() in the vicinity of xfs_acl_setmode().
Or is there some other reason?

I'll test it out.

Thanks,
Tim.


Christoph Hellwig wrote:
> On Wed, Apr 30, 2008 at 01:22:17PM +0200, Christoph Hellwig wrote:
>> Use the generic set, get and removexattr methods and supply the s_xattr
>> array with fine-grained handlers.  All XFS/Linux highlevel attr handling is
>> rewritten from scratch and placed into fs/xfs/linux-2.6/xfs_xattr.c so
>> that it's separated from the generic low-level code.
>>
>> The code size reduction is not as big as I had hoped, but it's still a
>> worthwile cleanup.
>>
>> I didn't managed to get rid of struct attrnames yet, as it's still used
>> to hack the Linux string prefixes into the output inside the lowest
>> level xattr code.  I have some plans to clean that bit up aswell, but
>> that will have to wait for a while.
> 
> Updated on top of the case-insensitive filename changes:
> 
> 


Just my notes (for my reference)...

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6-xfs/fs/xfs/Makefile
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/Makefile	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/Makefile	2008-05-21 10:06:38.000000000 +0200
> @@ -97,6 +97,7 @@ xfs-y				+= $(addprefix $(XFS_LINUX)/, \
>  				   xfs_lrw.o \
>  				   xfs_super.o \
>  				   xfs_vnode.o \
> +				   xfs_xattr.o \
>  				   xfs_ksyms.o) 
>  
Okay adding in a linux-2.6/xfs_xattr.c
(slightly different name than xfs_attr.c)


>  # Objects in support/
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c	2008-05-21 10:14:54.000000000 +0200
> @@ -282,7 +282,7 @@ xfs_vn_mknod(
>  	struct xfs_inode *ip = NULL;
>  	xfs_acl_t	*default_acl = NULL;
>  	struct xfs_name	name;
> -	attrexists_t	test_default_acl = _ACL_DEFAULT_EXISTS;
> +	int (*test_default_acl)(struct inode *) = _ACL_DEFAULT_EXISTS;
>  	int		error;
>  
>  	/*
> @@ -771,98 +771,6 @@ xfs_vn_truncate(
>  	WARN_ON(error);
>  }
>  

So you are removing xfs_vn_setxattr, xfs_vn_getxattr, and
xfs_vn_removexattr and calling generic_xattr and retaining the
code in xattr_handler's.

You leave xfs_vn_listxattr alone. Just like ext3 does its own. Ok.

> -STATIC int
> -xfs_vn_setxattr(
> -	struct dentry	*dentry,
> -	const char	*name,
> -	const void	*data,
> -	size_t		size,
> -	int		flags)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	/* Convert Linux syscall to XFS internal ATTR flags */
> -	if (flags & XATTR_CREATE)
> -		xflags |= ATTR_CREATE;
> -	if (flags & XATTR_REPLACE)
> -		xflags |= ATTR_REPLACE;
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_set(vp, attr, (void *)data, size, xflags);
> -}
> -
> -STATIC ssize_t
> -xfs_vn_getxattr(
> -	struct dentry	*dentry,
> -	const char	*name,
> -	void		*data,
> -	size_t		size)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	/* Convert Linux syscall to XFS internal ATTR flags */
> -	if (!size) {
> -		xflags |= ATTR_KERNOVAL;
> -		data = NULL;
> -	}
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_get(vp, attr, (void *)data, size, xflags);
> -}
> -
> -STATIC ssize_t
> -xfs_vn_listxattr(
> -	struct dentry		*dentry,
> -	char			*data,
> -	size_t			size)
> -{
> -	bhv_vnode_t		*vp = vn_from_inode(dentry->d_inode);
> -	int			error, xflags = ATTR_KERNAMELS;
> -	ssize_t			result;
> -
> -	if (!size)
> -		xflags |= ATTR_KERNOVAL;
> -	xflags |= capable(CAP_SYS_ADMIN) ? ATTR_KERNFULLS : ATTR_KERNORMALS;
> -
> -	error = attr_generic_list(vp, data, size, xflags, &result);
> -	if (error < 0)
> -		return error;
> -	return result;
> -}
> -
> -STATIC int
> -xfs_vn_removexattr(
> -	struct dentry	*dentry,
> -	const char	*name)
> -{
> -	bhv_vnode_t	*vp = vn_from_inode(dentry->d_inode);
> -	char		*attr = (char *)name;
> -	attrnames_t	*namesp;
> -	int		xflags = 0;
> -
> -	namesp = attr_lookup_namespace(attr, attr_namespaces, ATTR_NAMECOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	attr += namesp->attr_namelen;
> -
> -	xflags |= namesp->attr_flag;
> -	return namesp->attr_remove(vp, attr, xflags);
> -}
> -
>  STATIC long
>  xfs_vn_fallocate(
>  	struct inode	*inode,
> @@ -910,10 +818,10 @@ const struct inode_operations xfs_inode_
>  	.truncate		= xfs_vn_truncate,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  	.fallocate		= xfs_vn_fallocate,
>  };
>  
> @@ -930,10 +838,10 @@ const struct inode_operations xfs_dir_in
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };
>  
>  const struct inode_operations xfs_dir_ci_inode_operations = {
> @@ -949,10 +857,10 @@ const struct inode_operations xfs_dir_ci
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };
>  
>  const struct inode_operations xfs_symlink_inode_operations = {
> @@ -962,8 +870,8 @@ const struct inode_operations xfs_symlin
>  	.permission		= xfs_vn_permission,
>  	.getattr		= xfs_vn_getattr,
>  	.setattr		= xfs_vn_setattr,
> -	.setxattr		= xfs_vn_setxattr,
> -	.getxattr		= xfs_vn_getxattr,
> +	.setxattr		= generic_setxattr,
> +	.getxattr		= generic_getxattr,
> +	.removexattr		= generic_removexattr,
>  	.listxattr		= xfs_vn_listxattr,
> -	.removexattr		= xfs_vn_removexattr,
>  };




> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c	2008-05-21 10:06:38.000000000 +0200
> @@ -0,0 +1,266 @@
> +/*
> + * Copyright (C) 2008 Christoph Hellwig.
> + *	Released under GPL v2.
> + */
> +
> +#include "xfs.h"
> +#include "xfs_attr.h"
> +#include "xfs_acl.h"
> +#include "xfs_vnodeops.h"
> +
> +#include <linux/posix_acl_xattr.h>
> +#include <linux/xattr.h>
> +
> +
> +/*
> + * ACL handling.  Should eventually be moved into xfs_acl.c
> + */
> +
> +static int
> +xfs_decode_acl(const char *name)
> +{
> +	if (strcmp(name, "posix_acl_access") == 0)
> +		return _ACL_TYPE_ACCESS;
> +	else if (strcmp(name, "posix_acl_default") == 0)
> +		return _ACL_TYPE_DEFAULT;
> +	return -EINVAL;
> +}
> +
Fine.

> +static int
> +xfs_xattr_system_get(struct inode *inode, const char *name,
> +		void *buffer, size_t size)
> +{
> +	int acl;
> +
> +	acl = xfs_decode_acl(name);
> +	if (acl < 0)
> +		return acl;
> +
> +	return xfs_acl_vget(inode, buffer, size, acl);
> +}
> +
Fine.
Seems a little funny as we are calling it a system EA but
then directly calling the acl code.
i.e. acknowledging, I guess, that we only have Posix ACLs
as system EAs.
Almost could call it xfs_xattr_acl_get().
It saves on one liner wrappers I guess.

> +static int
> +xfs_xattr_system_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	int error, acl;
> +
> +	acl = xfs_decode_acl(name);
> +	if (acl < 0)
> +		return acl;
> +	if (flags & XATTR_CREATE)
> +		return -EINVAL;
> +
> +	if (!value)
> +		return xfs_acl_vremove(inode, acl);
> +
> +	error = xfs_acl_vset(inode, (void *)value, size, acl);
> +	if (!error)
> +		vn_revalidate(inode);
> +	return error;
> +}
> +
Why the vn_revalidate?
b/c we did that in xfs_attr.c/attr_system_set().
It updates the linux inode fields based on the xfs inode fields.
I wonder why.
Because we can modify an ACL which can cause mode changes?
in the call to xfs_acl_setmode(vp, xfs_acl, &basicperms)
I wonder why we do it at this point though as it doesn't
look so obvious.


> +static struct xattr_handler xfs_xattr_system_handler = {
> +	.prefix	= XATTR_SYSTEM_PREFIX,
> +	.get	= xfs_xattr_system_get,
> +	.set	= xfs_xattr_system_set,
> +};
> +
> +
> +/*
> + * Real xattr handling.  The only difference between the namespaces is
> + * a flag passed to the low-level attr code.
> + */
> +
> +static int
> +__xfs_xattr_get(struct inode *inode, const char *name,
> +		void *value, size_t size, int xflags)
> +{
> +	struct xfs_inode *ip = XFS_I(inode);
> +	int error, asize = size;
> +
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;
> +
> +	/* Convert Linux syscall to XFS internal ATTR flags */
> +	if (!size) {
> +		xflags |= ATTR_KERNOVAL;
> +		value = NULL;
> +	}
> +
> +	error = -xfs_attr_get(ip, name, value, &asize, xflags);
> +	if (error)
> +		return error;
> +	return asize;
> +}
> +
Ok.

> +static int
> +__xfs_xattr_set(struct inode *inode, const char *name, const void *value,
> +		size_t size, int flags, int xflags)
> +{
> +	struct xfs_inode *ip = XFS_I(inode);
> +
> +	if (strcmp(name, "") == 0)
> +		return -EINVAL;
> +
> +	/* Convert Linux syscall to XFS internal ATTR flags */
> +	if (flags & XATTR_CREATE)
> +		xflags |= ATTR_CREATE;
> +	if (flags & XATTR_REPLACE)
> +		xflags |= ATTR_REPLACE;
> +
> +	if (!value)
> +		return -xfs_attr_remove(ip, name, xflags);
> +	return -xfs_attr_set(ip, name, (void *)value, size, xflags);
> +}
> +
Ok.

> +static int
> +xfs_xattr_user_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, 0);
> +}
> +
> +static int
> +xfs_xattr_user_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, 0);
> +}
> +
> +struct attrnames attr_user = {
> +	.attr_name	= "user.",
> +	.attr_namelen	= sizeof("user.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_user_handler = {
> +	.prefix	= XATTR_USER_PREFIX,
> +	.get	= xfs_xattr_user_get,
> +	.set	= xfs_xattr_user_set,
> +};
> +
Ok.

> +
> +static int
> +xfs_xattr_trusted_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, ATTR_ROOT);
> +}
> +
> +static int
> +xfs_xattr_trusted_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_ROOT);
> +}
> +
> +struct attrnames attr_trusted = {
> +	.attr_name	= "trusted.",
> +	.attr_namelen	= sizeof("trusted.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_trusted_handler = {
> +	.prefix	= XATTR_TRUSTED_PREFIX,
> +	.get	= xfs_xattr_trusted_get,
> +	.set	= xfs_xattr_trusted_set,
> +};
> +
Ok.

> +
> +static int
> +xfs_xattr_secure_get(struct inode *inode, const char *name,
> +		void *value, size_t size)
> +{
> +	return __xfs_xattr_get(inode, name, value, size, ATTR_SECURE);
> +}
> +
> +static int
> +xfs_xattr_secure_set(struct inode *inode, const char *name,
> +		const void *value, size_t size, int flags)
> +{
> +	return __xfs_xattr_set(inode, name, value, size, flags, ATTR_SECURE);
> +}
> +
> +struct attrnames attr_secure = {
> +	.attr_name	= "security.",
> +	.attr_namelen	= sizeof("security.") - 1,
> +};
> +
> +static struct xattr_handler xfs_xattr_security_handler = {
> +	.prefix	= XATTR_SECURITY_PREFIX,
> +	.get	= xfs_xattr_secure_get,
> +	.set	= xfs_xattr_secure_set,
> +};
> +
Ok.

> +
> +struct xattr_handler *xfs_xattr_handlers[] = {
> +	&xfs_xattr_user_handler,
> +	&xfs_xattr_trusted_handler,
> +	&xfs_xattr_security_handler,
> +	&xfs_xattr_system_handler,
> +	NULL
> +};
> +

So List code is done separately. Hmmmm...

Oh, okay, ATTR_KERNAMELS (for attr_vn_listxattr) uses
concatenated string arrays
whereas for not ATTR_KERNAMELS, such as is used in
xfs_attrlist_by_handle is uses list data in the form:
<u32: valuelen> <char: name-array> <pad-to-4-byte-boundary>

> +static int
> +list_one_attr(const char *name, const size_t len, void *data,
> +		size_t size, ssize_t *result)
> +{
> +	char *p = data + *result;
> +
> +	*result += len;
> +	if (!size)
> +		return 0;
> +	if (*result > size)
> +		return -ERANGE;
> +
> +	strcpy(p, name);
> +	p += len;
Q: Why increment p?

Ok, this code looks like it is coming from attr_generic_listadd().

> +	return 0;
> +}
> +

Previously, in attr_generic_list it did:
-> xfs_attr_list
-> attr_system_list

So you've expanded out attr_system_list into xfs_vn_listxattr().


> +ssize_t
> +xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
> +{
> +	struct inode		*inode = dentry->d_inode;
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	attrlist_cursor_kern_t	cursor = { 0 };
> +	int			error, xflags;
> +	ssize_t			result;
> +
> +	xflags = ATTR_KERNAMELS;
> +	if (!size)
> +		xflags |= ATTR_KERNOVAL;
> +
> +	if (capable(CAP_SYS_ADMIN))
> +		xflags |= ATTR_KERNFULLS;
> +	else
> +		xflags |= ATTR_KERNORMALS;
> +
> +
> +	/*
> +	 * First read the regular on-disk attributes.
> +	 */
> +	result = -xfs_attr_list(ip, data, size, xflags, &cursor);
> +	if (result < 0)
> +		return result;
> +
> +	/*
> +	 * Then add the two synthetic ACL attributes.
> +	 */
> +	if (xfs_acl_vhasacl_access(inode)) {
> +		error = list_one_attr(POSIX_ACL_XATTR_ACCESS,
> +				strlen(POSIX_ACL_XATTR_ACCESS) + 1,
> +				data, size, &result);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (xfs_acl_vhasacl_default(inode)) {
> +		error = list_one_attr(POSIX_ACL_XATTR_DEFAULT,
> +				strlen(POSIX_ACL_XATTR_DEFAULT) + 1,
> +				data, size, &result);
> +		if (error)
> +			return error;
> +	}
> +
> +	return result;
> +}

Looks reasonable.

> Index: linux-2.6-xfs/fs/xfs/xfs_attr.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.c	2008-05-21 10:06:38.000000000 +0200
> @@ -57,11 +57,6 @@
>   * Provide the external interfaces to manage attribute lists.
>   */
>  
> -#define ATTR_SYSCOUNT	2
> -static struct attrnames posix_acl_access;
> -static struct attrnames posix_acl_default;
> -static struct attrnames *attr_system_names[ATTR_SYSCOUNT];
> -
>  /*========================================================================
>   * Function prototypes for the kernel.
>   *========================================================================*/
> @@ -2378,270 +2373,3 @@ xfs_attr_trace_enter(int type, char *whe
>  		(void *)a13, (void *)a14, (void *)a15);
>  }
>  #endif	/* XFS_ATTR_TRACE */
> -
> -
> -/*========================================================================
> - * System (pseudo) namespace attribute interface routines.
> - *========================================================================*/
> -
> -STATIC int
> -posix_acl_access_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vset(vp, data, size, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return xfs_acl_vremove(vp, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vget(vp, data, size, _ACL_TYPE_ACCESS);
> -}
> -
> -STATIC int
> -posix_acl_access_exists(
> -	bhv_vnode_t *vp)
> -{
> -	return xfs_acl_vhasacl_access(vp);
> -}
> -
> -STATIC int
> -posix_acl_default_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vset(vp, data, size, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return xfs_acl_vget(vp, data, size, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return xfs_acl_vremove(vp, _ACL_TYPE_DEFAULT);
> -}
> -
> -STATIC int
> -posix_acl_default_exists(
> -	bhv_vnode_t *vp)
> -{
> -	return xfs_acl_vhasacl_default(vp);
> -}
> -
> -static struct attrnames posix_acl_access = {
> -	.attr_name	= "posix_acl_access",
> -	.attr_namelen	= sizeof("posix_acl_access") - 1,
> -	.attr_get	= posix_acl_access_get,
> -	.attr_set	= posix_acl_access_set,
> -	.attr_remove	= posix_acl_access_remove,
> -	.attr_exists	= posix_acl_access_exists,
> -};
> -
> -static struct attrnames posix_acl_default = {
> -	.attr_name	= "posix_acl_default",
> -	.attr_namelen	= sizeof("posix_acl_default") - 1,
> -	.attr_get	= posix_acl_default_get,
> -	.attr_set	= posix_acl_default_set,
> -	.attr_remove	= posix_acl_default_remove,
> -	.attr_exists	= posix_acl_default_exists,
> -};
> -
> -static struct attrnames *attr_system_names[] =
> -	{ &posix_acl_access, &posix_acl_default };
> -
> -
> -/*========================================================================
> - * Namespace-prefix-style attribute name interface routines.
> - *========================================================================*/
> -
> -STATIC int
> -attr_generic_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	return -xfs_attr_set(xfs_vtoi(vp), name, data, size, xflags);
> -}
> -
> -STATIC int
> -attr_generic_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	int	error, asize = size;
> -
> -	error = xfs_attr_get(xfs_vtoi(vp), name, data, &asize, xflags);
> -	if (!error)
> -		return asize;
> -	return -error;
> -}
> -
> -STATIC int
> -attr_generic_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	return -xfs_attr_remove(xfs_vtoi(vp), name, xflags);
> -}
> -
> -STATIC int
> -attr_generic_listadd(
> -	attrnames_t		*prefix,
> -	attrnames_t		*namesp,
> -	void			*data,
> -	size_t			size,
> -	ssize_t			*result)
> -{
> -	char			*p = data + *result;
> -
> -	*result += prefix->attr_namelen;
> -	*result += namesp->attr_namelen + 1;
> -	if (!size)
> -		return 0;
> -	if (*result > size)
> -		return -ERANGE;
> -	strcpy(p, prefix->attr_name);
> -	p += prefix->attr_namelen;
> -	strcpy(p, namesp->attr_name);
> -	p += namesp->attr_namelen + 1;
> -	return 0;
> -}
> -
> -STATIC int
> -attr_system_list(
> -	bhv_vnode_t		*vp,
> -	void			*data,
> -	size_t			size,
> -	ssize_t			*result)
> -{
> -	attrnames_t		*namesp;
> -	int			i, error = 0;
> -
> -	for (i = 0; i < ATTR_SYSCOUNT; i++) {
> -		namesp = attr_system_names[i];
> -		if (!namesp->attr_exists || !namesp->attr_exists(vp))
> -			continue;
> -		error = attr_generic_listadd(&attr_system, namesp,
> -						data, size, result);
> -		if (error)
> -			break;
> -	}
> -	return error;
> -}
> -
> -int
> -attr_generic_list(
> -	bhv_vnode_t *vp, void *data, size_t size, int xflags, ssize_t *result)
> -{
> -	attrlist_cursor_kern_t	cursor = { 0 };
> -	int			error;
> -
> -	error = xfs_attr_list(xfs_vtoi(vp), data, size, xflags, &cursor);
> -	if (error > 0)
> -		return -error;
> -	*result = -error;
> -	return attr_system_list(vp, data, size, result);
> -}
> -
> -attrnames_t *
> -attr_lookup_namespace(
> -	char			*name,
> -	struct attrnames	**names,
> -	int			nnames)
> -{
> -	int			i;
> -
> -	for (i = 0; i < nnames; i++)
> -		if (!strncmp(name, names[i]->attr_name, names[i]->attr_namelen))
> -			return names[i];
> -	return NULL;
> -}
> -
> -STATIC int
> -attr_system_set(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	attrnames_t	*namesp;
> -	int		error;
> -
> -	if (xflags & ATTR_CREATE)
> -		return -EINVAL;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	error = namesp->attr_set(vp, name, data, size, xflags);
> -	if (!error)
> -		error = vn_revalidate(vp);
> -	return error;
> -}
> -
> -STATIC int
> -attr_system_get(
> -	bhv_vnode_t *vp, char *name, void *data, size_t size, int xflags)
> -{
> -	attrnames_t	*namesp;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	return namesp->attr_get(vp, name, data, size, xflags);
> -}
> -
> -STATIC int
> -attr_system_remove(
> -	bhv_vnode_t *vp, char *name, int xflags)
> -{
> -	attrnames_t	*namesp;
> -
> -	namesp = attr_lookup_namespace(name, attr_system_names, ATTR_SYSCOUNT);
> -	if (!namesp)
> -		return -EOPNOTSUPP;
> -	return namesp->attr_remove(vp, name, xflags);
> -}
> -
> -struct attrnames attr_system = {
> -	.attr_name	= "system.",
> -	.attr_namelen	= sizeof("system.") - 1,
> -	.attr_flag	= ATTR_SYSTEM,
> -	.attr_get	= attr_system_get,
> -	.attr_set	= attr_system_set,
> -	.attr_remove	= attr_system_remove,
> -};
> -
> -struct attrnames attr_trusted = {
> -	.attr_name	= "trusted.",
> -	.attr_namelen	= sizeof("trusted.") - 1,
> -	.attr_flag	= ATTR_ROOT,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames attr_secure = {
> -	.attr_name	= "security.",
> -	.attr_namelen	= sizeof("security.") - 1,
> -	.attr_flag	= ATTR_SECURE,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames attr_user = {
> -	.attr_name	= "user.",
> -	.attr_namelen	= sizeof("user.") - 1,
> -	.attr_get	= attr_generic_get,
> -	.attr_set	= attr_generic_set,
> -	.attr_remove	= attr_generic_remove,
> -};
> -
> -struct attrnames *attr_namespaces[] =
> -	{ &attr_system, &attr_trusted, &attr_secure, &attr_user };
> Index: linux-2.6-xfs/fs/xfs/xfs_attr.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_attr.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_attr.h	2008-05-21 10:06:38.000000000 +0200
> @@ -38,30 +38,14 @@
>  struct cred;
>  struct xfs_attr_list_context;
>  
> -typedef int (*attrset_t)(bhv_vnode_t *, char *, void *, size_t, int);
> -typedef int (*attrget_t)(bhv_vnode_t *, char *, void *, size_t, int);
> -typedef int (*attrremove_t)(bhv_vnode_t *, char *, int);
> -typedef int (*attrexists_t)(bhv_vnode_t *);
> -
>  typedef struct attrnames {
>  	char *		attr_name;
>  	unsigned int	attr_namelen;
> -	unsigned int	attr_flag;
> -	attrget_t	attr_get;
> -	attrset_t	attr_set;
> -	attrremove_t	attr_remove;
> -	attrexists_t	attr_exists;
>  } attrnames_t;
>  
> -#define ATTR_NAMECOUNT	4
>  extern struct attrnames attr_user;
>  extern struct attrnames attr_secure;
> -extern struct attrnames attr_system;
>  extern struct attrnames attr_trusted;
> -extern struct attrnames *attr_namespaces[ATTR_NAMECOUNT];
> -
> -extern attrnames_t *attr_lookup_namespace(char *, attrnames_t **, int);
> -extern int attr_generic_list(bhv_vnode_t *, void *, size_t, int, ssize_t *);
>  
>  #define ATTR_DONTFOLLOW	0x0001	/* -- unused, from IRIX -- */
>  #define ATTR_ROOT	0x0002	/* use attrs in root (trusted) namespace */
> @@ -69,7 +53,6 @@ extern int attr_generic_list(bhv_vnode_t
>  #define ATTR_SECURE	0x0008	/* use attrs in security namespace */
>  #define ATTR_CREATE	0x0010	/* pure create: fail if attr already exists */
>  #define ATTR_REPLACE	0x0020	/* pure set: fail if attr does not exist */
> -#define ATTR_SYSTEM	0x0100	/* use attrs in system (pseudo) namespace */
>  
>  #define ATTR_KERNACCESS	0x0400	/* [kernel] iaccess, inode held io-locked */
>  #define ATTR_KERNOTIME	0x1000	/* [kernel] don't update inode timestamps */
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.h	2008-05-21 10:14:20.000000000 +0200
> @@ -27,6 +27,7 @@ extern const struct file_operations xfs_
>  extern const struct file_operations xfs_dir_file_operations;
>  extern const struct file_operations xfs_invis_file_operations;
>  
> +extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
>  
>  struct xfs_inode;
>  extern void xfs_ichgtime(struct xfs_inode *, int);
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c	2008-05-21 10:14:20.000000000 +0200
> @@ -1767,6 +1767,7 @@ xfs_fs_fill_super(
>  		goto out_free_mp;
>  
>  	sb_min_blocksize(sb, BBSIZE);
> +	sb->s_xattr = xfs_xattr_handlers;
>  	sb->s_export_op = &xfs_export_operations;
>  	sb->s_qcop = &xfs_quotactl_operations;
>  	sb->s_op = &xfs_super_operations;
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 09:59:53.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.h	2008-05-21 10:14:20.000000000 +0200
> @@ -81,6 +81,7 @@ extern void xfs_flush_device(struct xfs_
>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>  
>  extern const struct export_operations xfs_export_operations;
> +extern struct xattr_handler *xfs_xattr_handlers[];
>  
>  #define XFS_M(sb)		((struct xfs_mount *)((sb)->s_fs_info))
>  
> 

Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig]

On Fri, May 23, 2008 at 03:22:14PM +1000, Timothy Shimmin wrote:
> Hi Christoph,
> 
> Looks reasonable to me.
> 
> In list_one_attr(), which looks based on attr_generic_listadd(),
> it does a final:
> > +	p += len;
> which seems useless.

Yeah, feel free to remove it when you commit the patch.  Alternatively
I'll send an incremental patch once commited.

> An aside, I noticed in passing (which was in existing code),
> how we call vn_revalidate
> in xfs_xattr_system_set(). I presume this is because we
> call xfs_acl_setmode() in xfs_acl_vset() when we want to sync
> the mode bits to the ACL.
> If this is the case, then I think it would have been clearer
> to put vn_revalidate() in the vicinity of xfs_acl_setmode().
> Or is there some other reason?

No real reason, I was just keeping what was there before.  But getting
rid of vn_revalidate complete is on my todo list.  The timestamp updates
in there are already not nessecary anymore, and the i_mode/i_uid/i_gid
and i_flags updates can be done much better in the caller that change
the values in the dinode.

> > +				   xfs_xattr.o \
> >  				   xfs_ksyms.o) 
> >  
> Okay adding in a linux-2.6/xfs_xattr.c
> (slightly different name than xfs_attr.c)

Yeah.  For one xfs_attr.c is already taken in fs/xfs and second the
xattr name makes it pretty clear these are the Linux xattr routines and
not the lowlevel XFS attr code


> So you are removing xfs_vn_setxattr, xfs_vn_getxattr, and
> xfs_vn_removexattr and calling generic_xattr and retaining the
> code in xattr_handler's.
> 
> You leave xfs_vn_listxattr alone. Just like ext3 does its own. Ok.

Yes, the generic listxattr doesn't buy us anything as we just traverse
the attr btree and list all of them in their natural order.  No point
in traversing it N times for N different attribute handlers.

> > +static int
> > +xfs_decode_acl(const char *name)
> > +{
> > +	if (strcmp(name, "posix_acl_access") == 0)
> > +		return _ACL_TYPE_ACCESS;
> > +	else if (strcmp(name, "posix_acl_default") == 0)
> > +		return _ACL_TYPE_DEFAULT;
> > +	return -EINVAL;
> > +}
> > +
> Fine.
> 
> > +static int
> > +xfs_xattr_system_get(struct inode *inode, const char *name,
> > +		void *buffer, size_t size)
> > +{
> > +	int acl;
> > +
> > +	acl = xfs_decode_acl(name);
> > +	if (acl < 0)
> > +		return acl;
> > +
> > +	return xfs_acl_vget(inode, buffer, size, acl);
> > +}
> > +
> Fine.
> Seems a little funny as we are calling it a system EA but
> then directly calling the acl code.
> i.e. acknowledging, I guess, that we only have Posix ACLs
> as system EAs.
> Almost could call it xfs_xattr_acl_get().
> It saves on one liner wrappers I guess.

Probably worth adding a comment that we only have acls for now.
The reason I did this is to avoid doing multiple memcpys on the xattr
name for decoding it.  It will probably need some revisiting if/when
we support other system xattrs.

> Why the vn_revalidate?
> b/c we did that in xfs_attr.c/attr_system_set().
> It updates the linux inode fields based on the xfs inode fields.
> I wonder why.
> Because we can modify an ACL which can cause mode changes?
> in the call to xfs_acl_setmode(vp, xfs_acl, &basicperms)
> I wonder why we do it at this point though as it doesn't
> look so obvious.

Yeah, se the comment above.  vn_revalidate will go away pretty soon at
which point this is sorted out.

> > +struct xattr_handler *xfs_xattr_handlers[] = {
> > +	&xfs_xattr_user_handler,
> > +	&xfs_xattr_trusted_handler,
> > +	&xfs_xattr_security_handler,
> > +	&xfs_xattr_system_handler,
> > +	NULL
> > +};
> > +
> 
> So List code is done separately. Hmmmm...
> 
> Oh, okay, ATTR_KERNAMELS (for attr_vn_listxattr) uses
> concatenated string arrays
> whereas for not ATTR_KERNAMELS, such as is used in
> xfs_attrlist_by_handle is uses list data in the form:
> <u32: valuelen> <char: name-array> <pad-to-4-byte-boundary>

Yes, this is quite unfortunate.  My plan is to refactor the low-level
attr code so that is passes down a formatter ala filldir for directory
reading.  This should clean up the attr listing code a lot and also
gets rid of the levtover struct attrnames bits.  But that a different
patch which still needs to be written.

> Previously, in attr_generic_list it did:
> -> xfs_attr_list
> -> attr_system_list
> 
> So you've expanded out attr_system_list into xfs_vn_listxattr().

Yes.  And loop-unrolled it while we're at it because the loop over
the system attrs didn't make much sense.

Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Fri, May 23, 2008 at 03:22:14PM +1000, Timothy Shimmin wrote:
>> Hi Christoph,
>>
>> Looks reasonable to me.
>>
>> In list_one_attr(), which looks based on attr_generic_listadd(),
>> it does a final:
>>> +	p += len;
>> which seems useless.
> 
> Yeah, feel free to remove it when you commit the patch.  Alternatively
> I'll send an incremental patch once commited.
> 
Yep, I'll remove it before checkin.

>> An aside, I noticed in passing (which was in existing code),
>> how we call vn_revalidate
>> in xfs_xattr_system_set(). I presume this is because we
>> call xfs_acl_setmode() in xfs_acl_vset() when we want to sync
>> the mode bits to the ACL.
>> If this is the case, then I think it would have been clearer
>> to put vn_revalidate() in the vicinity of xfs_acl_setmode().
>> Or is there some other reason?
> 
> No real reason, I was just keeping what was there before.  But getting
> rid of vn_revalidate complete is on my todo list.  The timestamp updates
> in there are already not nessecary anymore, and the i_mode/i_uid/i_gid
> and i_flags updates can be done much better in the caller that change
> the values in the dinode.
> 
Okay, good to know.


>> So you are removing xfs_vn_setxattr, xfs_vn_getxattr, and
>> xfs_vn_removexattr and calling generic_xattr and retaining the
>> code in xattr_handler's.
>>
>> You leave xfs_vn_listxattr alone. Just like ext3 does its own. Ok.
> 
> Yes, the generic listxattr doesn't buy us anything as we just traverse
> the attr btree and list all of them in their natural order.  No point
> in traversing it N times for N different attribute handlers.
> 
Good point (I didn't really look).


>>> +static int
>>> +xfs_xattr_system_get(struct inode *inode, const char *name,
>>> +		void *buffer, size_t size)
>>> +{
>>> +	int acl;
>>> +
>>> +	acl = xfs_decode_acl(name);
>>> +	if (acl < 0)
>>> +		return acl;
>>> +
>>> +	return xfs_acl_vget(inode, buffer, size, acl);
>>> +}
>>> +
>> Fine.
>> Seems a little funny as we are calling it a system EA but
>> then directly calling the acl code.
>> i.e. acknowledging, I guess, that we only have Posix ACLs
>> as system EAs.
>> Almost could call it xfs_xattr_acl_get().
>> It saves on one liner wrappers I guess.
> 
> Probably worth adding a comment that we only have acls for now.
> The reason I did this is to avoid doing multiple memcpys on the xattr
> name for decoding it.  It will probably need some revisiting if/when
> we support other system xattrs.
Okay, I'll add a comment.


> 
> Yeah, se the comment above.  vn_revalidate will go away pretty soon at
> which point this is sorted out.
> 
>>> +struct xattr_handler *xfs_xattr_handlers[] = {
>>> +	&xfs_xattr_user_handler,
>>> +	&xfs_xattr_trusted_handler,
>>> +	&xfs_xattr_security_handler,
>>> +	&xfs_xattr_system_handler,
>>> +	NULL
>>> +};
>>> +
>> So List code is done separately. Hmmmm...
>>
>> Oh, okay, ATTR_KERNAMELS (for attr_vn_listxattr) uses
>> concatenated string arrays
>> whereas for not ATTR_KERNAMELS, such as is used in
>> xfs_attrlist_by_handle is uses list data in the form:
>> <u32: valuelen> <char: name-array> <pad-to-4-byte-boundary>
> 
> Yes, this is quite unfortunate.  My plan is to refactor the low-level
> attr code so that is passes down a formatter ala filldir for directory
> reading.  This should clean up the attr listing code a lot and also
> gets rid of the levtover struct attrnames bits.  But that a different
> patch which still needs to be written.
> 
I guess this is done to some extent by the put_listent() callback.
Though, the context structure is probably a bit overused in different
ways.
The callback was also used for searching (for parent-ptr code) as well
as for list formatting.
I presume we are preserving the valuelen list format to keep the API
for xfs_attrlist_by_handle used by xfsdump and probably for dmf.

Thanks,
--Tim

Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig]

On Mon, May 26, 2008 at 11:43:42AM +1000, Timothy Shimmin wrote:
> I guess this is done to some extent by the put_listent() callback.
> Though, the context structure is probably a bit overused in different
> ways.
> The callback was also used for searching (for parent-ptr code) as well
> as for list formatting.
> I presume we are preserving the valuelen list format to keep the API
> for xfs_attrlist_by_handle used by xfsdump and probably for dmf.

Yes, the idea is to change the put_listen callback for work more like
filldir.  Thas is:

 - the callback is supplied by the xfs_attr_list caller, not set based
   on options
 - there will be an opaque object supplied to xfs_attr_list that is to
   be used by put_listent so that we don't have to pass down
   implementation-specific arguments directly.

I'd also like to move the attrlist_cursor_kern_t into this callback
opaque context because it doesn't make sense for the normal xattr API,
but I'll have to see if that's actually feasible.

Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Mon, May 26, 2008 at 11:43:42AM +1000, Timothy Shimmin wrote:
>> I guess this is done to some extent by the put_listent() callback.
>> Though, the context structure is probably a bit overused in different
>> ways.
>> The callback was also used for searching (for parent-ptr code) as well
>> as for list formatting.
>> I presume we are preserving the valuelen list format to keep the API
>> for xfs_attrlist_by_handle used by xfsdump and probably for dmf.
> 
> Yes, the idea is to change the put_listen callback for work more like
> filldir.  Thas is:
> 
>  - the callback is supplied by the xfs_attr_list caller, not set based
>    on options

Oh, okay. For example, instead of setting the flags to ATTR_KERNOVAL
such as in xfs_vn_listxattr when size is 0, one could just set the callback
to xfs_attr_kern_list_sizes and pass it in etc...

>  - there will be an opaque object supplied to xfs_attr_list that is to
>    be used by put_listent so that we don't have to pass down
>    implementation-specific arguments directly.
>

Ok.
So instead of overloading fields in xfs_attr_list_context_t,
you'll pass down a void* argument or some such for callback specific data.

> I'd also like to move the attrlist_cursor_kern_t into this callback
> opaque context because it doesn't make sense for the normal xattr API,
> but I'll have to see if that's actually feasible.

BTW, the cursor stuff is a bit flawed. Like the dir1 code (I believe),
if from userspace you use the cursor and modifications happen to the EAs
(add or removal) between calls, 
we can end up repeating elements in the list or miss some.
We don't preserve the position and we can compact the data etc.

--Tim


xfs_attr_list(
        xfs_inode_t     *dp,
        char            *buffer,
        int             bufsize,
        int             flags,
        attrlist_cursor_kern_t *cursor)
{       
        xfs_attr_list_context_t context;
...
        if (flags & ATTR_KERNAMELS) {
                context.bufsize = bufsize;
                context.firstu = context.bufsize;
                if (flags & ATTR_KERNOVAL)
                        context.put_listent = xfs_attr_kern_list_sizes;
                else
                        context.put_listent = xfs_attr_kern_list;
        } else {
                context.bufsize = (bufsize & ~(sizeof(int)-1));  /* align */
                context.firstu = context.bufsize;
                context.alist->al_count = 0;
                context.alist->al_more = 0;
                context.alist->al_offset[0] = context.bufsize;
                context.put_listent = xfs_attr_put_listent;
        }


ssize_t
xfs_vn_listxattr(struct dentry *dentry, char *data, size_t size)
{
        struct inode            *inode = dentry->d_inode;
        struct xfs_inode        *ip = XFS_I(inode);
        attrlist_cursor_kern_t  cursor = { 0 };
        int                     error, xflags;
        ssize_t                 result;

        xflags = ATTR_KERNAMELS;
        if (!size)
                xflags |= ATTR_KERNOVAL;

        if (capable(CAP_SYS_ADMIN))
                xflags |= ATTR_KERNFULLS;
        else
                xflags |= ATTR_KERNORMALS;


        /*
         * First read the regular on-disk attributes.
         */
        result = -xfs_attr_list(ip, data, size, xflags, &cursor);

Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig]

On Tue, May 27, 2008 at 08:50:48PM +1000, Timothy Shimmin wrote:
> >  - the callback is supplied by the xfs_attr_list caller, not set based
> >    on options
> 
> Oh, okay. For example, instead of setting the flags to ATTR_KERNOVAL
> such as in xfs_vn_listxattr when size is 0, one could just set the callback
> to xfs_attr_kern_list_sizes and pass it in etc...

Yes.  I have an initial patch that goes directly to xfs_attr_list_int
from xfs_xattr.c and kills most of the ATTR_KERN flags.  It's a quite
nice cleanup already.  Next step will be to convert dmapi to use it's
own callback aswell.  This will be an even bigger cleanup as
put_listent gets the xattr value aswell and we can kill the additional
xfs_attr_get calls, making this code simpler and more efficient.

> >  - there will be an opaque object supplied to xfs_attr_list that is to
> >    be used by put_listent so that we don't have to pass down
> >    implementation-specific arguments directly.
> >
> 
> Ok.
> So instead of overloading fields in xfs_attr_list_context_t,
> you'll pass down a void* argument or some such for callback specific data.

I've started looking at this and after some investigattion I think
we should just pass the xfs_inode directly to all the functions and then
a void parameter, yes.  We'll need to find a solution for the
seen_enough paramter, but I think this could be handled similar to
filldir.  There's also some functions directly touching the attr cursor
which seems solveable, too.

> > I'd also like to move the attrlist_cursor_kern_t into this callback
> > opaque context because it doesn't make sense for the normal xattr API,
> > but I'll have to see if that's actually feasible.
> 
> BTW, the cursor stuff is a bit flawed. Like the dir1 code (I believe),
> if from userspace you use the cursor and modifications happen to the EAs
> (add or removal) between calls, 
> we can end up repeating elements in the list or miss some.
> We don't preserve the position and we can compact the data etc.

Yes, I think this whole cursor is a rather bad idea.  But given that
it's used by xfsdump we can't easily get rid of it.

Re: [PATCH] use generic_*xattr routines

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Tue, May 27, 2008 at 08:50:48PM +1000, Timothy Shimmin wrote:
>>>  - the callback is supplied by the xfs_attr_list caller, not set based
>>>    on options
>> Oh, okay. For example, instead of setting the flags to ATTR_KERNOVAL
>> such as in xfs_vn_listxattr when size is 0, one could just set the callback
>> to xfs_attr_kern_list_sizes and pass it in etc...
> 
> Yes.  I have an initial patch that goes directly to xfs_attr_list_int
> from xfs_xattr.c and kills most of the ATTR_KERN flags.  It's a quite
> nice cleanup already.  Next step will be to convert dmapi to use it's
> own callback aswell.  This will be an even bigger cleanup as
> put_listent gets the xattr value aswell and we can kill the additional
> xfs_attr_get calls, making this code simpler and more efficient.
> 
Sorry, what xfs_attr_get call are you referring to?

>>>  - there will be an opaque object supplied to xfs_attr_list that is to
>>>    be used by put_listent so that we don't have to pass down
>>>    implementation-specific arguments directly.
>>>
>> Ok.
>> So instead of overloading fields in xfs_attr_list_context_t,
>> you'll pass down a void* argument or some such for callback specific data.
> 
> I've started looking at this and after some investigation I think
> we should just pass the xfs_inode directly to all the functions and then
> a void parameter, yes.  We'll need to find a solution for the
> seen_enough paramter, but I think this could be handled similar to
> filldir.  There's also some functions directly touching the attr cursor
> which seems solveable, too.
> 
I'll await the patch :)
The seen_enough param was added for search type callbacks so the callback
could terminate the list walk early.
Oh okay, I also used it to stop when we fill the buffer.

>>> I'd also like to move the attrlist_cursor_kern_t into this callback
>>> opaque context because it doesn't make sense for the normal xattr API,
>>> but I'll have to see if that's actually feasible.
>> BTW, the cursor stuff is a bit flawed. Like the dir1 code (I believe),
>> if from userspace you use the cursor and modifications happen to the EAs
>> (add or removal) between calls, 
>> we can end up repeating elements in the list or miss some.
>> We don't preserve the position and we can compact the data etc.
> 
> Yes, I think this whole cursor is a rather bad idea.  But given that
> it's used by xfsdump we can't easily get rid of it.
Not if we are to remain compatible with old xfsdump programs (assuming we changed
the latest one).
Yep.

--Tim

Re: [PATCH] use generic_*xattr routines

[Christoph Hellwig]

On Fri, May 30, 2008 at 04:37:42PM +1000, Timothy Shimmin wrote:
> > Yes.  I have an initial patch that goes directly to xfs_attr_list_int
> > from xfs_xattr.c and kills most of the ATTR_KERN flags.  It's a quite
> > nice cleanup already.  Next step will be to convert dmapi to use it's
> > own callback aswell.  This will be an even bigger cleanup as
> > put_listent gets the xattr value aswell and we can kill the additional
> > xfs_attr_get calls, making this code simpler and more efficient.
> > 
> Sorry, what xfs_attr_get call are you referring to?

xfs_dm_getall_dmattr currently does an xfs_attr_list and then performs
and xfs_attr_get for each attribute it cares about.  But rewriting this
to use put_listent we not only get rid of the temporary buffer but
also the need to calls xfs_attr_get because put_listent already gets
the attribute value passed.  It also fixes the race mentioned in the
comment and with a properly written put_listent helper can copy the
attributes directly to userspace without any kernel buffer at all.

That would also fix the direct userspace pointer dereference currently
lingering in that code :)

> > I've started looking at this and after some investigation I think
> > we should just pass the xfs_inode directly to all the functions and then
> > a void parameter, yes.  We'll need to find a solution for the
> > seen_enough paramter, but I think this could be handled similar to
> > filldir.  There's also some functions directly touching the attr cursor
> > which seems solveable, too.
> > 
> I'll await the patch :)
> The seen_enough param was added for search type callbacks so the callback
> could terminate the list walk early.
> Oh okay, I also used it to stop when we fill the buffer.

Yes. May idea is to break out of the loop as soon as put_listent returns
a non-zero value just like filldir.  We can then decided to play with
positivie / begative values to allow for a sucessfull early exit, or
just like filldir have the actual errno inside the private context
structure.  I suspect the first variant will be cleaner.