fs: xattr: rewrite simple_xattr_set()

PATCH Superseded Aristeu Rozanski 3 participants 8 messages Oct 25, 2012 – Oct 31, 2012 VFS

Share & tools

Aristeu Rozanski

author

2012-10-25 15:26 UTC #0 lore raw

PATCH 1/1 fs: xattr: rewrite simple_xattr_set()

The way this function was written is confusing and already caused problems. Rewriting it to be easier to understand and maintain.

Cc Tejun Heo <[email protected]>

Cc Li Zefan <[email protected]>

Cc Al Viro <[email protected]>

Signed-off-by Aristeu Rozanski <[email protected]>

---
 fs/xattr.c |  124 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 48 deletions(-)

fs/xattr.c +76 ~~-48~~

Index: github/fs/xattr.c
===================================================================
@@ -842,55 +842,46 @@
 	return ret;
 }
 
-static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
-			      const void *value, size_t size, int flags)
+static struct simple_xattr *__find_xattr(struct simple_xattrs *xattrs,
+					 const char *name)
 {
 	struct simple_xattr *xattr;
-	struct simple_xattr *new_xattr = NULL;
-	int err = 0;
-
-	/* value == NULL means remove */
-	if (value) {
-		new_xattr = simple_xattr_alloc(value, size);
-		if (!new_xattr)
-			return -ENOMEM;
-
-		new_xattr->name = kstrdup(name, GFP_KERNEL);
-		if (!new_xattr->name) {
-			kfree(new_xattr);
-			return -ENOMEM;
-		}
-	}
 
-	spin_lock(&xattrs->lock);
 	list_for_each_entry(xattr, &xattrs->head, list) {
-		if (!strcmp(name, xattr->name)) {
-			if (flags & XATTR_CREATE) {
-				xattr = new_xattr;
-				err = -EEXIST;
-			} else if (new_xattr) {
-				list_replace(&xattr->list, &new_xattr->list);
-			} else {
-				list_del(&xattr->list);
-			}
-			goto out;
-		}
+		if (!strcmp(name, xattr->name))
+			return xattr;
 	}
-	if (flags & XATTR_REPLACE) {
-		xattr = new_xattr;
-		err = -ENODATA;
-	} else {
-		list_add(&new_xattr->list, &xattrs->head);
-		xattr = NULL;
-	}
-out:
-	spin_unlock(&xattrs->lock);
+	return NULL;
+}
+
+static int __simple_xattr_remove(struct simple_xattrs *xattrs,
+				 const char *name)
+{
+	struct simple_xattr *xattr;
+
+	xattr = __find_xattr(xattrs, name);
 	if (xattr) {
+		list_del(&xattr->list);
 		kfree(xattr->name);
 		kfree(xattr);
+		return 0;
 	}
-	return err;
 
+	return 1;
+}
+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems
+ */
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
+{
+	int rc;
+
+	spin_lock(&xattrs->lock);
+	rc = __simple_xattr_remove(xattrs, name);
+	spin_unlock(&xattrs->lock);
+
+	return rc;
 }
 
 /**

@@ -910,17 +901,54 @@
 int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 		     const void *value, size_t size, int flags)
 {
+	struct simple_xattr *found, *new_xattr;
+	int err = 0;
+
 	if (size == 0)
-		value = ""; /* empty EA, do not remove */
-	return __simple_xattr_set(xattrs, name, value, size, flags);
-}
+		value = ""; /* empty EA */
 
-/*
- * xattr REMOVE operation for in-memory/pseudo filesystems
- */
-int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
-{
-	return __simple_xattr_set(xattrs, name, NULL, 0, XATTR_REPLACE);
+	/* if value == NULL is specified, remove the item */
+	if (value == NULL)
+		return simple_xattr_remove(xattrs, name);
+
+	new_xattr = simple_xattr_alloc(value, size);
+	if (!new_xattr)
+		return -ENOMEM;
+
+	new_xattr->name = kstrdup(name, GFP_KERNEL);
+	if (!new_xattr->name) {
+		kfree(new_xattr);
+		return -ENOMEM;
+	}
+
+	spin_lock(&xattrs->lock);
+
+	found = __find_xattr(xattrs, name);
+	if (found) {
+		if (flags & XATTR_CREATE) {
+			err = -EEXIST;
+			goto free_new;
+		}
+
+		list_replace(&found->list, &new_xattr->list);
+		kfree(found->name);
+		kfree(found);
+	} else {
+		if (flags & XATTR_REPLACE) {
+			err = -ENODATA;
+			goto free_new;
+		}
+
+		list_add_tail(&new_xattr->list, &xattrs->head);
+	}
+
+out:
+	spin_unlock(&xattrs->lock);
+	return err;
+free_new:
+	kfree(new_xattr->name);
+	kfree(new_xattr);
+	goto out;
 }
 
 static bool xattr_is_trusted(const char *name)

