[v2,2/2] btrfs-progs: Fix up memory leakage

Commit Message

Zhiyong Wu Sept. 25, 2012, 2:02 a.m. UTC

From: Zhi Yong Wu <[e-mail]>

  Some code pathes forget to free memory on exit.

Changelog from v1:
  Fix the variable is used uncorrectly. [Ram Pai]

Signed-off-by: Zhi Yong Wu <[e-mail]>
---
 cmds-filesystem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Comments

David Sterba Sept. 25, 2012, 10:14 a.m. UTC | #1

On Tue, Sep 25, 2012 at 10:02:16AM +0800, [e-mail] wrote:
> From: Zhi Yong Wu <[e-mail]>
> 
>   Some code pathes forget to free memory on exit.

Same as with the fd's, kernel will free all memory for us at exit().

If there's lots of memory allocated, it may be even faster to leave the
unallocation process to kernel as it will do it in one go, while the
application would unnecessarily free it chunk by chunk.

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhiyong Wu Sept. 25, 2012, 2:03 p.m. UTC | #2

On Tue, Sep 25, 2012 at 6:14 PM, David Sterba <[e-mail]> wrote:
> On Tue, Sep 25, 2012 at 10:02:16AM +0800, [e-mail] wrote:
>> From: Zhi Yong Wu <[e-mail]>
>>
>>   Some code pathes forget to free memory on exit.
>
> Same as with the fd's, kernel will free all memory for us at exit().
hi, can you let me know the pointer to exit() said by you?

>
> If there's lots of memory allocated, it may be even faster to leave the
> unallocation process to kernel as it will do it in one go, while the
> application would unnecessarily free it chunk by chunk.
got it, thanks.
>
> david

Goffredo Baroncelli Sept. 25, 2012, 5:14 p.m. UTC | #3

On 09/25/2012 12:14 PM, David Sterba wrote:
> On Tue, Sep 25, 2012 at 10:02:16AM +0800, [e-mail] wrote:
>> From: Zhi Yong Wu<[e-mail]>
>>
>>    Some code pathes forget to free memory on exit.
>
> Same as with the fd's, kernel will free all memory for us at exit().

I strongly disagree with this approach. The callee often don't know what 
happen after and before the call. The same is true for the programmer, 
because the code is quite often updated by several people. A clean 
exit() is the right thing to do as general rule. I don't see any valid 
reason (in the btrfs context) to do otherwise.

Relying on the exit() for a proper clean-up increase the likelihood of 
bug when the code evolves (see my patch   [RESPOST][BTRFS-PROGS][PATCH] 
btrfs_read_dev_super(): uninitialized variable for an example of what 
means an incorrect deallocation of resource).

> If there's lots of memory allocated, it may be even faster to leave the
> unallocation process to kernel as it will do it in one go, while the
> application would unnecessarily free it chunk by chunk.

May be I am wrong, but I don't think that the increase of speed of the 
btrfs "command" is even measurable relying on exit instead of free()-ing 
each chunk of memory one at time.... The same should be true for the 
open()/close()

My 2¢

BR
G.Baroncelli

>
> david
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zhiyong Wu Sept. 26, 2012, 2:58 a.m. UTC | #4

On Wed, Sep 26, 2012 at 1:14 AM, Goffredo Baroncelli <[e-mail]> wrote:
> On 09/25/2012 12:14 PM, David Sterba wrote:
>>
>> On Tue, Sep 25, 2012 at 10:02:16AM +0800, [e-mail] wrote:
>>>
>>> From: Zhi Yong Wu<[e-mail]>
>>>
>>>    Some code pathes forget to free memory on exit.
>>
>>
>> Same as with the fd's, kernel will free all memory for us at exit().
>
>
> I strongly disagree with this approach. The callee often don't know what
> happen after and before the call. The same is true for the programmer,
> because the code is quite often updated by several people. A clean exit() is
> the right thing to do as general rule. I don't see any valid reason (in the
> btrfs context) to do otherwise.
>
> Relying on the exit() for a proper clean-up increase the likelihood of bug
> when the code evolves (see my patch   [RESPOST][BTRFS-PROGS][PATCH]
> btrfs_read_dev_super(): uninitialized variable for an example of what means
> an incorrect deallocation of resource).
>
>
>> If there's lots of memory allocated, it may be even faster to leave the
>> unallocation process to kernel as it will do it in one go, while the
>> application would unnecessarily free it chunk by chunk.
>
>
> May be I am wrong, but I don't think that the increase of speed of the btrfs
> "command" is even measurable relying on exit instead of free()-ing each
> chunk of memory one at time.... The same should be true for the
> open()/close()

I fully agree with you. In one same function, i find that some code
path free system sources,
while other code path doesn't. This is one nice way.

>
> My 2¢
>
> BR
> G.Baroncelli
>
>>
>> david
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> .
>>
>

Goffredo Baroncelli Sept. 26, 2012, 9:13 p.m. UTC | #5

On 09/25/2012 07:14 PM, Goffredo Baroncelli wrote:
> I strongly disagree with this approach. The callee often don't know what
> happen after and before the call. The same is true for the programmer,
> because the code is quite often updated by several people. A clean
> exit() is the right thing to do as general rule.

My fingers were faster than my brain :-)
	s/clean exit()/clean-up/
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index e62c4fd..9c43d35 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -47,7 +47,7 @@  static const char * const cmd_df_usage[] = {
 
 static int cmd_df(int argc, char **argv)
 {
-	struct btrfs_ioctl_space_args *sargs;
+	struct btrfs_ioctl_space_args *sargs, *sargs_orig;
 	u64 count = 0, i;
 	int ret;
 	int fd;
@@ -65,7 +65,7 @@  static int cmd_df(int argc, char **argv)
 		return 12;
 	}
 
-	sargs = malloc(sizeof(struct btrfs_ioctl_space_args));
+	sargs_orig = sargs = malloc(sizeof(struct btrfs_ioctl_space_args));
 	if (!sargs)
 		return -ENOMEM;
 
@@ -83,6 +83,7 @@  static int cmd_df(int argc, char **argv)
 	}
 	if (!sargs->total_spaces) {
 		close(fd);
+		free(sargs);
 		return 0;
 	}
 
@@ -92,6 +93,7 @@  static int cmd_df(int argc, char **argv)
 			(count * sizeof(struct btrfs_ioctl_space_info)));
 	if (!sargs) {
 		close(fd);
+		free(sargs_orig);
 		return -ENOMEM;
 	}