Re: [PATCH] Revised extended attributes interface

[Anton Altaparmakov <aia21@cam.ac.uk>]

At 03:32 05/12/01, Nathan Scott wrote:
>Here is the revised interface.  I believe it takes into account
>the issues raised so far - further suggestions are also welcome,
>of course.

Hi,

Looks good to me. Just one tiny point: you seem to like setting error=xyz; 
a lot which is completely unnecessary some times. Any particular reason? 
Here is an example of what I mean:

>+static long
>+setxattr(struct dentry *d, char *name, void *value, size_t size, int flags)
>+{
>+       int error;
>+       void *kvalue;
>+       char kname[XATTR_NAME_MAX + 1];
>+
>+       error = -EINVAL;
>+       if (flags & ~(XATTR_CREATE|XATTR_REPLACE))
>+               return error;

Why not:

+       if (flags & ~(XATTR_CREATE|XATTR_REPLACE))
+               return -EINVAL;

>+
>+       error = -EFAULT;
>+       if (copy_from_user(kname, name, XATTR_NAME_MAX))
>+               return error;

+       if (copy_from_user(kname, name, XATTR_NAME_MAX))
+               return -EFAULT;

>+       kname[XATTR_NAME_MAX] = '\0';
>+
>+       kvalue = xattr_alloc(size, XATTR_SIZE_MAX);
>+       if (IS_ERR(kvalue))
>+               return PTR_ERR(kvalue);
>+
>+       error = -EFAULT;
>+       if (size > 0 && copy_from_user(kvalue, value, size)) {
>+               xattr_free(kvalue, size);
>+               return error;
>+       }

+       if (size > 0 && copy_from_user(kvalue, value, size)) {
+               xattr_free(kvalue, size);
+               return -EFAULT;
+       }

Shorter, faster in the common non-error path, and looks nicer, although the 
latter is probably a matter of personal preference. (-;

Best regards,

         Anton


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/