Carlos Maiolino

Reviewed-by on v1 1/1

2012-10-25 17:04 UTC #1 lore raw

Quoted 17 lines · Aristeu Rozanski

The way this function was written is confusing and already caused problems. Rewriting it to be easier to understand and maintain.
Cc Tejun Heo <[email protected]>
Cc Li Zefan <[email protected]>
Cc Al Viro <[email protected]>
Signed-off-by Aristeu Rozanski <[email protected]>
---
 fs/xattr.c |  124 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 48 deletions(-)
Index: github/fs/xattr.c
===================================================================

fs/xattr.c line 844

Quoted 3 lines

fs/xattr.c +0 -0

@@ -842,55 +842,46 @@
 	return ret;
 }

Looks good to me

Reviewed-by Carlos Maiolino <[email protected]>

Signature · 1 line

--Carlos

Tejun Heo

2012-10-25 17:33 UTC #2 lore raw

In reply to Aristeu Rozanski

(cc'ing Hugh and keeping the whole body)

Hello,

Quoted 7 lines · Aristeu Rozanski

The way this function was written is confusing and already caused problems. Rewriting it to be easier to understand and maintain.
Cc Tejun Heo <[email protected]>
Cc Li Zefan <[email protected]>
Cc Al Viro <[email protected]>
Signed-off-by Aristeu Rozanski <[email protected]>

Generally looks okay to me but I think the return value from removal path is wrong. More below.

Quoted 68 lines

---
 fs/xattr.c |  124 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 48 deletions(-)

Index: github/fs/xattr.c
===================================================================

fs/xattr.c +13 ~~-38~~

@@ -842,55 +842,46 @@
 	return ret;
 }
 
-static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
-			      const void *value, size_t size, int flags)
+static struct simple_xattr *__find_xattr(struct simple_xattrs *xattrs,
+					 const char *name)
 {
 	struct simple_xattr *xattr;
-	struct simple_xattr *new_xattr = NULL;
-	int err = 0;
-
-	/* value == NULL means remove */
-	if (value) {
-		new_xattr = simple_xattr_alloc(value, size);
-		if (!new_xattr)
-			return -ENOMEM;
-
-		new_xattr->name = kstrdup(name, GFP_KERNEL);
-		if (!new_xattr->name) {
-			kfree(new_xattr);
-			return -ENOMEM;
-		}
-	}
 
-	spin_lock(&xattrs->lock);
 	list_for_each_entry(xattr, &xattrs->head, list) {
-		if (!strcmp(name, xattr->name)) {
-			if (flags & XATTR_CREATE) {
-				xattr = new_xattr;
-				err = -EEXIST;
-			} else if (new_xattr) {
-				list_replace(&xattr->list, &new_xattr->list);
-			} else {
-				list_del(&xattr->list);
-			}
-			goto out;
-		}
+		if (!strcmp(name, xattr->name))
+			return xattr;
 	}
-	if (flags & XATTR_REPLACE) {
-		xattr = new_xattr;
-		err = -ENODATA;
-	} else {
-		list_add(&new_xattr->list, &xattrs->head);
-		xattr = NULL;
-	}
-out:
-	spin_unlock(&xattrs->lock);
+	return NULL;
+}
+
+static int __simple_xattr_remove(struct simple_xattrs *xattrs,
+				 const char *name)
+{
+	struct simple_xattr *xattr;
+
+	xattr = __find_xattr(xattrs, name);

fs/xattr.c line 871

Quoted 10 lines

fs/xattr.c +4 -1

 	if (xattr) {
+		list_del(&xattr->list);
 		kfree(xattr->name);
 		kfree(xattr);
+		return 0;
 	}
-	return err;
 
+	return 1;
+}

So, it returns 0 on success and 1 on failure, which in itself isn't a particularly good idea.

Quoted 28 lines

fs/xattr.c +17 -3

+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems
+ */
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
+{
+	int rc;
+
+	spin_lock(&xattrs->lock);
+	rc = __simple_xattr_remove(xattrs, name);
+	spin_unlock(&xattrs->lock);
+
+	return rc;
 }
 
 /**

@@ -910,17 +901,54 @@
 int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 		     const void *value, size_t size, int flags)
 {
+	struct simple_xattr *found, *new_xattr;
+	int err = 0;
+
 	if (size == 0)
-		value = ""; /* empty EA, do not remove */
-	return __simple_xattr_set(xattrs, name, value, size, flags);
-}
+		value = ""; /* empty EA */

