[PATCH 1/6] btrfs-progs: fix floating point exception for btrfs-calc-size

[PATCH 1/6] btrfs-progs: fix floating point exception for btrfs-calc-size

[Zhao Lei]

Current code exit with floating point exception on a blank fs:
 # btrfs-calc-size -b /dev/sda6
 Calculating size of root tree
         Total size: 16384
                 Inline data: 0
         Total seeks: 0
                 Forward seeks: 0
                 Backward seeks: 0
 Floating point exception

This patch add a condition check for above case.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 btrfs-calc-size.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index b756693..17d44ae 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -372,8 +372,8 @@ out_print:
 		printf("\tTotal seeks: %Lu\n", stat.total_seeks);
 		printf("\t\tForward seeks: %Lu\n", stat.forward_seeks);
 		printf("\t\tBackward seeks: %Lu\n", stat.backward_seeks);
-		printf("\t\tAvg seek len: %Lu\n", stat.total_seek_len /
-		       stat.total_seeks);
+		printf("\t\tAvg seek len: %llu\n", stat.total_seeks ?
+			stat.total_seek_len / stat.total_seeks : 0);
 		print_seek_histogram(&stat);
 		printf("\tTotal clusters: %Lu\n", stat.total_clusters);
 		printf("\t\tAvg cluster size: %Lu\n", stat.total_cluster_size /
-- 
1.8.5.1

[PATCH 2/6] btrfs-progs: Fix negative eb's ref_cnt in btrfs-calc-size

[Zhao Lei]

btrfs-calc-size show following warning:
 # btrfs-calc-size /dev/sda6
 Calculating size of root tree
 ...
 extent_io.c:582: free_extent_buffer: Assertion `eb->refs < 0` failed.
 ./btrfs-calc-size[0x41d642]
 ./btrfs-calc-size(free_extent_buffer+0x70)[0x41e1c1]
 ./btrfs-calc-size(btrfs_free_fs_root+0x11)[0x40e1e8]
 ./btrfs-calc-size[0x40e215]
 ./btrfs-calc-size(rb_free_nodes+0x1d)[0x4326fe]
 ./btrfs-calc-size(close_ctree+0x3f3)[0x40f9ea]
 ./btrfs-calc-size(main+0x200)[0x431b4e]
 /lib64/libc.so.6(__libc_start_main+0xf5)[0x3858621d65]
 ./btrfs-calc-size[0x407009]

Reason:
 path in calc_root_size() is only used to save node data,
 it don't hold ref_cnt for each eb in.
 Using btrfs_free_path() to free path will reduce these eb
 again, and cause many problems, as negative ref_cnt or
 invalid memory access.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 btrfs-calc-size.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/btrfs-calc-size.c b/btrfs-calc-size.c
index 17d44ae..b84cda9 100644
--- a/btrfs-calc-size.c
+++ b/btrfs-calc-size.c
@@ -417,7 +417,14 @@ out:
 		free(seek);
 	}
 
-	btrfs_free_path(path);
+	/*
+	 * We only use path to save node data in iterating,
+	 * without holding eb's ref_cnt in path.
+	 * Don't use btrfs_free_path() here, it will free these
+	 * eb again, and cause many problems, as negative ref_cnt
+	 * or invalid memory access.
+	 */
+	free(path);
 	return ret;
 }
 
-- 
1.8.5.1

[PATCH 3/6] btrfs-progs: free fslabel for btrfs-convert

[Zhao Lei]

fslabel need to be freed before exit.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 btrfs-convert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 5b9171e..1693d03 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -3027,6 +3027,9 @@ int main(int argc, char *argv[])
 		ret = do_convert(file, datacsum, packing, noxattr, nodesize,
 				copylabel, fslabel, progress, features);
 	}
+
+	free(fslabel);
+
 	if (ret)
 		return 1;
 	return 0;
-- 
1.8.5.1

Re: [PATCH 3/6] btrfs-progs: free fslabel for btrfs-convert

[David Sterba]

