[PATCH 1/4] btrfs-progs: cleanup duplicate assignment of variable leaf for btrfs-image

[PATCH 1/4] btrfs-progs: cleanup duplicate assignment of variable leaf for btrfs-image

[Gui Hecheng]

The value of variable leaf in while loop don't have to be set
for every round. Just move it outside.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-image.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index cf1fe2d..db5d193 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -1090,8 +1090,9 @@ static int copy_space_cache(struct btrfs_root *root,
 		return ret;
 	}
 
+	leaf = path->nodes[0];
+
 	while (1) {
-		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(root, path);
 			if (ret < 0) {
@@ -1157,8 +1158,9 @@ static int copy_from_extent_tree(struct metadump_struct *metadump,
 	}
 	ret = 0;
 
+	leaf = path->nodes[0];
+
 	while (1) {
-		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			ret = btrfs_next_leaf(extent_root, path);
 			if (ret < 0) {
-- 
1.8.1.4

[PATCH 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[Gui Hecheng]

For btrfs-image,
	dump 	may not come with option '-o'
	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
	-m	may not come with dev_cnt < 2
All of the above should be regarded as invalid combinations,
and the usage will show up.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-image.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index db5d193..549722c 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2462,6 +2462,7 @@ int main(int argc, char *argv[])
 	int ret;
 	int sanitize = 0;
 	int dev_cnt = 0;
+	int usage_error = 0;
 	FILE *out;
 
 	while (1) {
@@ -2500,15 +2501,20 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if ((old_restore) && create)
-		print_usage();
-
 	argc = argc - optind;
 	dev_cnt = argc - 1;
 
-	if (multi_devices && dev_cnt < 2)
-		print_usage();
-	if (!multi_devices && dev_cnt != 1)
+	if (create) {
+		usage_error = old_restore;
+	} else {
+		usage_error = walk_trees || sanitize || compress_level;
+		if (multi_devices)
+			usage_error = usage_error || (dev_cnt < 2);
+		else
+			usage_error = usage_error || (dev_cnt != 1);
+	}
+
+	if (usage_error)
 		print_usage();
 
 	source = argv[optind];
-- 
1.8.1.4

Re: [PATCH 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[David Sterba]

On Thu, Jun 19, 2014 at 09:46:01AM +0800, Gui Hecheng wrote:
> For btrfs-image,
> 	dump 	may not come with option '-o'
> 	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
> 	-m	may not come with dev_cnt < 2
> All of the above should be regarded as invalid combinations,
> and the usage will show up.

The generic usage works, but is not very descriptive what really
happened. Would be good to print a message as soon as an error is
detectd. See below:

> @@ -2500,15 +2501,20 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> -	if ((old_restore) && create)
> -		print_usage();

In this case the old code makes more sense followed by the message that
create and restore cannot be used at the same time.

> -	if (multi_devices && dev_cnt < 2)
> -		print_usage();

Applies to both modes, eg. "not enoug devices specified for -m option".

> -	if (!multi_devices && dev_cnt != 1)
> +	if (create) {
> +		usage_error = old_restore;
> +	} else {
> +		usage_error = walk_trees || sanitize || compress_level;

Error message here if usage_error is set.

> +		if (multi_devices)
> +			usage_error = usage_error || (dev_cnt < 2);
> +		else
> +			usage_error = usage_error || (dev_cnt != 1);

Could be merged with the multi_devices check above, accepts only one
without -m option.

Re: [PATCH 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[Gui Hecheng]

On Mon, 2014-06-23 at 16:12 +0200, David Sterba wrote:
> On Thu, Jun 19, 2014 at 09:46:01AM +0800, Gui Hecheng wrote:
> > For btrfs-image,
> > 	dump 	may not come with option '-o'
> > 	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
> > 	-m	may not come with dev_cnt < 2
> > All of the above should be regarded as invalid combinations,
> > and the usage will show up.
> 
> The generic usage works, but is not very descriptive what really
> happened. Would be good to print a message as soon as an error is
> detectd. See below:
> 
> > @@ -2500,15 +2501,20 @@ int main(int argc, char *argv[])
> >  		}
> >  	}
> >  
> > -	if ((old_restore) && create)
> > -		print_usage();
> 
> In this case the old code makes more sense followed by the message that
> create and restore cannot be used at the same time.
> 
> > -	if (multi_devices && dev_cnt < 2)
> > -		print_usage();
> 
> Applies to both modes, eg. "not enoug devices specified for -m option".
> 
> > -	if (!multi_devices && dev_cnt != 1)
> > +	if (create) {
> > +		usage_error = old_restore;
> > +	} else {
> > +		usage_error = walk_trees || sanitize || compress_level;
> 
> Error message here if usage_error is set.
> 
> > +		if (multi_devices)
> > +			usage_error = usage_error || (dev_cnt < 2);
> > +		else
> > +			usage_error = usage_error || (dev_cnt != 1);
> 
> Could be merged with the multi_devices check above, accepts only one
> without -m option.

Yes, I'll arrange the error messages well soon. Thanks.

[PATCH v2 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[Gui Hecheng]

For btrfs-image,
	dump 	may not come with option '-o'
	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
	-m	may not come with dev_cnt < 2
All of the above should be regarded as invalid combinations,
and the usage will show up.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog
	v1->v2: add error messages for each case
---
 btrfs-image.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index db5d193..c18dff1 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2462,6 +2462,7 @@ int main(int argc, char *argv[])
 	int ret;
 	int sanitize = 0;
 	int dev_cnt = 0;
+	int usage_error = 0;
 	FILE *out;
 
 	while (1) {
@@ -2500,15 +2501,26 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if ((old_restore) && create)
-		print_usage();
-
 	argc = argc - optind;
 	dev_cnt = argc - 1;
 
-	if (multi_devices && dev_cnt < 2)
-		print_usage();
-	if (!multi_devices && dev_cnt != 1)
+	if (create) {
+		usage_error = old_restore;
+		fprintf(stderr, "Usage error: create and restore cannot be used at the same time\n");
+	} else {
+		usage_error = walk_trees || sanitize || compress_level;
+		fprintf(stderr, "Usage error: use -w, -s, -c options for restore makes no sense\n");
+
+		if (multi_devices) {
+			usage_error = usage_error || (dev_cnt < 2);
+			fprintf(stderr, "Usage error: not enough devices specified for -m option\n");
+		} else {
+			usage_error = usage_error || (dev_cnt != 1);
+			fprintf(stderr, "Usage error: accepts only 1 device without -m option\n");
+		}
+	}
+
+	if (usage_error)
 		print_usage();
 
 	source = argv[optind];
-- 
1.8.1.4

Re: [PATCH v2 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[Gui Hecheng]

On Tue, 2014-06-24 at 10:36 +0800, Gui Hecheng wrote:
> For btrfs-image,
> 	dump 	may not come with option '-o'
> 	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
> 	-m	may not come with dev_cnt < 2
> All of the above should be regarded as invalid combinations,
> and the usage will show up.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
> changelog
> 	v1->v2: add error messages for each case
> ---
>  btrfs-image.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/btrfs-image.c b/btrfs-image.c
> index db5d193..c18dff1 100644
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -2462,6 +2462,7 @@ int main(int argc, char *argv[])
>  	int ret;
>  	int sanitize = 0;
>  	int dev_cnt = 0;
> +	int usage_error = 0;
>  	FILE *out;
>  
>  	while (1) {
> @@ -2500,15 +2501,26 @@ int main(int argc, char *argv[])
>  		}
>  	}
>  
> -	if ((old_restore) && create)
> -		print_usage();
> -
>  	argc = argc - optind;
>  	dev_cnt = argc - 1;
>  
> -	if (multi_devices && dev_cnt < 2)
> -		print_usage();
> -	if (!multi_devices && dev_cnt != 1)
> +	if (create) {
> +		usage_error = old_restore;
> +		fprintf(stderr, "Usage error: create and restore cannot be used at the same time\n");
> +	} else {
> +		usage_error = walk_trees || sanitize || compress_level;
> +		fprintf(stderr, "Usage error: use -w, -s, -c options for restore makes no sense\n");
> +
> +		if (multi_devices) {
> +			usage_error = usage_error || (dev_cnt < 2);
> +			fprintf(stderr, "Usage error: not enough devices specified for -m option\n");
> +		} else {
> +			usage_error = usage_error || (dev_cnt != 1);
> +			fprintf(stderr, "Usage error: accepts only 1 device without -m option\n");
> +		}
> +	}
> +
> +	if (usage_error)
>  		print_usage();
>  
>  	source = argv[optind];
Ah, sorry, I forget about the if() judgement, please *ignore* this one.

[PATCH v3 2/4] btrfs-progs: deal with invalid option combinations for btrfs-image

[Gui Hecheng]

For btrfs-image,
	dump 	may not come with option '-o'
	-r	may not come with option '-c', '-s', '-w', dev_cnt != 1
	-m	may not come with dev_cnt < 2
All of the above should be regarded as invalid combinations,
and the usage will show up.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog
	v1->v2: add error messages for each case
	v2->v3: add missing judgements
---
 btrfs-image.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index db5d193..9e5b8b3 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2462,6 +2462,7 @@ int main(int argc, char *argv[])
 	int ret;
 	int sanitize = 0;
 	int dev_cnt = 0;
+	int usage_error = 0;
 	FILE *out;
 
 	while (1) {
@@ -2500,15 +2501,30 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if ((old_restore) && create)
-		print_usage();
-
 	argc = argc - optind;
 	dev_cnt = argc - 1;
 
-	if (multi_devices && dev_cnt < 2)
-		print_usage();
-	if (!multi_devices && dev_cnt != 1)
+	if (create) {
+		if (old_restore) {
+			fprintf(stderr, "Usage error: create and restore cannot be used at the same time\n");
+			usage_error++;
+		}
+	} else {
+		if (walk_trees || sanitize || compress_level) {
+			fprintf(stderr, "Usage error: use -w, -s, -c options for restore makes no sense\n");
+			usage_error++;
+		}
+		if (multi_devices && dev_cnt < 2) {
+			fprintf(stderr, "Usage error: not enough devices specified for -m option\n");
+			usage_error++;
+		}
+		if (!multi_devices && dev_cnt != 1) {
+			fprintf(stderr, "Usage error: accepts only 1 device without -m option\n");
+			usage_error++;
+		}
+	}
+
+	if (usage_error)
 		print_usage();
 
 	source = argv[optind];
-- 
1.8.1.4

[PATCH 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[Gui Hecheng]

When btrfs-image failed to create an image, the invalid output file
had better be deleted to prevent being used mistakenly in the future.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-image.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 549722c..b235b87 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2598,8 +2598,16 @@ int main(int argc, char *argv[])
 out:
 	if (out == stdout)
 		fflush(out);
-	else
+	else {
 		fclose(out);
+		if (ret && create) {
+			ret = unlink(target);
+			if (ret)
+				fprintf(stderr,
+					"unlink output file failed : %s\n",
+					strerror(errno));
+		}
+	}
 
 	return !!ret;
 }
-- 
1.8.1.4

Re: [PATCH 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[David Sterba]

On Thu, Jun 19, 2014 at 09:46:02AM +0800, Gui Hecheng wrote:
> When btrfs-image failed to create an image, the invalid output file
> had better be deleted to prevent being used mistakenly in the future.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
>  btrfs-image.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/btrfs-image.c b/btrfs-image.c
> index 549722c..b235b87 100644
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -2598,8 +2598,16 @@ int main(int argc, char *argv[])
>  out:
>  	if (out == stdout)
>  		fflush(out);
> -	else
> +	else {
>  		fclose(out);
> +		if (ret && create) {
> +			ret = unlink(target);

The previous value is overwritten and in case of unlink() success it's
returned as success of main(), which is not true. So a local variable
has to be used instead.

> +			if (ret)
> +				fprintf(stderr,
> +					"unlink output file failed : %s\n",
> +					strerror(errno));
> +		}
> +	}
>  
>  	return !!ret;

Re: [PATCH 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[Gui Hecheng]

On Mon, 2014-06-23 at 16:15 +0200, David Sterba wrote:
> On Thu, Jun 19, 2014 at 09:46:02AM +0800, Gui Hecheng wrote:
> > When btrfs-image failed to create an image, the invalid output file
> > had better be deleted to prevent being used mistakenly in the future.
> > 
> > Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> > ---
> >  btrfs-image.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/btrfs-image.c b/btrfs-image.c
> > index 549722c..b235b87 100644
> > --- a/btrfs-image.c
> > +++ b/btrfs-image.c
> > @@ -2598,8 +2598,16 @@ int main(int argc, char *argv[])
> >  out:
> >  	if (out == stdout)
> >  		fflush(out);
> > -	else
> > +	else {
> >  		fclose(out);
> > +		if (ret && create) {
> > +			ret = unlink(target);
> 
> The previous value is overwritten and in case of unlink() success it's
> returned as success of main(), which is not true. So a local variable
> has to be used instead.
> 
Yes, I'll rework & resend it soon. Thanks.

> > +			if (ret)
> > +				fprintf(stderr,
> > +					"unlink output file failed : %s\n",
> > +					strerror(errno));
> > +		}
> > +	}
> >  
> >  	return !!ret;

[PATCH v2 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[Gui Hecheng]

When btrfs-image failed to create an image, the invalid output file
had better be deleted to prevent being used mistakenly in the future.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog
	v1->v2: use a new local variable to avoid return value overwritten
---
 btrfs-image.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index c18dff1..321c331 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2604,8 +2604,18 @@ int main(int argc, char *argv[])
 out:
 	if (out == stdout)
 		fflush(out);
-	else
+	else {
 		fclose(out);
+		if (ret && create) {
+			int unlink_ret;
+		       
+			unlink_ret = unlink(target);
+			if (unlink_ret)
+				fprintf(stderr,
+					"unlink output file failed : %s\n",
+					strerror(errno));
+		}
+	}
 
 	return !!ret;
 }
-- 
1.8.1.4

Re: [PATCH v2 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[Gui Hecheng]

On Tue, 2014-06-24 at 10:38 +0800, Gui Hecheng wrote:
> When btrfs-image failed to create an image, the invalid output file
> had better be deleted to prevent being used mistakenly in the future.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
> changelog
> 	v1->v2: use a new local variable to avoid return value overwritten
> ---
>  btrfs-image.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/btrfs-image.c b/btrfs-image.c
> index c18dff1..321c331 100644
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -2604,8 +2604,18 @@ int main(int argc, char *argv[])
>  out:
>  	if (out == stdout)
>  		fflush(out);
> -	else
> +	else {
>  		fclose(out);
> +		if (ret && create) {
> +			int unlink_ret;
> +		       
> +			unlink_ret = unlink(target);
> +			if (unlink_ret)
> +				fprintf(stderr,
> +					"unlink output file failed : %s\n",
> +					strerror(errno));
> +		}
> +	}
>  
>  	return !!ret;
>  }
Sorry, I will kick out the tailing whitespace and send a v3. Please
*ignore* this.

[PATCH v3 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[Gui Hecheng]

When btrfs-image failed to create an image, the invalid output file
had better be deleted to prevent being used mistakenly in the future.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
changelog
        v1->v2: use a new local variable to avoid return value overwritten
	v2->v3: fix patch format problem: tailing whitespace
---
 btrfs-image.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/btrfs-image.c b/btrfs-image.c
index 9e5b8b3..1a3e6ca 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2608,8 +2608,18 @@ int main(int argc, char *argv[])
 out:
 	if (out == stdout)
 		fflush(out);
-	else
+	else {
 		fclose(out);
+		if (ret && create) {
+			int unlink_ret;
+
+			unlink_ret = unlink(target);
+			if (unlink_ret)
+				fprintf(stderr,
+					"unlink output file failed : %s\n",
+					strerror(errno));
+		}
+	}
 
 	return !!ret;
 }
-- 
1.8.1.4

Re: [PATCH v3 3/4] btrfs-progs: delete invalid output file when btrfs-image failed

[David Sterba]

On Tue, Jun 24, 2014 at 11:16:45AM +0800, Gui Hecheng wrote:
> When btrfs-image failed to create an image, the invalid output file
> had better be deleted to prevent being used mistakenly in the future.
> 
> Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
> ---
> changelog
>         v1->v2: use a new local variable to avoid return value overwritten
> 	v2->v3: fix patch format problem: tailing whitespace

Thanks, both v3 patches added to integration.

[PATCH 4/4] btrfs-progs: update manpage for btrfs-image with -m option added

[Gui Hecheng]

The btrfs-image support multiple devices with -m specified.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 Documentation/btrfs-image.txt | 3 +++
 btrfs-image.c                 | 1 +
 2 files changed, 4 insertions(+)

diff --git a/Documentation/btrfs-image.txt b/Documentation/btrfs-image.txt
index 7ee820c..155194a 100644
--- a/Documentation/btrfs-image.txt
+++ b/Documentation/btrfs-image.txt
@@ -48,6 +48,9 @@ Walk all the trees manually and copy any blocks that are referenced. Use this
 option if your extent tree is corrupted to make sure that all of the metadata is
 captured.
 
+-m::
+Restore for multiple devices, more than 1 device should be provided.
+
 EXIT STATUS
 -----------
 *btrfs-image* will return 0 if no error happened.
diff --git a/btrfs-image.c b/btrfs-image.c
index b235b87..a87dcb7 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -2446,6 +2446,7 @@ static void print_usage(void)
 	fprintf(stderr, "\t-o      \tdon't mess with the chunk tree when restoring\n");
 	fprintf(stderr, "\t-s      \tsanitize file names, use once to just use garbage, use twice if you want crc collisions\n");
 	fprintf(stderr, "\t-w      \twalk all trees instead of using extent tree, do this if your extent tree is broken\n");
+	fprintf(stderr, "\t-m	   \trestore for multiple devices\n");
 	exit(1);
 }
 
-- 
1.8.1.4