[resend][PATCH v2 0/2] btrfs-progs: some bugfixes

[resend][PATCH v2 0/2] btrfs-progs: some bugfixes

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

  Some misc bugs are found when i work on other tasks.
Now send out them for interview, thanks.

Zhi Yong Wu (2):
  btrfs-progs: Close file descriptor on exit
  btrfs-progs: Fix up memory leakage

 cmds-filesystem.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

-- 
1.7.6.5

[PATCH v2 1/2] btrfs-progs: Close file descriptor on exit

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

  Need to close fd on exit.

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 cmds-filesystem.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b1457de..e62c4fd 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -77,18 +77,23 @@ static int cmd_df(int argc, char **argv)
 	if (ret) {
 		fprintf(stderr, "ERROR: couldn't get space info on '%s' - %s\n",
 			path, strerror(e));
+		close(fd);
 		free(sargs);
 		return ret;
 	}
-	if (!sargs->total_spaces)
+	if (!sargs->total_spaces) {
+		close(fd);
 		return 0;
+	}
 
 	count = sargs->total_spaces;
 
 	sargs = realloc(sargs, sizeof(struct btrfs_ioctl_space_args) +
 			(count * sizeof(struct btrfs_ioctl_space_info)));
-	if (!sargs)
+	if (!sargs) {
+		close(fd);
 		return -ENOMEM;
+	}
 
 	sargs->space_slots = count;
 	sargs->total_spaces = 0;
@@ -148,6 +153,7 @@ static int cmd_df(int argc, char **argv)
 		printf("%s: total=%s, used=%s\n", description, total_bytes,
 		       used_bytes);
 	}
+	close(fd);
 	free(sargs);
 
 	return 0;
-- 
1.7.6.5

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

[zwu.kernel]

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

  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 <wuzhy@linux.vnet.ibm.com>
---
 cmds-filesystem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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;
 	}
 
-- 
1.7.6.5

Re: [PATCH v2 1/2] btrfs-progs: Close file descriptor on exit

[David Sterba]

On Tue, Sep 25, 2012 at 10:02:15AM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
>   Need to close fd on exit.

Strictly you don't need to, kernel will do that at exit() time.

david

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

[David Sterba]

On Tue, Sep 25, 2012 at 10:02:16AM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
>   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

Re: [PATCH v2 1/2] btrfs-progs: Close file descriptor on exit

[Zhi Yong Wu]

On Tue, Sep 25, 2012 at 6:12 PM, David Sterba <dave@jikos.cz> wrote:
> On Tue, Sep 25, 2012 at 10:02:15AM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>>   Need to close fd on exit.
>
> Strictly you don't need to, kernel will do that at exit() time.
I know, but it is not so nice.

>
> david



-- 
Regards,

Zhi Yong Wu

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

[Zhi Yong Wu]

On Tue, Sep 25, 2012 at 6:14 PM, David Sterba <dave@jikos.cz> wrote:
> On Tue, Sep 25, 2012 at 10:02:16AM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>>   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



-- 
Regards,

Zhi Yong Wu

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

[Goffredo Baroncelli]

On 09/25/2012 12:14 PM, David Sterba wrote:
> On Tue, Sep 25, 2012 at 10:02:16AM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com>
>>
>>    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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> .
>

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

[Zhi Yong Wu]

On Wed, Sep 26, 2012 at 1:14 AM, Goffredo Baroncelli <kreijack@libero.it> wrote:
> On 09/25/2012 12:14 PM, David Sterba wrote:
>>
>> On Tue, Sep 25, 2012 at 10:02:16AM +0800, zwu.kernel@gmail.com wrote:
>>>
>>> From: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com>
>>>
>>>    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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> .
>>
>



-- 
Regards,

Zhi Yong Wu

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

[Goffredo Baroncelli]

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/

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

[Zhi Yong Wu]

  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 <wuzhy@linux.vnet.ibm.com>
---
 cmds-filesystem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

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;
 	}
 
-- 
1.7.6.5