[PATCH] btrfs-progs: alias btrfs device delete to btrfs device remove

[PATCH] btrfs-progs: alias btrfs device delete to btrfs device remove

[Omar Sandoval]

There's an awkward asymmetry between btrfs device add and btrfs device
delete. Resolve this by aliasing delete to remove.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 Documentation/btrfs-device.asciidoc |  5 ++++-
 cmds-device.c                       | 33 +++++++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/btrfs-device.asciidoc b/Documentation/btrfs-device.asciidoc
index c56cf5ef48fb..2827598a37f5 100644
--- a/Documentation/btrfs-device.asciidoc
+++ b/Documentation/btrfs-device.asciidoc
@@ -74,9 +74,12 @@ do not perform discard by default
 -f|--force::::
 force overwrite of existing filesystem on the given disk(s)
 
-*delete* <dev> [<dev>...] <path>::
+*remove* <dev> [<dev>...] <path>::
 Remove device(s) from a filesystem identified by <path>.
 
+*delete* <dev> [<dev>...] <path>::
+Alias of remove kept for backwards compatability
+
 *ready* <device>::
 Check device to see if it has all of it's devices in cache for mounting.
 
diff --git a/cmds-device.c b/cmds-device.c
index 1022656988c2..0e1ea94a0e41 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -145,20 +145,17 @@ error_out:
 }
 
 static const char * const cmd_rm_dev_usage[] = {
-	"btrfs device delete <device> [<device>...] <path>",
+	"btrfs device remove <device> [<device>...] <path>",
 	"Remove a device from a filesystem",
 	NULL
 };
 
-static int cmd_rm_dev(int argc, char **argv)
+static int _cmd_rm_dev(int argc, char **argv)
 {
 	char	*mntpnt;
 	int	i, fdmnt, ret=0, e;
 	DIR	*dirstream = NULL;
 
-	if (check_argc_min(argc, 3))
-		usage(cmd_rm_dev_usage);
-
 	mntpnt = argv[argc - 1];
 
 	fdmnt = open_file_or_dir(mntpnt, &dirstream);
@@ -198,6 +195,29 @@ static int cmd_rm_dev(int argc, char **argv)
 	return !!ret;
 }
 
