[PATCH 0/4] Resend: btrfs-progs: Some cosmetic changes (mainly) to btrfsck

[PATCH 0/4] Resend: btrfs-progs: Some cosmetic changes (mainly) to btrfsck

[Dieter Ries]

Hi,

I sent an earlier version of this patchset before, and didn't get any response,
so here is the next try:

This patch series implements some mainly cosmetic changes to.
btrfs-progs, most in btrfsck.

As this is my first contribution here, I'd kindly ask you for feedback,
and if work like this is appreciated in general.

Cheers,

Dieter

Dieter Ries (4):
  btrfs-progs: Remove redundant "Btrfs" string from version string
  btrfs-progs: btrfsck: Print which filesystem to be checked to stdout
  btrfs-progs: btrfsck: Print feedback about fscking to stdout.
  btrfs-progs: btrfsck: Remove binary error code output

 btrfsck.c  |   29 ++++++++++++++++++++++-------
 version.sh |    2 +-
 2 files changed, 23 insertions(+), 8 deletions(-)

-- 
1.7.3.GIT

[PATCH 1/4] btrfs-progs: Remove redundant "Btrfs" string from version string

[Dieter Ries]

In the first line of version.sh, $v was set to "Btrfs vx.yy", and in
the end "Btrfs $v" was echoed to the version.h file. This resulted in
the version string "Btrfs Btrfs vx.yy". This patch removes the second
occurrence of "Btrfs".

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 version.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/version.sh b/version.sh
index af3e441..2c1aff1 100644
--- a/version.sh
+++ b/version.sh
@@ -46,7 +46,7 @@ fi
  
 echo "#ifndef __BUILD_VERSION" > .build-version.h
 echo "#define __BUILD_VERSION" >> .build-version.h
-echo "#define BTRFS_BUILD_VERSION \"Btrfs $v\"" >> .build-version.h
+echo "#define BTRFS_BUILD_VERSION \"$v\"" >> .build-version.h
 echo "#endif" >> .build-version.h
 
 diff -q version.h .build-version.h >& /dev/null
-- 
1.7.3.GIT

[PATCH 2/4] btrfs-progs: btrfsck: Print which filesystem to be checked to stdout

[Dieter Ries]

This patch makes btrfsck print the filesystem, which is to be checked,
to stdout. This should be helpful when analyzing (copied and pasted)
output of btrfsck.

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 btrfsck.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/btrfsck.c b/btrfsck.c
index 67f4a9d..516bcdf 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -3540,6 +3540,8 @@ int main(int ac, char **av)
 	} else if(ret) {
 		fprintf(stderr, "%s is currently mounted. Aborting.\n", av[optind]);
 		return -EBUSY;
+	} else {
+		printf("Checking filesystem on %s\n",av[optind]);
 	}
 
 	info = open_ctree_fs_info(av[optind], bytenr, rw, 1);
-- 
1.7.3.GIT

Re: [PATCH 2/4] btrfs-progs: btrfsck: Print which filesystem to be checked to stdout

[David Sterba]

On Mon, Oct 08, 2012 at 09:07:48PM +0200, Dieter Ries wrote:
> --- a/btrfsck.c
> +++ b/btrfsck.c
> @@ -3540,6 +3540,8 @@ int main(int ac, char **av)
>  	} else if(ret) {
>  		fprintf(stderr, "%s is currently mounted. Aborting.\n", av[optind]);
>  		return -EBUSY;
> +	} else {
> +		printf("Checking filesystem on %s\n",av[optind]);

This could be extended to print the UUID of the filesystem as well.
Otherwise ok.

david

[PATCH 3/4] btrfs-progs: btrfsck: Print feedback about fscking to stdout.

[Dieter Ries]

Status reports of the checking process should be printed to stdout
instead of stderr, as that is normal program output and not related to
problems in btrfsck. This patch changes this behaviour and adds the
output "Done!" after each of the parts.

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 btrfsck.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/btrfsck.c b/btrfsck.c
index ea654de..da8b4d7 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -3560,7 +3560,7 @@ int main(int ac, char **av)
 
 	root = info->fs_root;
 
-	fprintf(stderr, "checking extents\n");
+	printf("checking extents... ");
 	if (rw)
 		trans = btrfs_start_transaction(root, 1);
 
@@ -3568,22 +3568,32 @@ int main(int ac, char **av)
 		fprintf(stderr, "Reinit crc root\n");
 		ret = btrfs_fsck_reinit_root(trans, info->csum_root);
 		if (ret) {
+			printf("\n");
 			fprintf(stderr, "crc root initialization failed\n");
 			return -EIO;
 		}
 		goto out;
 	}
 	ret = check_extents(trans, root, repair);
-	if (ret)
+	if (ret) {
 		fprintf(stderr, "Errors found in extent allocation tree\n");
+		printf("\n");
+	}
+	else
+		printf("Done!\n");
 
