Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

From: Anton Altaparmakov <anton@tuxera.com>
To: Andrzej Hajda <a.hajda@samsung.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-ntfs-dev@lists.sourceforge.net" 
	<linux-ntfs-dev@lists.sourceforge.net>
Subject: Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation
Date: Thu, 24 Sep 2015 10:22:04 +0100	[thread overview]
Message-ID: <17AD20CA-2436-481E-B048-46857710B759@tuxera.com> (raw)
In-Reply-To: <AB2004BE-4B4A-47B8-A21E-7D1F8787B4CA@tuxera.com>

> On 24 Sep 2015, at 10:20, Anton Altaparmakov <anton@tuxera.com> wrote:
> 
> Hi Andrzej,
> 
>> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> 
>> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>>> Hi Andrzej,
>>> 
>>> Thanks for your patch.  It looks fine though I don't quite see the point of it to be honest.
>>> 
>>> It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother?  What does it gain?
>> 
>> kmemdup replaces combo (kmalloc + memdup) with one call.
>> The patch follows quite common practice of abstracting out common patterns.
> 
> Sure it does I am just questioning the sanity of the practice...  (-;
> 
> Such changes reduce the size of the kernel binary by a few bytes at the cost of adding CPU cycles to the execution time.  How is that good thing?  Unless you are on an embedded system desperate for RAM throwing away CPU cycles on pointless abstractions makes no sense to me...
> 
> But as I said patch is fine.  Feel free to send it onto Andrew to get it into mainline.  You can add my Acked-by: Anton Altaparmakoc <anton@tuxera.com>

Altaparmakov even.  Can't even spell my own surname...  )-;

> line to it when sending it.  I am just saying that I think patches like that don't make much sense to me...
> 
> Best regards,
> 
> 	Anton
> 
>> Regards
>> Andrzej
>> 
>>> 
>>> Best regards,
>>> 
>>> 	Anton
>>> 
>>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> 
>>>> The patch was generated using fixed coccinelle semantic patch
>>>> scripts/coccinelle/api/memdup.cocci [1].
>>>> 
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>>> 
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> fs/ntfs/dir.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>>> index 9e38daf..2b7fef0 100644
>>>> --- a/fs/ntfs/dir.c
>>>> +++ b/fs/ntfs/dir.c
>>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>>>> 	 * map the mft record without deadlocking.
>>>> 	 */
>>>> 	rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>>> -	ir = kmalloc(rc, GFP_NOFS);
>>>> +	/* Copy the index root value (it has been verified in read_inode). */
>>>> +	ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
>>>> +		     rc, GFP_NOFS);
>>>> 	if (unlikely(!ir)) {
>>>> 		err = -ENOMEM;
>>>> 		goto err_out;
>>>> 	}
>>>> -	/* Copy the index root value (it has been verified in read_inode). */
>>>> -	memcpy(ir, (u8*)ctx->attr +
>>>> -			le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>>> 	ntfs_attr_put_search_ctx(ctx);
>>>> 	unmap_mft_record(ndir);
>>>> 	ctx = NULL;
>>>> -- 
>>>> 1.9.1
> 
> -- 
> Anton Altaparmakov <anton at tuxera.com> (replace at with @)
> Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
> Linux NTFS maintainer

Best regards,

	Anton
-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer