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
===================================================================
--- github.orig/fs/xattr.c 2012-10-23 16:02:41.155857391 -0400
+++ github/fs/xattr.c 2012-10-25 11:17:15.118197552 -0400
@@ -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)
On Thu, Oct 25, 2012 at 11:26:14AM -0400, Aristeu Rozanski wrote:
> 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
> ===================================================================
> --- github.orig/fs/xattr.c 2012-10-23 16:02:41.155857391 -0400
> +++ github/fs/xattr.c 2012-10-25 11:17:15.118197552 -0400
> @@ -842,55 +842,46 @@
> return ret;
> }
>
Looks good to me
Reviewed-by: Carlos Maiolino <[email protected]>
--
--Carlos
(cc'ing Hugh and keeping the whole body)
Hello,
On Thu, Oct 25, 2012 at 11:26:14AM -0400, Aristeu Rozanski wrote:
> 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.
> ---
> fs/xattr.c | 124 +++++++++++++++++++++++++++++++++++++------------------------
> 1 file changed, 76 insertions(+), 48 deletions(-)
>
> Index: github/fs/xattr.c
> ===================================================================
> --- github.orig/fs/xattr.c 2012-10-23 16:02:41.155857391 -0400
> +++ github/fs/xattr.c 2012-10-25 11:17:15.118197552 -0400
> @@ -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;
> +}
So, it returns 0 on success and 1 on failure, which in itself isn't a
particularly good idea.
> +
> +/*
> + * 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);
And gets relayed to the caller.
> +
> + 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
On Thu, Oct 25, 2012 at 10:33:26AM -0700, Tejun Heo wrote:
> On Thu, Oct 25, 2012 at 11:26:14AM -0400, Aristeu Rozanski wrote:
> > - 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?
--
Aristeu
Hello,
On Thu, Oct 25, 2012 at 10:54 AM, Aristeu Rozanski <[email protected]> wrote:
>> 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.
--
tejun
On Thu, Oct 25, 2012 at 10:59:35AM -0700, Tejun Heo wrote:
> On Thu, Oct 25, 2012 at 10:54 AM, Aristeu Rozanski <[email protected]> wrote:
> >> 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!
--
Aristeu
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(-)
Index: github/fs/xattr.c
===================================================================
--- github.orig/fs/xattr.c 2012-10-23 16:02:41.155857391 -0400
+++ github/fs/xattr.c 2012-10-25 11:17:15.118197552 -0400
@@ -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)
Hello, sorry about the delay.
Just one nitpick.
On Thu, Oct 25, 2012 at 02:30:18PM -0400, Aristeu Rozanski wrote:
> +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;
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.
--
tejun