thr3ads.net - Btrfs devel - [PATCH 0/4] Resend: btrfs-progs: Some cosmetic changes (mainly) to btrfsck [Oct 2012]

If this information is useful, please help other people find it:
Share via:

Dieter Ries

2012-Oct-08 19:07 UTC

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

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

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

Dieter Ries

2012-Oct-08 19:07 UTC

head link

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

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

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

Dieter Ries

2012-Oct-08 19:07 UTC

head link

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

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

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

Dieter Ries

2012-Oct-08 19:07 UTC

head link

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

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

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

Dieter Ries

2012-Oct-08 19:07 UTC

head link

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

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

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

David Sterba

2012-Oct-09 15:08 UTC

head link

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

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

David Sterba

2012-Oct-09 15:21 UTC

head link

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

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

mail@dieterries.net

2012-Oct-11 20:50 UTC

head link

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

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

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

Dieter Ries

2012-Oct-14 15:15 UTC

head link

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

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

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

Dieter Ries

2012-Oct-14 15:17 UTC

head link

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

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

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

Dieter Ries

2012-Oct-14 15:18 UTC

head link

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

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

Cheers,
Dieter
--
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

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

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

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

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

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

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

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

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

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

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

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

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