-	fprintf(stderr, "checking fs roots\n");
+	printf("checking fs roots... ");
 	ret = check_fs_roots(root, &root_cache);
-	if (ret)
+	if (ret) {
+		printf("\n");
 		goto out;
+	}
+	else
+		printf("Done!\n");
 
-	fprintf(stderr, "checking root refs\n");
+	printf("checking root refs... ");
 	ret = check_root_refs(root, &root_cache);
+	printf("Done!\n");
 out:
 	free_root_recs(&root_cache);
 	if (rw) {
-- 
1.7.3.GIT

Re: [PATCH 3/4] btrfs-progs: btrfsck: Print feedback about fscking to stdout.

[Dieter Ries]

Sorry, that was the wrong patch... please ignore the last mail!

Cheers,
Dieter

[PATCH 2/4] btrfs-progs: btrfsck: Print which filesystem to be checked to stdout

[Dieter Ries]

This patch makes btrfsck print the filesystem, which is to be checked,
to stdout, as well as the UUID of the corresponding partition.
This should be helpful when analyzing (copied and pasted) output of
btrfsck.

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 btrfsck.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/btrfsck.c b/btrfsck.c
index 67f4a9d..ea654de 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -20,6 +20,7 @@
 #define _GNU_SOURCE 1
 #include <stdio.h>
 #include <stdlib.h>
+#include <uuid/uuid.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <sys/stat.h>
@@ -3492,6 +3493,7 @@ int main(int ac, char **av)
 	struct btrfs_fs_info *info;
 	struct btrfs_trans_handle *trans = NULL;
 	u64 bytenr = 0;
+	char uuidbuf[37];
 	int ret;
 	int num;
 	int repair = 0;
@@ -3540,9 +3542,10 @@ int main(int ac, char **av)
 	} else if(ret) {
 		fprintf(stderr, "%s is currently mounted. Aborting.\n", av[optind]);
 		return -EBUSY;
-	}
-
+	} 
 	info = open_ctree_fs_info(av[optind], bytenr, rw, 1);
+	uuid_unparse(info->super_copy.fsid, uuidbuf);
+	printf("Checking filesystem on %s\nUUID: %s\n",av[optind],uuidbuf);
 
 	if (info == NULL)
 		return 1;
-- 
1.7.3.GIT

[PATCH 3/4] btrfs-progs: btrfsck: Print feedback about fscking to stdout.

[Dieter Ries]

Status reports of the checking process should be printed to stdout
instead of stderr, as that is normal program output and not related to
problems in btrfsck. This patch changes this behaviour and adds the
output "Done!" after each of the parts.

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 btrfsck.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/btrfsck.c b/btrfsck.c
index 516bcdf..83275cd 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -3559,7 +3559,7 @@ int main(int ac, char **av)
 
 	root = info->fs_root;
 
-	fprintf(stderr, "checking extents\n");
+	printf("checking extents... ");
 	if (rw)
 		trans = btrfs_start_transaction(root, 1);
 
@@ -3567,22 +3567,32 @@ int main(int ac, char **av)
 		fprintf(stderr, "Reinit crc root\n");
 		ret = btrfs_fsck_reinit_root(trans, info->csum_root);
 		if (ret) {
+			printf("\n");
 			fprintf(stderr, "crc root initialization failed\n");
 			return -EIO;
 		}
 		goto out;
 	}
 	ret = check_extents(trans, root, repair);
-	if (ret)
+	if (ret) {
 		fprintf(stderr, "Errors found in extent allocation tree\n");
+		printf("\n");
+	}
+	else
+		printf("Done!\n");
 
-	fprintf(stderr, "checking fs roots\n");
+	printf("checking fs roots... ");
 	ret = check_fs_roots(root, &root_cache);
-	if (ret)
+	if (ret) {
+		printf("\n");
 		goto out;
+	}
+	else
+		printf("Done!\n");
 
-	fprintf(stderr, "checking root refs\n");
+	printf("checking root refs... ");
 	ret = check_root_refs(root, &root_cache);
