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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrzej Hajda
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 <[hidden email]>
---
 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


------------------------------------------------------------------------------
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrzej Hajda
Ping.

Regards
Andrzej

On 08/07/2015 09:59 AM, Andrzej Hajda 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 <[hidden email]>
> ---
>  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;


------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Anton Altaparmakov-2
In reply to this post by Andrzej Hajda
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?

Best regards,

        Anton

> On 7 Aug 2015, at 08:59, Andrzej Hajda <[hidden email]> 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 <[hidden email]>
> ---
> 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


------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrzej Hajda
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.

Regards
Andrzej

>
> Best regards,
>
> Anton
>
>> On 7 Aug 2015, at 08:59, Andrzej Hajda <[hidden email]> 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 <[hidden email]>
>> ---
>> 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


------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Anton Altaparmakov-2
In reply to this post by Anton Altaparmakov-2
Hi Andrzej,

> On 24 Sep 2015, at 09:34, Andrzej Hajda <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>>> ---
>>> 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


------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Anton Altaparmakov-2

> On 24 Sep 2015, at 10:20, Anton Altaparmakov <[hidden email]> wrote:
>
> Hi Andrzej,
>
>> On 24 Sep 2015, at 09:34, Andrzej Hajda <[hidden email]> 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 <[hidden email]>

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 <[hidden email]> 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 <[hidden email]>
>>>> ---
>>>> 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


------------------------------------------------------------------------------
Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
Get real-time metrics from all of your servers, apps and tools
in one place.
SourceForge users - Click here to start your Free Trial of Datadog now!
http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Loading...