On Thu, Oct 29, 2015 at 05:31:45PM +0800, Zhao Lei wrote:
> fslabel need to be freed before exit.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  btrfs-convert.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/btrfs-convert.c b/btrfs-convert.c
> index 5b9171e..1693d03 100644
> --- a/btrfs-convert.c
> +++ b/btrfs-convert.c
> @@ -3027,6 +3027,9 @@ int main(int argc, char *argv[])
>  		ret = do_convert(file, datacsum, packing, noxattr, nodesize,
>  				copylabel, fslabel, progress, features);
>  	}
> +
> +	free(fslabel);

fslabel is on stack:

btrfs-convert.c: In function 'main':
btrfs-convert.c:3031:6: warning: attempt to free a non-heap object 'fslabel' [-Wfree-nonheap-object]

[PATCH 4/6] btrfs-progs: Fix uninitialized key.type for btrfs_find_free_objectid

[Zhao Lei]

To avoid using uninitialized value in btrfs_search_slot().

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 inode-map.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/inode-map.c b/inode-map.c
index 1321bfb..346952b 100644
--- a/inode-map.c
+++ b/inode-map.c
@@ -44,6 +44,7 @@ int btrfs_find_free_objectid(struct btrfs_trans_handle *trans,
 				BTRFS_FIRST_FREE_OBJECTID);
 	search_key.objectid = search_start;
 	search_key.offset = 0;
+	search_key.type = 0;
 
 	btrfs_init_path(path);
 	start_found = 0;
-- 
1.8.5.1

[PATCH 5/6] btrfs-progs: free comparer_set in cmd_qgroup_show

[Zhao Lei]

comparer_set, which was allocated by malloc(), should be free before
function return.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 cmds-qgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index a64b716..f069d32 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -290,7 +290,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 	int filter_flag = 0;
 	unsigned unit_mode;
 
-	struct btrfs_qgroup_comparer_set *comparer_set;
+	struct btrfs_qgroup_comparer_set *comparer_set = NULL;
 	struct btrfs_qgroup_filter_set *filter_set;
 	filter_set = btrfs_qgroup_alloc_filter_set();
 	comparer_set = btrfs_qgroup_alloc_comparer_set();
@@ -372,6 +372,8 @@ static int cmd_qgroup_show(int argc, char **argv)
 		fprintf(stderr, "ERROR: can't list qgroups: %s\n",
 				strerror(e));
 
+	free(comparer_set);
+
 	return !!ret;
 }
 
-- 
1.8.5.1

Re: [PATCH 5/6] btrfs-progs: free comparer_set in cmd_qgroup_show

[David Sterba]

On Thu, Oct 29, 2015 at 05:31:47PM +0800, Zhao Lei wrote:
> comparer_set, which was allocated by malloc(), should be free before
> function return.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  cmds-qgroup.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index a64b716..f069d32 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -290,7 +290,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>  	int filter_flag = 0;
>  	unsigned unit_mode;
>  
> -	struct btrfs_qgroup_comparer_set *comparer_set;
> +	struct btrfs_qgroup_comparer_set *comparer_set = NULL;
>  	struct btrfs_qgroup_filter_set *filter_set;
>  	filter_set = btrfs_qgroup_alloc_filter_set();
>  	comparer_set = btrfs_qgroup_alloc_comparer_set();
> @@ -372,6 +372,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		fprintf(stderr, "ERROR: can't list qgroups: %s\n",
>  				strerror(e));
>  
> +	free(comparer_set);

Doh, coverity correctly found that comparer_set is freed inside
btrfs_show_qgroups() a few lines above. Patch dropped.

> +

RE: [PATCH 5/6] btrfs-progs: free comparer_set in cmd_qgroup_show

[Zhao Lei]

Hi, David Sterba