+	printf("Done!\n");
 out:
 	free_root_recs(&root_cache);
 	if (rw) {
-- 
1.7.3.GIT

Re: [PATCH 3/4] btrfs-progs: btrfsck: Print feedback about fscking to stdout.

[David Sterba]

On Mon, Oct 08, 2012 at 09:07:49PM +0200, Dieter Ries wrote:
> Status reports of the checking process should be printed to stdout
> instead of stderr, as that is normal program output and not related to
> problems in btrfsck.

I agree that the important messages from fsck process should be printed
to stdout, and the rest like 'cannot find a valid fs on /dev' belong to
stderr so the user can simply call

  btrfsck > logfile

and does not miss any messages in the log, while will be informed that
the process cannot proceed for some urgent reason.

So far we all do 'btrfsck >& logfile' to be sure we don't miss
anythingk.

> Signed-off-by: Dieter Ries <mail@dieterries.net>
> ---
>  btrfsck.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/btrfsck.c b/btrfsck.c
> index 516bcdf..83275cd 100644
> --- a/btrfsck.c
> +++ b/btrfsck.c
> @@ -3559,7 +3559,7 @@ int main(int ac, char **av)
>  
>  	root = info->fs_root;
>  
> -	fprintf(stderr, "checking extents\n");
> +	printf("checking extents... ");
>  	if (rw)
>  		trans = btrfs_start_transaction(root, 1);
>  
> @@ -3567,22 +3567,32 @@ int main(int ac, char **av)
>  		fprintf(stderr, "Reinit crc root\n");
>  		ret = btrfs_fsck_reinit_root(trans, info->csum_root);
>  		if (ret) {
> +			printf("\n");

This looks strange, it's missing the context where it actually adds the
newline.

>  			fprintf(stderr, "crc root initialization failed\n");
^^^
this is IMHO another printf candidate, though btrfs_fsck_reinit_root
will not fail.

>  			return -EIO;
>  		}
>  		goto out;
>  	}
>  	ret = check_extents(trans, root, repair);
> -	if (ret)
> +	if (ret) {
>  		fprintf(stderr, "Errors found in extent allocation tree\n");

same here

> +		printf("\n");
> +	}
> +	else
> +		printf("Done!\n");
>  
> -	fprintf(stderr, "checking fs roots\n");
> +	printf("checking fs roots... ");

If you remove the newline, the output may be buffered and not visible
until the newline arrives

>  	ret = check_fs_roots(root, &root_cache);
> -	if (ret)
> +	if (ret) {
> +		printf("\n");
>  		goto out;
> +	}
> +	else

	} else {

> +		printf("Done!\n");

	}
>  
> -	fprintf(stderr, "checking root refs\n");
> +	printf("checking root refs... ");
>  	ret = check_root_refs(root, &root_cache);

Done, but what if ret is not zero? This goes unreported.

> +	printf("Done!\n");
>  out:
>  	free_root_recs(&root_cache);
>  	if (rw) {

I think doing the stdout/stderr split properly would need more than
fixing btrfsck.c, it uses code from other .c files, looks like an
overhaul of the logging in the whole codebase.

david

Re: [PATCH 3/4] btrfs-progs: btrfsck: Print feedback about fscking to stdout.

[mail]

Hi,

> I agree that the important messages from fsck process should be printed
> to stdout, and the rest like 'cannot find a valid fs on /dev' belong to
> stderr so the user can simply call
>
>   btrfsck > logfile
>
> and does not miss any messages in the log, while will be informed that
> the process cannot proceed for some urgent reason.

That's what I thought as well. I started small, but IMHO, a lot of the
output of btrfsck should go to stdout instead of stderr

Is there a general agreement on that for a fsck utility, it is normal
output, if the filesystem is damaged, and really only stuff which makes
the checking stop abnormally should go to stderr?

Also, right now there is a very mixed level of verbosity used. I was
thinking about adding a '-v' option, and making some of the output
conditional on that. The normal enduser will not really care about the
number of csum bytes, and as a dev you can still alias the -v.

> I think doing the stdout/stderr split properly would need more than
> fixing btrfsck.c, it uses code from other .c files, looks like an
> overhaul of the logging in the whole codebase.

I haven't looked into many other files there, but maybe you are right. I
started with btrfsck.c because I think it's a nice entry point.

> david

Cheers,

Dieter

[PATCH 4/4] btrfs-progs: btrfsck: Remove binary error code output

[Dieter Ries]

This patch changes the output after checking a filesystem. Before, the
default output was "found x bytes used err is 0", where the last integer
corresponds to the return value of check_root_refs(), which is either 1
or 0. Now this value is evaluated, and a message saying if errors were
found or not is printed.

Signed-off-by: Dieter Ries <mail@dieterries.net>
---
 btrfsck.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/btrfsck.c b/btrfsck.c
index 83275cd..45ce681 100644
--- a/btrfsck.c
+++ b/btrfsck.c
@@ -3613,8 +3613,11 @@ out:
 		       "backup data and re-format the FS. *\n\n");
 		ret = 1;
 	}
-	printf("found %llu bytes used err is %d\n",
-	       (unsigned long long)bytes_used, ret);
+	if (ret)
+		printf("Filesystem is damaged! One or more errors found.\n");
+	else
+		printf("Filesystem is clean! No errors found.\n");
+	printf("%llu bytes used\n",(unsigned long long)bytes_used);
 	printf("total csum bytes: %llu\n",(unsigned long long)total_csum_bytes);
 	printf("total tree bytes: %llu\n",
 	       (unsigned long long)total_btree_bytes);
-- 
1.7.3.GIT