fs/xattr.c line 912

Quoted 10 lines

fs/xattr.c +3 -6

 
-/*
- * xattr REMOVE operation for in-memory/pseudo filesystems
- */
-int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
-{
-	return __simple_xattr_set(xattrs, name, NULL, 0, XATTR_REPLACE);
+	/* if value == NULL is specified, remove the item */
+	if (value == NULL)
+		return simple_xattr_remove(xattrs, name);

And gets relayed to the caller.

Quoted 42 lines

fs/xattr.c +39 -0

+
+	new_xattr = simple_xattr_alloc(value, size);
+	if (!new_xattr)
+		return -ENOMEM;
+
+	new_xattr->name = kstrdup(name, GFP_KERNEL);
+	if (!new_xattr->name) {
+		kfree(new_xattr);
+		return -ENOMEM;
+	}
+
+	spin_lock(&xattrs->lock);
+
+	found = __find_xattr(xattrs, name);
+	if (found) {
+		if (flags & XATTR_CREATE) {
+			err = -EEXIST;
+			goto free_new;
+		}
+
+		list_replace(&found->list, &new_xattr->list);
+		kfree(found->name);
+		kfree(found);
+	} else {
+		if (flags & XATTR_REPLACE) {
+			err = -ENODATA;
+			goto free_new;
+		}
+
+		list_add_tail(&new_xattr->list, &xattrs->head);
+	}
+
+out:
+	spin_unlock(&xattrs->lock);
+	return err;
+free_new:
+	kfree(new_xattr->name);
+	kfree(new_xattr);
+	goto out;
 }
 
 static bool xattr_is_trusted(const char *name)

Signature · 1 line

tejun

Aristeu Rozanski

author

2012-10-25 17:54 UTC #3 lore raw

Quoted 8 lines · Tejun Heo

fs/xattr.c line 871
Quoted 4 lines · Aristeu Rozanski
fs/xattr.c +2 -1
-	return err;
 
+	return 1;
+}
So, it returns 0 on success and 1 on failure, which in itself isn't a particularly good idea.

you mean it should return -ENODATA instead?

Signature · 1 line

Aristeu

Tejun Heo

2012-10-25 17:59 UTC #4 lore raw

Hello,

Quoted 4 lines · Aristeu Rozanski

Quoted 2 lines
So, it returns 0 on success and 1 on failure, which in itself isn't a particularly good idea.
you mean it should return -ENODATA instead?

Yeap, this is a bug, right? We end up passing that 1 to the set_xattr caller.

Signature · 1 line

tejun

Aristeu Rozanski

author

2012-10-25 18:12 UTC #5 lore raw

Quoted 7 lines · Tejun Heo

Quoted 4 lines · Aristeu Rozanski
Quoted 2 lines
So, it returns 0 on success and 1 on failure, which in itself isn't a particularly good idea.
you mean it should return -ENODATA instead?
Yeap, this is a bug, right? We end up passing that 1 to the set_xattr caller.

yes, will resubmit

Thanks!

Signature · 1 line

Aristeu

Aristeu Rozanski

author

2012-10-25 18:30 UTC #6 lore raw

PATCH v2 1/1 fs: xattr: rewrite simple_xattr_set()

In reply to Tejun Heo

The way this function was written is confusing and already caused problems. Rewriting it to be easier to understand and maintain.

v2:
- fix error return value in __simple_xattr_remove() (pointed by Tejun Heo)

Cc Hugh Dickins <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Li Zefan <[email protected]>

Cc Al Viro <[email protected]>

Signed-off-by Aristeu Rozanski <[email protected]>

---
 fs/xattr.c |  124 +++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 48 deletions(-)

fs/xattr.c +76 ~~-48~~

Index: github/fs/xattr.c
===================================================================
@@ -842,55 +842,46 @@
 	return ret;
 }
 
