| Message ID | 1349723270-12773-4-git-send-email-mail@dieterries.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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) {
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(-)