[RFC] Move btrfsck in to the btrfs command

[RFC] Move btrfsck in to the btrfs command

[Ian Kumlien]

NOTE: in order to apply this patch you should:
	git mv btrfsck.c cmd-fsck.c

This patch moves btrfsck in to "btrfs fsck".

It also adds support for symlinks to the btrfs binary to
retain compablity, =)

I think something should be done to the help description but i'm not
sure what... Anyway, feedback is welcome.

[PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Ian Kumlien]

This patch includes fsck as a subcommand of btrfs, but if you rename
the binary to btrfsck (or, preferably, use a symlink) it will act like
the old btrfs command.

It will also handle fsck.btrfs which currently is a noop.
---
 Makefile    |  4 ++--
 btrfs.c     | 68 +++++++++++++++++++++++++++++++++++++++++++++----------------
 cmds-fsck.c | 38 +++++++++++++++++++---------------
 commands.h  |  3 +++
 4 files changed, 77 insertions(+), 36 deletions(-)

diff --git a/Makefile b/Makefile
index 4894903..8467530 100644
--- a/Makefile
+++ b/Makefile
@@ -8,7 +8,7 @@ objects = ctree.o disk-io.o radix-tree.o extent-tree.o print-tree.o \
 	  send-stream.o send-utils.o qgroup.o
 cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \
 	       cmds-inspect.o cmds-balance.o cmds-send.o cmds-receive.o \
-	       cmds-quota.o cmds-qgroup.o
+	       cmds-quota.o cmds-qgroup.o cmds-fsck.o
 
 CHECKFLAGS= -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ -Wbitwise \
 	    -Wuninitialized -Wshadow -Wundef
@@ -20,7 +20,7 @@ bindir = $(prefix)/bin
 LIBS=-luuid -lm
 RESTORE_LIBS=-lz
 
-progs = btrfsctl mkfs.btrfs btrfs-debug-tree btrfs-show btrfs-vol btrfsck \
+progs = btrfsctl mkfs.btrfs btrfs-debug-tree btrfs-show btrfs-vol \
 	btrfs btrfs-map-logical btrfs-image btrfs-zero-log btrfs-convert \
 	btrfs-find-root btrfs-restore btrfstune
 
diff --git a/btrfs.c b/btrfs.c
index 687acec..5c1220e 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -48,8 +48,13 @@ int prefixcmp(const char *str, const char *prefix)
 			return (unsigned char)*prefix - (unsigned char)*str;
 }
 
