------------------------------
On Thu, Apr 10, 2014 8:30 AM BST Vyacheslav Dubeyko 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.
>
The difference is simply to avoid unforseeable regression - the stack buffer for names
wasn't null terminated before the switch to dynamic allocation, therefore
it is switch to plain kmalloc; but the xattr one is null-terminated at initialization, so
I used kzalloc when switching, just to be on the safe side. It is really just about
"changing like to like", avoiding unforseeable regression.
I can also see in the latter case, it is being used subsequently in strcpy,
so null-terminate is probably important. Whereas the name is being passed
around with its length.
I think Anton has a point there - it might be more efficient to do kmalloc, and if
necessary, a single assignment to the first byte only, rather than calling kzalloc
(which we are doing, BTW, 127 x 6 ~ over 750 bytes). I am hoping kzalloc
is cleverer and does say, blocks of 4 bytes or even 64 at one go, which won't suck too
much, compared to just one byte assignment. I wish there is an intermediate
between kmalloc and kzalloc where it zero's only the first byte.
I hope this answer the question. Perhaps kzalloc is not needed, but
since the code started with "strbuf[size] = {0};", it is safer to switch to kzalloc
without making the change too ugly.
I mean "safer" in the sense of "without causing
unforseeable regressions", *not* safer in the general philosophical sense of
"zero'ed buffers always better than non-zero'ed".
Hin-Tak
>[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.
>
>
--
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
