Skip to contentSkip to conversation

btrfs-progs: some bugfixes

PATCH RESEND In review [email protected] 3 participants 10 messages Sep 25, 2012 – Sep 26, 2012 Btrfs file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

v2 cover btrfs-progs: some bugfixes

From: Zhi Yong Wu <[email protected]>

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(-)
Signature · 1 line
1.7.6.5
author
#1 lore raw

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

From: Zhi Yong Wu <[email protected]>

Need to close fd on exit.

Signed-off-by Zhi Yong Wu <[email protected]>
---
 cmds-filesystem.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
cmds-filesystem.c +8 -2
index b1457de..e62c4fd 100644
@@ -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;
Signature · 1 line
1.7.6.5
#2 lore raw

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

Quoted 3 lines · On Tue, Sep 25, 2012 at 10:02:15AM +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>

Need to close fd on exit.

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

david

author
#3 lore raw

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

Quoted 6 lines · David Sterba
Quoted 3 lines · On Tue, Sep 25, 2012 at 10:02:15AM +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>

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.

Quoted 2 lines

david

Signature · 3 lines
Regards,

Zhi Yong Wu
author
#4 lore raw

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

In reply to [email protected]

From: Zhi Yong Wu <[email protected]>

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 <[email protected]>
---
 cmds-filesystem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
cmds-filesystem.c +4 -2
index e62c4fd..9c43d35 100644
@@ -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;
 	}
Signature · 1 line
1.7.6.5
#5 lore raw

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

Quoted 3 lines · On Tue, Sep 25, 2012 at 10:02:16AM +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>

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

author
#6 lore raw

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

Quoted 6 lines · David Sterba
Quoted 3 lines · On Tue, Sep 25, 2012 at 10:02:16AM +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>

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?

Quoted 4 lines

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.

Quoted 2 lines

david

Signature · 3 lines
Regards,

Zhi Yong Wu
#7 lore raw

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

In reply to David Sterba
Quoted 6 lines · David Sterba
Quoted 3 lines · On Tue, Sep 25, 2012 at 10:02:16AM +0800, [email protected] wrote:

From: Zhi Yong Wu<[email protected]>

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).

Quoted 3 lines

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

Quoted 8 lines

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

author
#8 lore raw

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

Quoted 29 lines · Goffredo Baroncelli
Quoted 9 lines · David Sterba
Quoted 4 lines · On Tue, Sep 25, 2012 at 10:02:16AM +0800, [email protected] wrote:

From: Zhi Yong Wu<[email protected]>

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).

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

My 2¢

BR
G.Baroncelli

Quoted 9 lines

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
.

Signature · 3 lines
Regards,

Zhi Yong Wu
#9 lore raw

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

In reply to Goffredo Baroncelli
Quoted 4 lines · Goffredo Baroncelli

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/