On Wed, Apr 17, 2013 at 11:57 AM, Brian Behlendorf <behlendorf1@xxxxxxxx> wrote:
>
> Here's a patch for the ERANGE error (lightly tested). Sage's patch looks
> good but only covers one of two code paths for xattrs. With zfs they may
> either be stored as a system attribute which is usually close to the dnode
> on disk (zfs set xattr=sa pool/dataset). Or they may be stored in their own
> object which is how it's implemented on Solaris (zfs set xattr=on
> pool/dataset). The second method is still the default for compatibility
> reasons even though it's slower. Sage's patch only covered the SA case.
>
>
>> Well, looking at the code again it's not going to work, as setxattr is
>> going to fail with ERANGE.
>
> Why? We support an arbitrary number of maximum sized xattrs (65536). What
> am I missing here?
>
> Incidentally, does anybody know of an good xattr test suite we could add to
> our regression tests?
>
> Thanks,
> Brian
>
> diff --git a/module/zfs/zpl_xattr.c b/module/zfs/zpl_xattr.c
> index c03764f..9f4d63c 100644
> --- a/module/zfs/zpl_xattr.c
> +++ b/module/zfs/zpl_xattr.c
> @@ -225,6 +225,11 @@ zpl_xattr_get_dir(struct inode *ip, const char *name,
> void *value,
> goto out;
> }
>
> + if (size < i_size_read(xip)) {
> + error = -ERANGE;
> + goto out;
> + }
> +
> error = zpl_read_common(xip, value, size, 0, UIO_SYSSPACE, 0, cr);
> out:
> if (xip)
> @@ -263,7 +268,10 @@ zpl_xattr_get_sa(struct inode *ip, const char *name,
> void *value, size_t size)
> if (!size)
> return (nv_size);
>
> - memcpy(value, nv_value, MIN(size, nv_size));
>
> + if (size < nv_size)
> + return (-ERANGE);
Note, that zpl_xattr_get_sa() is called by __zpl_xattr_get() which can
also be called by zpl_xattr_get() to test for xattr existence. So it
needs to make sure that zpl_xattr_set() doesn't fail if getting
-ERANGE.
> +
> + memcpy(value, nv_value, size);
>
> return (MIN(size, nv_size));
No need for MIN() here.
Yehuda
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
