Skip to contentSkip to conversation

: nfsd: more vfs_getxattr

PATCH In review Christoph Hellwig 2 participants 3 messages Nov 7, 2005

Subsystems (2)
VFS NFS server
lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 : nfsd: more vfs_getxattr

looks like I missed NFSv4 previously. Instead of just converting it add an nfsd_getxattr helper for the code shared by NFSv2/3 and NFSv4 ACLs. In fact that code isn't even NFS-specific, but I'll wait for more users to pop up first before moving it to common code.

Signed-off-by Christoph Hellwig <[email protected]>
fs/nfsd/vfs.c +44 -49
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
@@ -358,8 +358,30 @@
 	goto out;
 }
 
-#if defined(CONFIG_NFSD_V4)
+#if defined(CONFIG_NFSD_V2_ACL) || \
+    defined(CONFIG_NFSD_V3_ACL) || \
+    defined(CONFIG_NFSD_V4)
+static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
+{
+	ssize_t buflen;
+	int error;
+
+	buflen = vfs_getxattr(dentry, key, NULL, 0);
+	if (buflen <= 0)
+		return buflen;
+
+	*buf = kmalloc(buflen, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	error = vfs_getxattr(dentry, key, *buf, buflen);
+	if (error < 0)
+		return error;
+	return buflen;
+}
+#endif
 
+#if defined(CONFIG_NFSD_V4)
 static int
 set_nfsv4_acl_one(struct dentry *dentry, struct posix_acl *pacl, char *key)
 {

@@ -439,30 +461,17 @@
 static struct posix_acl *
 _get_posix_acl(struct dentry *dentry, char *key)
 {
-	char *buf = NULL;
-	int buflen, error = 0;
+	void *buf = NULL;
 	struct posix_acl *pacl = NULL;
+	int buflen;
 
-	buflen = vfs_getxattr(dentry, key, NULL, 0);
-	if (buflen <= 0) {
-		pacl = ERR_PTR(buflen < 0 ? buflen : -ENODATA);
-		goto out;
-	}
-
-	buf = kmalloc(buflen, GFP_KERNEL);
-	if (buf == NULL) {
-		pacl = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	error = vfs_getxattr(dentry, key, buf, buflen);
-	if (error < 0) {
-		pacl = ERR_PTR(error);
-		goto out;
-	}
+	buflen = nfsd_getxattr(dentry, key, &buf);
+	if (!buflen)
+		buflen = -ENODATA;
+	if (buflen <= 0)
+		return ERR_PTR(buflen);
 
 	pacl = posix_acl_from_xattr(buf, buflen);
- out:
 	kfree(buf);
 	return pacl;
 }

@@ -1846,39 +1855,25 @@
 	ssize_t size;
 	struct posix_acl *acl;
 
-	if (!IS_POSIXACL(inode) || !inode->i_op || !inode->i_op->getxattr)
+	if (!IS_POSIXACL(inode))
+		return ERR_PTR(-EOPNOTSUPP);
+
+	switch (type) {
+	case ACL_TYPE_ACCESS:
+		name = POSIX_ACL_XATTR_ACCESS;
+		break;
+	case ACL_TYPE_DEFAULT:
+		name = POSIX_ACL_XATTR_DEFAULT;
+		break;
+	default:
 		return ERR_PTR(-EOPNOTSUPP);
-	switch(type) {
-		case ACL_TYPE_ACCESS:
-			name = POSIX_ACL_XATTR_ACCESS;
-			break;
-		case ACL_TYPE_DEFAULT:
-			name = POSIX_ACL_XATTR_DEFAULT;
-			break;
-		default:
-			return ERR_PTR(-EOPNOTSUPP);
 	}
 
-	size = inode->i_op->getxattr(fhp->fh_dentry, name, NULL, 0);
+	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
+	if (size < 0)
+		return ERR_PTR(size);
 
-	if (size < 0) {
-		acl = ERR_PTR(size);
-		goto getout;
-	} else if (size > 0) {
-		value = kmalloc(size, GFP_KERNEL);
-		if (!value) {
-			acl = ERR_PTR(-ENOMEM);
-			goto getout;
-		}
-		size = inode->i_op->getxattr(fhp->fh_dentry, name, value, size);
-		if (size < 0) {
-			acl = ERR_PTR(size);
-			goto getout;
-		}
-	}
 	acl = posix_acl_from_xattr(value, size);
-
-getout:
 	kfree(value);
 	return acl;
 }
#1 lore raw
Quoted 2 lines · Christoph Hellwig

looks like I missed NFSv4 previously.

When sending fixes for earlier patches, it greatly helps if you can identify which patch it is that needs fixing. Usually it's obvious, but in this case it's not.

fs/nfsd/vfs.c line 375

Quoted 3 lines
fs/nfsd/vfs.c +3 -0
+	*buf = kmalloc(buflen, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;

That's obviously broken.

author
#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 2 lines · Christoph Hellwig

looks like I missed NFSv4 previously.

When sending fixes for earlier patches, it greatly helps if you can identify which patch it is that needs fixing. Usually it's obvious, but in this case it's not.

vfs_getxattr was introduced in

[PATCH 1/8] add vfs_* helpers for xattr operations

but at that point it was just a cleanup. it only became mandatory in later patches to remove xattr checks in the filesystems.

Quoted 6 lines

Review

Quoted 4 lines
(quoted diff) +3 -0
+	*buf = kmalloc(buflen, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;

That's obviously broken.

Fixed one below:

fs/nfsd/vfs.c +44 -49
Index: linux-2.6/fs/nfsd/vfs.c
===================================================================
@@ -358,8 +358,30 @@
 	goto out;
 }
 
-#if defined(CONFIG_NFSD_V4)
+#if defined(CONFIG_NFSD_V2_ACL) || \
+    defined(CONFIG_NFSD_V3_ACL) || \
+    defined(CONFIG_NFSD_V4)
+static ssize_t nfsd_getxattr(struct dentry *dentry, char *key, void **buf)
+{
+	ssize_t buflen;
+	int error;
+
+	buflen = vfs_getxattr(dentry, key, NULL, 0);
+	if (buflen <= 0)
+		return buflen;
+
+	*buf = kmalloc(buflen, GFP_KERNEL);
+	if (!*buf)
+		return -ENOMEM;
+
+	error = vfs_getxattr(dentry, key, *buf, buflen);
+	if (error < 0)
+		return error;
+	return buflen;
+}
+#endif
 
+#if defined(CONFIG_NFSD_V4)
 static int
 set_nfsv4_acl_one(struct dentry *dentry, struct posix_acl *pacl, char *key)
 {

@@ -439,30 +461,17 @@
 static struct posix_acl *
 _get_posix_acl(struct dentry *dentry, char *key)
 {
-	char *buf = NULL;
-	int buflen, error = 0;
+	void *buf = NULL;
 	struct posix_acl *pacl = NULL;
+	int buflen;
 
-	buflen = vfs_getxattr(dentry, key, NULL, 0);
-	if (buflen <= 0) {
-		pacl = ERR_PTR(buflen < 0 ? buflen : -ENODATA);
-		goto out;
-	}
-
-	buf = kmalloc(buflen, GFP_KERNEL);
-	if (buf == NULL) {
-		pacl = ERR_PTR(-ENOMEM);
-		goto out;
-	}
-
-	error = vfs_getxattr(dentry, key, buf, buflen);
-	if (error < 0) {
-		pacl = ERR_PTR(error);
-		goto out;
-	}
+	buflen = nfsd_getxattr(dentry, key, &buf);
+	if (!buflen)
+		buflen = -ENODATA;
+	if (buflen <= 0)
+		return ERR_PTR(buflen);
 
 	pacl = posix_acl_from_xattr(buf, buflen);
- out:
 	kfree(buf);
 	return pacl;
 }

@@ -1846,39 +1855,25 @@
 	ssize_t size;
 	struct posix_acl *acl;
 
-	if (!IS_POSIXACL(inode) || !inode->i_op || !inode->i_op->getxattr)
+	if (!IS_POSIXACL(inode))
+		return ERR_PTR(-EOPNOTSUPP);
+
+	switch (type) {
+	case ACL_TYPE_ACCESS:
+		name = POSIX_ACL_XATTR_ACCESS;
+		break;
+	case ACL_TYPE_DEFAULT:
+		name = POSIX_ACL_XATTR_DEFAULT;
+		break;
+	default:
 		return ERR_PTR(-EOPNOTSUPP);
-	switch(type) {
-		case ACL_TYPE_ACCESS:
-			name = POSIX_ACL_XATTR_ACCESS;
-			break;
-		case ACL_TYPE_DEFAULT:
-			name = POSIX_ACL_XATTR_DEFAULT;
-			break;
-		default:
-			return ERR_PTR(-EOPNOTSUPP);
 	}
 
-	size = inode->i_op->getxattr(fhp->fh_dentry, name, NULL, 0);
+	size = nfsd_getxattr(fhp->fh_dentry, name, &value);
+	if (size < 0)
+		return ERR_PTR(size);
 
-	if (size < 0) {
-		acl = ERR_PTR(size);
-		goto getout;
-	} else if (size > 0) {
-		value = kmalloc(size, GFP_KERNEL);
-		if (!value) {
-			acl = ERR_PTR(-ENOMEM);
-			goto getout;
-		}
-		size = inode->i_op->getxattr(fhp->fh_dentry, name, value, size);
-		if (size < 0) {
-			acl = ERR_PTR(size);
-			goto getout;
-		}
-	}
 	acl = posix_acl_from_xattr(value, size);
-
-getout:
 	kfree(value);
 	return acl;
 }