-static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
-			      const void *value, size_t size, int flags)
+static struct simple_xattr *__find_xattr(struct simple_xattrs *xattrs,
+					 const char *name)
 {
 	struct simple_xattr *xattr;
-	struct simple_xattr *new_xattr = NULL;
-	int err = 0;
-
-	/* value == NULL means remove */
-	if (value) {
-		new_xattr = simple_xattr_alloc(value, size);
-		if (!new_xattr)
-			return -ENOMEM;
-
-		new_xattr->name = kstrdup(name, GFP_KERNEL);
-		if (!new_xattr->name) {
-			kfree(new_xattr);
-			return -ENOMEM;
-		}
-	}
 
-	spin_lock(&xattrs->lock);
 	list_for_each_entry(xattr, &xattrs->head, list) {
-		if (!strcmp(name, xattr->name)) {
-			if (flags & XATTR_CREATE) {
-				xattr = new_xattr;
-				err = -EEXIST;
-			} else if (new_xattr) {
-				list_replace(&xattr->list, &new_xattr->list);
-			} else {
-				list_del(&xattr->list);
-			}
-			goto out;
-		}
+		if (!strcmp(name, xattr->name))
+			return xattr;
 	}
-	if (flags & XATTR_REPLACE) {
-		xattr = new_xattr;
-		err = -ENODATA;
-	} else {
-		list_add(&new_xattr->list, &xattrs->head);
-		xattr = NULL;
-	}
-out:
-	spin_unlock(&xattrs->lock);
+	return NULL;
+}
+
+static int __simple_xattr_remove(struct simple_xattrs *xattrs,
+				 const char *name)
+{
+	struct simple_xattr *xattr;
+
+	xattr = __find_xattr(xattrs, name);
 	if (xattr) {
+		list_del(&xattr->list);
 		kfree(xattr->name);
 		kfree(xattr);
+		return 0;
 	}
-	return err;
 
+	return -ENODATA;
+}
+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems
+ */
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
+{
+	int rc;
+
+	spin_lock(&xattrs->lock);
+	rc = __simple_xattr_remove(xattrs, name);
+	spin_unlock(&xattrs->lock);
+
+	return rc;
 }
 
 /**

@@ -910,17 +901,54 @@
 int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
 		     const void *value, size_t size, int flags)
 {
+	struct simple_xattr *found, *new_xattr;
+	int err = 0;
+
 	if (size == 0)
-		value = ""; /* empty EA, do not remove */
-	return __simple_xattr_set(xattrs, name, value, size, flags);
-}
+		value = ""; /* empty EA */
 
-/*
- * xattr REMOVE operation for in-memory/pseudo filesystems
- */
-int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
-{
-	return __simple_xattr_set(xattrs, name, NULL, 0, XATTR_REPLACE);
+	/* if value == NULL is specified, remove the item */
+	if (value == NULL)
+		return simple_xattr_remove(xattrs, name);
+
+	new_xattr = simple_xattr_alloc(value, size);
+	if (!new_xattr)
+		return -ENOMEM;
+
+	new_xattr->name = kstrdup(name, GFP_KERNEL);
+	if (!new_xattr->name) {
+		kfree(new_xattr);
+		return -ENOMEM;
+	}
+
+	spin_lock(&xattrs->lock);
+
+	found = __find_xattr(xattrs, name);
+	if (found) {
+		if (flags & XATTR_CREATE) {
+			err = -EEXIST;
+			goto free_new;
+		}
+
+		list_replace(&found->list, &new_xattr->list);
+		kfree(found->name);
+		kfree(found);
+	} else {
+		if (flags & XATTR_REPLACE) {
+			err = -ENODATA;
+			goto free_new;
+		}
+
+		list_add_tail(&new_xattr->list, &xattrs->head);
+	}
+
+out:
+	spin_unlock(&xattrs->lock);
+	return err;
+free_new:
+	kfree(new_xattr->name);
+	kfree(new_xattr);
+	goto out;
 }
 
 static bool xattr_is_trusted(const char *name)

Tejun Heo

2012-10-31 21:11 UTC #7 lore raw

Hello, sorry about the delay.

Just one nitpick.

Quoted 19 lines · Aristeu Rozanski

fs/xattr.c +13 -1

+static int __simple_xattr_remove(struct simple_xattrs *xattrs,
+				 const char *name)
+{
+	struct simple_xattr *xattr;
+
+	xattr = __find_xattr(xattrs, name);
 	if (xattr) {
+		list_del(&xattr->list);
 		kfree(xattr->name);
 		kfree(xattr);
+		return 0;
 	}
-	return err;
 
+	return -ENODATA;
+}
+
+/*
+ * xattr REMOVE operation for in-memory/pseudo filesystems

fs/xattr.c line 884

Quoted 10 lines

fs/xattr.c +10 -0

+ */
+int simple_xattr_remove(struct simple_xattrs *xattrs, const char *name)
+{
+	int rc;
+
+	spin_lock(&xattrs->lock);
+	rc = __simple_xattr_remove(xattrs, name);
+	spin_unlock(&xattrs->lock);
+
+	return rc;

Do we need these two functions? Can't you either collapse __simple_xttar_remove() into simple_xattr_remove() or just call __simple_xattr_remove() directly from simple_xattr_set() with locking handled there? Also, why doesn't simple_xattr_remove() have static?

Thanks.

Signature · 1 line

tejun