Re: [PATCH] Revised extended attributes interface

Anton Altaparmakov
2001-12-05T09:08:49Z

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/