-static int parse_one_token(const char *arg, const struct cmd_group *grp,
-			   const struct cmd_struct **cmd_ret)
+#define parse_one_token(arg, grp, cmd_ret) \
+	_parse_one_token((arg), (grp), (cmd_ret), 0)
+#define parse_one_exact_token(arg, grp, cmd_ret) \
+	_parse_one_token((arg), (grp), (cmd_ret), 1)
+
+static int _parse_one_token(const char *arg, const struct cmd_group *grp,
+			   const struct cmd_struct **cmd_ret, int exact)
 {
 	const struct cmd_struct *cmd = grp->commands;
 	const struct cmd_struct *abbrev_cmd = NULL, *ambiguous_cmd = NULL;
@@ -80,12 +85,15 @@ static int parse_one_token(const char *arg, const struct cmd_group *grp,
 		return 0;
 	}
 
-	if (ambiguous_cmd)
-		return -2;
+	if (!exact)
+	{
+		if (ambiguous_cmd)
+			return -2;
 
-	if (abbrev_cmd) {
-		*cmd_ret = abbrev_cmd;
-		return 0;
+		if (abbrev_cmd) {
+			*cmd_ret = abbrev_cmd;
+			return 0;
+		}
 	}
 
 	return -1;
@@ -246,6 +254,7 @@ const struct cmd_group btrfs_cmd_group = {
 		{ "balance", cmd_balance, NULL, &balance_cmd_group, 0 },
 		{ "device", cmd_device, NULL, &device_cmd_group, 0 },
 		{ "scrub", cmd_scrub, NULL, &scrub_cmd_group, 0 },
+		{ "fsck", cmd_fsck, cmd_fsck_usage, NULL, 0 },
 		{ "inspect-internal", cmd_inspect, NULL, &inspect_cmd_group, 0 },
 		{ "send", cmd_send, NULL, &send_cmd_group, 0 },
 		{ "receive", cmd_receive, NULL, &receive_cmd_group, 0 },
@@ -257,24 +266,47 @@ const struct cmd_group btrfs_cmd_group = {
 	},
 };
 
+static int cmd_dummy(int argc, char **argv)
+{
+	return 0;
+}
+
+/* change behaviour depending on what we're called */
+const struct cmd_group function_cmd_group = {
+	NULL, NULL,
+	{
+		{ "btrfsck", cmd_fsck, NULL, NULL, 0 },
+		{ "fsck.btrfs", cmd_dummy, NULL, NULL, 0 },
+		{ 0, 0, 0, 0, 0 }
+	},
+};
+
 int main(int argc, char **argv)
 {
 	const struct cmd_struct *cmd;
+	char *func = strrchr(argv[0], '/');
+	if (func)
+		argv[0] = ++func;
 
 	crc32c_optimization_init();
 
-	argc--;
-	argv++;
-	handle_options(&argc, &argv);
-	if (argc > 0) {
-		if (!prefixcmp(argv[0], "--"))
-			argv[0] += 2;
-	} else {
-		usage_command_group(&btrfs_cmd_group, 0, 0);
-		exit(1);
-	}
+	/* if we have cmd, we're started as a sub command */
+	if (parse_one_exact_token(argv[0], &function_cmd_group, &cmd) < 0)
+	{
+		argc--;
+		argv++;
 
-	cmd = parse_command_token(argv[0], &btrfs_cmd_group);
+		handle_options(&argc, &argv);
+		if (argc > 0) {
+			if (!prefixcmp(argv[0], "--"))
+				argv[0] += 2;
+		} else {
+			usage_command_group(&btrfs_cmd_group, 0, 0);
+			exit(1);
+		}
+
+		cmd = parse_command_token(argv[0], &btrfs_cmd_group);
+	}
 
 	handle_help_options_next_level(cmd, argc, argv);
 
diff --git a/cmds-fsck.c b/cmds-fsck.c
index 67f4a9d..bb5d81f 100644
--- a/cmds-fsck.c
+++ b/cmds-fsck.c
@@ -34,6 +34,7 @@
 #include "list.h"
 #include "version.h"
 #include "utils.h"
+#include "commands.h"
 
 static u64 bytes_used = 0;
 static u64 total_csum_bytes = 0;
@@ -3470,13 +3471,6 @@ static int check_extents(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static void print_usage(void)
-{
-	fprintf(stderr, "usage: btrfsck dev\n");
-	fprintf(stderr, "%s\n", BTRFS_BUILD_VERSION);
-	exit(1);
-}
-
 static struct option long_options[] = {
 	{ "super", 1, NULL, 's' },
 	{ "repair", 0, NULL, 0 },
@@ -3485,7 +3479,18 @@ static struct option long_options[] = {
 	{ 0, 0, 0, 0}
 };
 
-int main(int ac, char **av)
+const char * const cmd_fsck_usage[] = {
+	"btrfs fsck [-s <superblock>] [--repair] [--init-csum-tree] [--init-extent-tree] <device>",
+	"check a btrfs filesystem",
+	"",
+	"-s|--super <superblock>     use this superblock copy",
+	"--repair                    try to repair the filesystem",
+	"--init-csum-tree            create a new CRC tree",
+	"--init-extent-tree          create a new extent tree",
+	NULL
+};
+
+int cmd_fsck(int argc, char **argv)
 {
 	struct cache_tree root_cache;
 	struct btrfs_root *root;
@@ -3501,7 +3506,7 @@ int main(int ac, char **av)
 
 	while(1) {
 		int c;
-		c = getopt_long(ac, av, "s:", long_options,
+		c = getopt_long(argc, argv, "s:", long_options,
 				&option_index);
 		if (c < 0)
 			break;
@@ -3513,7 +3518,8 @@ int main(int ac, char **av)
 				       (unsigned long long)bytenr);
 				break;
 			case '?':
-				print_usage();
+			case 'h':
+				usage(cmd_fsck_usage);
 		}
 		if (option_index == 1) {
 			printf("enabling repair mode\n");
@@ -3526,23 +3532,23 @@ int main(int ac, char **av)
 		}
 
 	}
-	ac = ac - optind;
+	argc = argc - optind;
 
-	if (ac != 1)
-		print_usage();
+	if (argc != 1)
+		usage(cmd_fsck_usage);
 
 	radix_tree_init();
 	cache_tree_init(&root_cache);
 
-	if((ret = check_mounted(av[optind])) < 0) {
+	if((ret = check_mounted(argv[optind])) < 0) {
 		fprintf(stderr, "Could not check mount status: %s\n", strerror(-ret));
 		return ret;
 	} else if(ret) {
-		fprintf(stderr, "%s is currently mounted. Aborting.\n", av[optind]);
+		fprintf(stderr, "%s is currently mounted. Aborting.\n", argv[optind]);
 		return -EBUSY;
 	}
 
-	info = open_ctree_fs_info(av[optind], bytenr, rw, 1);
+	info = open_ctree_fs_info(argv[optind], bytenr, rw, 1);
 
 	if (info == NULL)
 		return 1;
diff --git a/commands.h b/commands.h
index bb6d2dd..308eb61 100644
--- a/commands.h
+++ b/commands.h
@@ -93,11 +93,14 @@ extern const struct cmd_group receive_cmd_group;
 extern const struct cmd_group quota_cmd_group;
 extern const struct cmd_group qgroup_cmd_group;
 
+extern const char * const cmd_fsck_usage[];
+
 int cmd_subvolume(int argc, char **argv);
 int cmd_filesystem(int argc, char **argv);
 int cmd_balance(int argc, char **argv);
 int cmd_device(int argc, char **argv);
 int cmd_scrub(int argc, char **argv);
+int cmd_fsck(int argc, char **argv);
 int cmd_inspect(int argc, char **argv);
 int cmd_send(int argc, char **argv);
 int cmd_receive(int argc, char **argv);
-- 
1.8.1.2

Re: [PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Filipe Brandenburger]

Hi Ian,

On Tue, Jan 29, 2013 at 3:03 PM, Ian Kumlien <pomac@demius.net> wrote:
> This patch includes fsck as a subcommand of btrfs, but if you rename
> the binary to btrfsck (or, preferably, use a symlink) it will act like
> the old btrfs command.

You can rename files in your git (there's "git mv" for that), only
thing is when you generate the patch with format-patch (or "git show",
"git diff" etc.) pass it the -M option to detect moves and act
appropriately.

Regarding your patches, I really like the idea of "btrfs fsck" but I
think I'd prefer to keep the external commands as wrapper scripts
instead of adding busybox-style name detection to btrfs... But then,
that's just my opinion.

I guess I would have a "btrfsck" that would simply contain:

    #! /bin/sh
    exec btrfs fsck "$@"

Downside is that error reporting (e.g. invalid syntax, etc.) would
show "btrfs fsck" instead of the command the user actually typed...

Cheers,
Filipe

Re: [PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Ian Kumlien]

On Wed, Jan 30, 2013 at 12:33:42PM -0800, Filipe Brandenburger wrote:
> Hi Ian,
> 
> On Tue, Jan 29, 2013 at 3:03 PM, Ian Kumlien <pomac@demius.net> wrote:
> > This patch includes fsck as a subcommand of btrfs, but if you rename
> > the binary to btrfsck (or, preferably, use a symlink) it will act like
> > the old btrfs command.
> 
> You can rename files in your git (there's "git mv" for that), only
> thing is when you generate the patch with format-patch (or "git show",
> "git diff" etc.) pass it the -M option to detect moves and act
> appropriately.

git send-email seems to send the full diff, diffing against /dev/null =P
This is why i skipped that part.

> Regarding your patches, I really like the idea of "btrfs fsck" but I
> think I'd prefer to keep the external commands as wrapper scripts
> instead of adding busybox-style name detection to btrfs... But then,
> that's just my opinion.

Well, now both works.

> I guess I would have a "btrfsck" that would simply contain:
> 
>     #! /bin/sh
>     exec btrfs fsck "$@"
> 
> Downside is that error reporting (e.g. invalid syntax, etc.) would
> show "btrfs fsck" instead of the command the user actually typed...

Actually it still does, due to how btrfs handles things... It's a simple
enough fix and it will make rescue cd's or dracut images, or just about
anything.

I understand your point, but i think this is a simpler solution =)

> Cheers,
> Filipe

Re: [PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Ilya Dryomov]

On Wed, Jan 30, 2013 at 10:11:44PM +0100, Ian Kumlien wrote:
> On Wed, Jan 30, 2013 at 12:33:42PM -0800, Filipe Brandenburger wrote:
> > Hi Ian,
> > 
> > On Tue, Jan 29, 2013 at 3:03 PM, Ian Kumlien <pomac@demius.net> wrote:
> > > This patch includes fsck as a subcommand of btrfs, but if you rename
> > > the binary to btrfsck (or, preferably, use a symlink) it will act like
> > > the old btrfs command.
> > 
> > You can rename files in your git (there's "git mv" for that), only
> > thing is when you generate the patch with format-patch (or "git show",
> > "git diff" etc.) pass it the -M option to detect moves and act
> > appropriately.
> 
> git send-email seems to send the full diff, diffing against /dev/null =P
> This is why i skipped that part.
> 
> > Regarding your patches, I really like the idea of "btrfs fsck" but I
> > think I'd prefer to keep the external commands as wrapper scripts
> > instead of adding busybox-style name detection to btrfs... But then,
> > that's just my opinion.
> 
> Well, now both works.
> 
> > I guess I would have a "btrfsck" that would simply contain:
> > 
> >     #! /bin/sh
> >     exec btrfs fsck "$@"
> > 
> > Downside is that error reporting (e.g. invalid syntax, etc.) would
> > show "btrfs fsck" instead of the command the user actually typed...
> 
> Actually it still does, due to how btrfs handles things... It's a simple
> enough fix and it will make rescue cd's or dracut images, or just about
> anything.
> 
> I understand your point, but i think this is a simpler solution =)

FWIW I agree with Filipe, this name detection thing looks ugly to me.
The merge itself is a good idea, but I think we should stick with shell
wrappers for everything else.

Thanks,

		Ilya

Re: [PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Ian Kumlien]

On Wed, Jan 30, 2013 at 11:59:05PM +0200, Ilya Dryomov wrote:
> On Wed, Jan 30, 2013 at 10:11:44PM +0100, Ian Kumlien wrote:
> > On Wed, Jan 30, 2013 at 12:33:42PM -0800, Filipe Brandenburger wrote:
> > > Hi Ian,
> > > 
> > > On Tue, Jan 29, 2013 at 3:03 PM, Ian Kumlien <pomac@demius.net> wrote:
> > > > This patch includes fsck as a subcommand of btrfs, but if you rename
> > > > the binary to btrfsck (or, preferably, use a symlink) it will act like
> > > > the old btrfs command.
> > > 
> > > You can rename files in your git (there's "git mv" for that), only
> > > thing is when you generate the patch with format-patch (or "git show",
> > > "git diff" etc.) pass it the -M option to detect moves and act
> > > appropriately.
> > 
> > git send-email seems to send the full diff, diffing against /dev/null =P
> > This is why i skipped that part.
> > 
> > > Regarding your patches, I really like the idea of "btrfs fsck" but I
> > > think I'd prefer to keep the external commands as wrapper scripts
> > > instead of adding busybox-style name detection to btrfs... But then,
> > > that's just my opinion.
> > 
> > Well, now both works.
> > 
> > > I guess I would have a "btrfsck" that would simply contain:
> > > 
> > >     #! /bin/sh
> > >     exec btrfs fsck "$@"
> > > 
> > > Downside is that error reporting (e.g. invalid syntax, etc.) would
> > > show "btrfs fsck" instead of the command the user actually typed...
> > 
> > Actually it still does, due to how btrfs handles things... It's a simple
> > enough fix and it will make rescue cd's or dracut images, or just about
> > anything.
> > 
> > I understand your point, but i think this is a simpler solution =)
> 
> FWIW I agree with Filipe, this name detection thing looks ugly to me.
> The merge itself is a good idea, but I think we should stick with shell
> wrappers for everything else.

Which part of it?

        char *func = strrchr(argv[0], '/');
        if (func)
                argv[0] = ++func;

Would you prefer i rewrote it as:
	...
	char *func = strrchr(argv[0], '/');
	if (func)
		++func;
	else
		func = argv[0]
	...
	if (parse_one_exact_token(func, &function_cmd_group, &cmd) < 0)
---

Would that be better?

The only thing it actually does is remove any path if present....

I have now added:
    btrfs rescue restore [options] <device>
        Restore filesystem
    btrfs rescue select-super -s <number> <device>
        Select a superblock
    btrfs rescue dump-super <device>
        Dump a superblock to disk
    btrfs rescue debug-tree [options] <device>
        Debug the filesystem

And i'm waiting to rebase my patch series since i need to rewrite the
commit messages if this is wanted changes.

> Thanks,
> 
> 		Ilya

Re: [RFC] Move btrfsck in to the btrfs command

[Roman Mamedov]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Wed, 30 Jan 2013 00:03:52 +0100
Ian Kumlien <pomac@demius.net> wrote:

> This patch moves btrfsck in to "btrfs fsck".

Does the "...fs fs.." combination look less than ideally beautiful to anyone
else? That's "Filesystem" abbreviated two times right there.
Who can use an ATM Machine? Someone who knows a PIN Number.

Also it's a complete clstrfck of nine consonants, with no vowels.

How about "btrfs check"?

-- 
With respect,
Roman

~~~~~~~~~~~~~~~~~~~~~~~~~~~
"Stallman had a printer,
with code he could not see.
So he began to tinker,
and set the software free."

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] Move btrfsck in to the btrfs command

[Chris Murphy]

On Jan 30, 2013, at 9:10 PM, Roman Mamedov <rm@romanrm.ru> wrote:
> 
> How about "btrfs check"?

For that matter, separate out check and repair. Is there a potential for a btrfsck repair to make things worse? And if so, could this be determined with a check?


Chris Murphy

Re: [PATCH] [RFC] include btrfsck in btrfs - including "name check"

[Chris Mason]

On Wed, Jan 30, 2013 at 02:59:05PM -0700, Ilya Dryomov wrote:
> On Wed, Jan 30, 2013 at 10:11:44PM +0100, Ian Kumlien wrote:
> > On Wed, Jan 30, 2013 at 12:33:42PM -0800, Filipe Brandenburger wrote:
> > > Hi Ian,
> > > 
> > > On Tue, Jan 29, 2013 at 3:03 PM, Ian Kumlien <pomac@demius.net> wrote:
> > > > This patch includes fsck as a subcommand of btrfs, but if you rename
> > > > the binary to btrfsck (or, preferably, use a symlink) it will act like
> > > > the old btrfs command.
> > > 
> > > You can rename files in your git (there's "git mv" for that), only
> > > thing is when you generate the patch with format-patch (or "git show",
> > > "git diff" etc.) pass it the -M option to detect moves and act
> > > appropriately.
> > 
> > git send-email seems to send the full diff, diffing against /dev/null =P
> > This is why i skipped that part.
> > 
> > > Regarding your patches, I really like the idea of "btrfs fsck" but I
> > > think I'd prefer to keep the external commands as wrapper scripts
> > > instead of adding busybox-style name detection to btrfs... But then,
> > > that's just my opinion.
> > 
> > Well, now both works.
> > 
> > > I guess I would have a "btrfsck" that would simply contain:
> > > 
> > >     #! /bin/sh
> > >     exec btrfs fsck "$@"
> > > 
> > > Downside is that error reporting (e.g. invalid syntax, etc.) would
> > > show "btrfs fsck" instead of the command the user actually typed...
> > 
> > Actually it still does, due to how btrfs handles things... It's a simple
> > enough fix and it will make rescue cd's or dracut images, or just about
> > anything.
> > 
> > I understand your point, but i think this is a simpler solution =)
> 
> FWIW I agree with Filipe, this name detection thing looks ugly to me.
> The merge itself is a good idea, but I think we should stick with shell
> wrappers for everything else.

I like the shell scripts for the things we expect to eventually go away.
But we're probably stuck with btrfsck and fsck.btrfs forever, so I'd
prefer these get the name detection treatment.

At the end of the day, I'll defer to our distro friends on what it
easiest for them to package and include.

-chris