Hi,
On 10 Apr 2014, at 08:30, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
> On Wed, 2014-04-09 at 22:53 +0100, Hin-Tak Leung wrote:
> [snip]
>> @@ -127,7 +128,7 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
>> struct inode *inode = file_inode(file);
>> struct super_block *sb = inode->i_sb;
>> int len, err;
>> - char strbuf[HFSPLUS_MAX_STRLEN + 1];
>> + char *strbuf;
>> hfsplus_cat_entry entry;
>> struct hfs_find_data fd;
>> struct hfsplus_readdir_data *rd;
>> @@ -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);
>
> I think that if you use kzalloc for second case then it makes sense to
> use kzalloc and here. Anyway, kzalloc can be much more safe way, from my
> viewpoint.
Why are you wanting to throw away CPU time on zeroing buffers when it is not needed? And anyway, what about the fact that in readdir the buffer is used several times in a loop thus after the first use it will no longer be zeroed! So you are now saying the buffer should be zeroed out between each use, too? Honestly, if your programming abilities cannot cope with non-zeroed buffers you should not be touching kernel code!!!
If anything the xattr name buffer does not need kzalloc() and should just have the first byte set to zero instead which is much more CPU effective (if that is even needed - depends on the use case of the buffer). 1 byte vs high-hundreds of bytes being set can make a huge performance difference on embedded with slow RAM... But at least there is a justification why you might want a zeroed buffer where there is none at all for readdir case where the buffer keeps getting reused without reinitialization thus it by definition must cope with a non-initialized buffer already.
Best regards,
Anton
> [snip]
>
>> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
>> index 4e27edc..3034ce6 100644
>> --- a/fs/hfsplus/xattr.c
>> +++ b/fs/hfsplus/xattr.c
>> @@ -8,6 +8,7 @@
>>
>
>>
>> + strbuf = kzalloc(NLS_MAX_CHARSET_SIZE * HFSPLUS_ATTR_MAX_STRLEN +
>> + XATTR_MAC_OSX_PREFIX_LEN + 1, GFP_KERNEL);
>> + if (!strbuf) {
>> + res = -ENOMEM;
>> + goto out;
>> + }
>> +
>
> Thanks,
> Vyacheslav Dubeyko.
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge
J.J. Thomson Avenue, Cambridge, CB3 0RB, UK
--
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
