------------------------------
On Tue, Apr 8, 2014 8:05 AM BST Vyacheslav Dubeyko wrote:
>Hi Hin-Tak,
>
>On Tue, 2014-04-08 at 03:19 +0100, Hin-Tak Leung wrote:
>
>[snip]
>
> @@ -139,6 +140,11 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
> err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> if (err)
> return err;
> + strbuf = kmalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN + 1, GFP_KERNEL);
>
>Why do you prefer kmalloc instead of kmem_cache_alloc? I suppose that
>operations with names are frequent operations. So, it makes sense to use
>look-aside cache, from my viewpoint. What do you think?
>
I just wasn't sure which *alloc does what :-). Even kmalloc vs kzalloc vz kcalloc
took a while to find. (and kcalloc isn't the kernel equivalent of calloc, kzalloc is).
Any reference to which *alloc does what?
>Maybe it makes sense to define special constants for
>(NLS_MAX_CHARSET_SIZE * HFSPLUS_MAX_STRLEN) and for
>(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN)?
>
>[snip]
>
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 4e27edc..cfbb23d 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -8,6 +8,7 @@
>
> #include "hfsplus_fs.h"
> #include <linux/posix_acl_xattr.h>
> +#include <linux/nls.h>
> #include "xattr.h"
> #include "acl.h"
>
> @@ -645,8 +646,7 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, char *buffer, size_t size)
> struct hfs_find_data fd;
> u16 key_len = 0;
> struct hfsplus_attr_key attr_key;
> - char strbuf[HFSPLUS_ATTR_MAX_STRLEN +
> - XATTR_MAC_OSX_PREFIX_LEN + 1] = {0};
>
>I think that you missed likewise cases in:
I did not miss them - my patch specifically addresses what comes out
of hfsplus_uni2asc() (maybe I should be clearer about that).
There are only two usages of it, and that's what my patch does.
I had a look at the other usages of HFSPLUS_ATTR_MAX_STRLEN
- some of them are about setting (instead of getting) attributes.
(and quite strangely, some of them says "this is not used" - in one version
of kernel source I check out).
I just think the other usages of HFSPLUS_ATTR_MAX_STRLEN needs
to be addressed by another patch.
So at least my thought is to address the problem with hfsplus_uni2asc() in
one patch, and other usage of HFSPLUS_ATTR_MAX_STRLEN in another.
Otherwise, we'd have to drop the attribute part of the patch and bundle
that with the attribute related changes, as they are rather large, and also,
as I said, it is not obvious which is get and which is set.
So perhaps I should ask the question - there are only one read-off-diskroutine and one
write-to-disk routine for names (and wrappers around those), which is why it is
quite quick and obvious which one needs to be fixed for names.
But for attributes, again there is *one* place where hfsplus_uni2asc() is used,
but there seems to be a whole lot of other things going on.
In any case, many of them also needs to switch from stack allocation
to heap allocation (and error propagation for those!). It needs to be in a different
patch.
Hin-Tak
>(1) hfsplus_attr_build_key()
>(http://lxr.free-electrons.com/source/fs/hfsplus/attributes.c#L58)
>
>(2) xattr_security.c
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L17)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L23)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L35)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L41)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L65)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_security.c#L76)
>
>(3) xattr_trusted.c
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_trusted.c#L15)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_trusted.c#L21)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_trusted.c#L33)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_trusted.c#L39)
>
>(4) xattr_user.c
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_user.c#L15)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_user.c#L21)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_user.c#L33)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr_user.c#L39)
>
>(5) hfsplus_osx_getxattr()
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr.c#L800)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr.c#L807)
>
>(6) hfsplus_osx_setxattr()
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr.c#L823)
>(http://lxr.free-electrons.com/source/fs/hfsplus/xattr.c#L830)
>
>Thanks,
>Vyacheslav Dubeyko.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