+static int cmd_rm_dev(int argc, char **argv)
+{
+	if (check_argc_min(argc, 3))
+		usage(cmd_rm_dev_usage);
+
+	return _cmd_rm_dev(argc, argv);
+}
+
+
+static const char * const cmd_del_dev_usage[] = {
+	"btrfs device delete <device> [<device>...] <path>",
+	"Remove a device from a filesystem (alias of remove)",
+	NULL
+};
+
+static int cmd_del_dev(int argc, char **argv)
+{
+	if (check_argc_min(argc, 3))
+		usage(cmd_del_dev_usage);
+
+	return _cmd_rm_dev(argc, argv);
+}
+
 static const char * const cmd_scan_dev_usage[] = {
 	"btrfs device scan [(-d|--all-devices)|<device> [<device>...]]",
 	"Scan devices for a btrfs filesystem",
@@ -586,12 +606,13 @@ out:
 const struct cmd_group device_cmd_group = {
 	device_cmd_group_usage, NULL, {
 		{ "add", cmd_add_dev, cmd_add_dev_usage, NULL, 0 },
-		{ "delete", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
+		{ "remove", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
 		{ "scan", cmd_scan_dev, cmd_scan_dev_usage, NULL, 0 },
 		{ "ready", cmd_ready_dev, cmd_ready_dev_usage, NULL, 0 },
 		{ "stats", cmd_dev_stats, cmd_dev_stats_usage, NULL, 0 },
 		{ "usage", cmd_device_usage,
 			cmd_device_usage_usage, NULL, 0 },
+		{ "delete", cmd_del_dev, cmd_del_dev_usage, NULL, 0 },
 		NULL_CMD_STRUCT
 	}
 };
-- 
2.4.3

Re: [PATCH] btrfs-progs: alias btrfs device delete to btrfs device remove

[David Sterba]

On Thu, Jun 18, 2015 at 03:07:09PM -0700, Omar Sandoval wrote:
> @@ -586,12 +606,13 @@ out:
>  const struct cmd_group device_cmd_group = {
>  	device_cmd_group_usage, NULL, {
>  		{ "add", cmd_add_dev, cmd_add_dev_usage, NULL, 0 },
> -		{ "delete", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
> +		{ "remove", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
>  		{ "scan", cmd_scan_dev, cmd_scan_dev_usage, NULL, 0 },
>  		{ "ready", cmd_ready_dev, cmd_ready_dev_usage, NULL, 0 },
>  		{ "stats", cmd_dev_stats, cmd_dev_stats_usage, NULL, 0 },
>  		{ "usage", cmd_device_usage,
>  			cmd_device_usage_usage, NULL, 0 },
> +		{ "delete", cmd_del_dev, cmd_del_dev_usage, NULL, 0 },

No need to introduce the wrappers, it's enough to add an alternative
usage string and the callback function will be the same. Also please
keep the aliased entries next to each other.

Suggestion for separate change: redefine the last argument (currently to
denote hidden commands) to be a flag set and add a new one for aliases.
That way we can automatically generate a compact help. Currently, the
device section looks like this:

    btrfs device add [options] <device> [<device>...] <path>
        Add a device to a filesystem
    btrfs device remove <device> [<device>...] <path>
        Remove a device from a filesystem
    btrfs device scan [(-d|--all-devices)|<device> [<device>...]]
        Scan devices for a btrfs filesystem
    btrfs device ready <device>
        Check device to see if it has all of its devices in cache for mounting
    btrfs device stats [-z] <path>|<device>
        Show current device IO stats. -z to reset stats afterwards.
    btrfs device usage [options] <path> [<path>..]
        Show detailed information about internal allocations in devices.
    btrfs device delete <device> [<device>...] <path>
        Remove a device from a filesystem (alias of remove)

For the first version it could be simply the first line of the usage string:

    btrfs device add [options] <device> [<device>...] <path>
        Add a device to a filesystem
    btrfs device delete <device> [<device>...] <path>
    btrfs device remove <device> [<device>...] <path>
        Remove a device from a filesystem
    btrfs device scan [(-d|--all-devices)|<device> [<device>...]]
        Scan devices for a btrfs filesystem
    btrfs device ready <device>
        Check device to see if it has all of its devices in cache for mounting
    btrfs device stats [-z] <path>|<device>
        Show current device IO stats. -z to reset stats afterwards.
    btrfs device usage [options] <path> [<path>..]
        Show detailed information about internal allocations in devices.
        Remove a device from a filesystem (alias of remove)

Re: [PATCH] btrfs-progs: alias btrfs device delete to btrfs device remove

[Omar Sandoval]

On Tue, Jun 23, 2015 at 05:40:39PM +0200, David Sterba wrote:
> On Thu, Jun 18, 2015 at 03:07:09PM -0700, Omar Sandoval wrote:
> > @@ -586,12 +606,13 @@ out:
> >  const struct cmd_group device_cmd_group = {
> >  	device_cmd_group_usage, NULL, {
> >  		{ "add", cmd_add_dev, cmd_add_dev_usage, NULL, 0 },
> > -		{ "delete", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
> > +		{ "remove", cmd_rm_dev, cmd_rm_dev_usage, NULL, 0 },
> >  		{ "scan", cmd_scan_dev, cmd_scan_dev_usage, NULL, 0 },
> >  		{ "ready", cmd_ready_dev, cmd_ready_dev_usage, NULL, 0 },
> >  		{ "stats", cmd_dev_stats, cmd_dev_stats_usage, NULL, 0 },
> >  		{ "usage", cmd_device_usage,
> >  			cmd_device_usage_usage, NULL, 0 },
> > +		{ "delete", cmd_del_dev, cmd_del_dev_usage, NULL, 0 },
> 
> No need to introduce the wrappers, it's enough to add an alternative
> usage string and the callback function will be the same. Also please
> keep the aliased entries next to each other.

So the reason I did that way is that this:

static int cmd_rm_dev(int argc, char **argv)
{
	char	*mntpnt;
	int	i, fdmnt, ret=0, e;
	DIR	*dirstream = NULL;

	if (check_argc_min(argc, 3))
		usage(cmd_rm_dev_usage);

would use the same usage string for both commands. E.g., if you do
"btrfs device delete", the usage string would say
"btrfs device remove...". That's a small cosmetic issue, but what do you
think?

> Suggestion for separate change: redefine the last argument (currently to
> denote hidden commands) to be a flag set and add a new one for aliases.
> That way we can automatically generate a compact help. Currently, the
> device section looks like this:
> 
>     btrfs device add [options] <device> [<device>...] <path>
>         Add a device to a filesystem
>     btrfs device remove <device> [<device>...] <path>
>         Remove a device from a filesystem
>     btrfs device scan [(-d|--all-devices)|<device> [<device>...]]
>         Scan devices for a btrfs filesystem
>     btrfs device ready <device>
>         Check device to see if it has all of its devices in cache for mounting
>     btrfs device stats [-z] <path>|<device>
>         Show current device IO stats. -z to reset stats afterwards.
>     btrfs device usage [options] <path> [<path>..]
>         Show detailed information about internal allocations in devices.
>     btrfs device delete <device> [<device>...] <path>
>         Remove a device from a filesystem (alias of remove)
> 
> For the first version it could be simply the first line of the usage string:
> 
>     btrfs device add [options] <device> [<device>...] <path>
>         Add a device to a filesystem
>     btrfs device delete <device> [<device>...] <path>
>     btrfs device remove <device> [<device>...] <path>
>         Remove a device from a filesystem
>     btrfs device scan [(-d|--all-devices)|<device> [<device>...]]
>         Scan devices for a btrfs filesystem
>     btrfs device ready <device>
>         Check device to see if it has all of its devices in cache for mounting
>     btrfs device stats [-z] <path>|<device>
>         Show current device IO stats. -z to reset stats afterwards.
>     btrfs device usage [options] <path> [<path>..]
>         Show detailed information about internal allocations in devices.
>         Remove a device from a filesystem (alias of remove)

Cool, that sounds like a good idea.

Thanks!
-- 
Omar

Re: [PATCH] btrfs-progs: alias btrfs device delete to btrfs device remove

[David Sterba]

On Tue, Jun 23, 2015 at 02:30:06PM -0700, Omar Sandoval wrote:
> > No need to introduce the wrappers, it's enough to add an alternative
> > usage string and the callback function will be the same. Also please
> > keep the aliased entries next to each other.
> 
> So the reason I did that way is that this:
> 
> static int cmd_rm_dev(int argc, char **argv)
> {
> 	char	*mntpnt;
> 	int	i, fdmnt, ret=0, e;
> 	DIR	*dirstream = NULL;
> 
> 	if (check_argc_min(argc, 3))
> 		usage(cmd_rm_dev_usage);
> 
> would use the same usage string for both commands. E.g., if you do
> "btrfs device delete", the usage string would say
> "btrfs device remove...". That's a small cosmetic issue, but what do you
> think?

Ah right, I forgot about the separate usage string. We'll have to pass
it to the callback somehow in general, as it is printed eg. in the
'default:' branch of the geopt switch loop. The alias wrappers are
probably inevitable:

{ "delete", cmd_device_delete, cmd_device_delete_usage, NULL, 0 },
{ "remove", cmd_device_remove, cmd_device_remove_usage, NULL, 0 },

int cmd_device_delete(int argc, char **argv) {
	return _cmd_device_delete(argc, argv, cmd_device_delete_usage);
}

int cmd_device_remove(int argc, char **argv) {
	return _cmd_device_delete(argc, argv, cmd_device_remove_usage);
}

This follows a pattern so we can add a macro wrapper eventually if this
proves to be the best option.