> -----Original Message-----
> From: David Sterba [mailto:dsterba@suse.cz]
> Sent: Friday, October 30, 2015 9:36 PM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH 5/6] btrfs-progs: free comparer_set in cmd_qgroup_show
> 
> On Thu, Oct 29, 2015 at 05:31:47PM +0800, Zhao Lei wrote:
> > comparer_set, which was allocated by malloc(), should be free before
> > function return.
> >
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  cmds-qgroup.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/cmds-qgroup.c b/cmds-qgroup.c index a64b716..f069d32
> > 100644
> > --- a/cmds-qgroup.c
> > +++ b/cmds-qgroup.c
> > @@ -290,7 +290,7 @@ static int cmd_qgroup_show(int argc, char **argv)
> >  	int filter_flag = 0;
> >  	unsigned unit_mode;
> >
> > -	struct btrfs_qgroup_comparer_set *comparer_set;
> > +	struct btrfs_qgroup_comparer_set *comparer_set = NULL;
> >  	struct btrfs_qgroup_filter_set *filter_set;
> >  	filter_set = btrfs_qgroup_alloc_filter_set();
> >  	comparer_set = btrfs_qgroup_alloc_comparer_set();
> > @@ -372,6 +372,8 @@ static int cmd_qgroup_show(int argc, char **argv)
> >  		fprintf(stderr, "ERROR: can't list qgroups: %s\n",
> >  				strerror(e));
> >
> > +	free(comparer_set);
> 
> Doh, coverity correctly found that comparer_set is freed inside
> btrfs_show_qgroups() a few lines above. Patch dropped.
> 
My bad.

This problem is find in my node by valgrind memckeck, maybe it
is not freed in some case, or a valgrind misreport.
I'll check it deeply.

Thanks
Zhaolei

> > +

[PATCH 6/6] btrfs-progs: Avoid use pointer in handle_options

[Zhao Lei]

We use pointer of argc and argv in handle_options() because they
are necessary in very old code which are not exist now.

This patch move to use argc and argv directly in handle_options(),
alone with following update:
1: rename handle_options() to check_options()
   to fit its function.
2: cleanup for condition in handle_options() to make line short.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 btrfs.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/btrfs.c b/btrfs.c
index 9416a29..f881c18 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -172,20 +172,22 @@ static int cmd_version(int argc, char **argv)
 	return 0;
 }
 
-static void handle_options(int *argc, char ***argv)
+static void check_options(int argc, char **argv)
 {
-	if (*argc > 0) {
-		const char *arg = (*argv)[0];
-		if (arg[0] != '-' ||
-		    !strcmp(arg, "--help") ||
-		    !strcmp(arg, "--version"))
-			return;
-		fprintf(stderr, "Unknown option: %s\n", arg);
-		fprintf(stderr, "usage: %s\n",
-			btrfs_cmd_group.usagestr[0]);
-		exit(129);
-	}
-	return;
+	if (argc == 0)
+		return;
+
+	const char *arg = argv[0];
+
+	if (arg[0] != '-' ||
+	    !strcmp(arg, "--help") ||
+	    !strcmp(arg, "--version"))
+		return;
+
+	fprintf(stderr, "Unknown option: %s\n", arg);
+	fprintf(stderr, "usage: %s\n",
+		btrfs_cmd_group.usagestr[0]);
+	exit(129);
 }
 
 static const struct cmd_group btrfs_cmd_group = {
@@ -227,7 +229,7 @@ int main(int argc, char **argv)
 	} else {
 		argc--;
 		argv++;
-		handle_options(&argc, &argv);
+		check_options(argc, argv);
 		if (argc > 0) {
 			if (!prefixcmp(argv[0], "--"))
 				argv[0] += 2;
-- 
1.8.5.1

Re: [PATCH 6/6] btrfs-progs: Avoid use pointer in handle_options

[David Sterba]

On Thu, Oct 29, 2015 at 05:31:48PM +0800, Zhao Lei wrote:
> +static void check_options(int argc, char **argv)
>  {
> +	if (argc == 0)
> +		return;
> +
> +	const char *arg = argv[0];

Declaration after statements, fixed at commit time.

> +
> +	if (arg[0] != '-' ||
> +	    !strcmp(arg, "--help") ||
> +	    !strcmp(arg, "--version"))
> +		return;
> +
> +	fprintf(stderr, "Unknown option: %s\n", arg);
> +	fprintf(stderr, "usage: %s\n",
> +		btrfs_cmd_group.usagestr[0]);
> +	exit(129);
>  }
>  

Re: [PATCH 1/6] btrfs-progs: fix floating point exception for btrfs-calc-size

[David Sterba]

Hi,

paches 1,2,4,5,6 applied, thanks.