[PATCH 0/2] btrfs device remove alias

[PATCH 0/2] btrfs device remove alias

[Omar Sandoval]

The opposite of btrfs device add is btrfs device delete. This really
should be btrfs device remove.

Changes from v1:
- Add support for flags to cmd_struct and a CMD_ALIAS flag which only
  prints the one-line usage string
- Rearrange the command wrappers in a way that could be made generic if
  needed

Thanks!

Omar Sandoval (2):
  btrfs-progs: replace struct cmd_group->hidden with flags
  btrfs-progs: alias btrfs device delete to btrfs device remove

 Documentation/btrfs-device.asciidoc |  5 ++++-
 cmds-device.c                       | 35 ++++++++++++++++++++++++++---------
 cmds-filesystem.c                   |  2 +-
 commands.h                          |  9 +++++++--
 help.c                              | 12 +++++++-----
 5 files changed, 45 insertions(+), 18 deletions(-)

-- 
2.4.4

[PATCH 1/2] btrfs-progs: replace struct cmd_group->hidden with flags

[Omar Sandoval]

We're also going to want to support aliases, so rather than adding
another member, replace "hidden" with a "flags" member.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 cmds-filesystem.c | 2 +-
 commands.h        | 8 ++++++--
 help.c            | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index b93bb33028f2..284dc87bc464 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -1327,7 +1327,7 @@ const struct cmd_group filesystem_cmd_group = {
 		{ "show", cmd_show, cmd_show_usage, NULL, 0 },
 		{ "sync", cmd_sync, cmd_sync_usage, NULL, 0 },
 		{ "defragment", cmd_defrag, cmd_defrag_usage, NULL, 0 },
-		{ "balance", cmd_balance, NULL, &balance_cmd_group, 1 },
+		{ "balance", cmd_balance, NULL, &balance_cmd_group, CMD_HIDDEN },
 		{ "resize", cmd_resize, cmd_resize_usage, NULL, 0 },
 		{ "label", cmd_label, cmd_label_usage, NULL, 0 },
 		{ "usage", cmd_filesystem_usage,
diff --git a/commands.h b/commands.h
index bd23340d45ae..42d31781f1a3 100644
--- a/commands.h
+++ b/commands.h
@@ -17,6 +17,10 @@
 #ifndef __BTRFS_COMMANDS_H__
 #define __BTRFS_COMMANDS_H__
 
+enum {
+	CMD_HIDDEN = (1 << 0),	/* should not be in help listings */
+};
+
 struct cmd_struct {
 	const char *token;
 	int (*fn)(int, char **);
@@ -47,8 +51,8 @@ struct cmd_struct {
 	/* should be NULL if token is not a subgroup */
 	const struct cmd_group *next;
 
-	/* if true don't list this token in help listings */
-	int hidden;
+	/* CMD_* flags above */
+	int flags;
 };
 
 #define NULL_CMD_STRUCT {NULL, NULL, NULL, NULL, 0}
diff --git a/help.c b/help.c
index 56aaf9c3c64c..34754c16e6fe 100644
--- a/help.c
+++ b/help.c
@@ -133,7 +133,7 @@ static void usage_command_group_internal(const struct cmd_group *grp, int full,
 	int do_sep = 0;
 
 	for (; cmd->token; cmd++) {
-		if (cmd->hidden)
+		if (cmd->flags & CMD_HIDDEN)
 			continue;
 
 		if (full && cmd != grp->commands)
-- 
2.4.4

[PATCH 2/2] 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                       | 35 ++++++++++++++++++++++++++---------
 commands.h                          |  1 +
 help.c                              | 10 ++++++----
 4 files changed, 37 insertions(+), 14 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..6972156fdf70 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -144,20 +144,14 @@ error_out:
 	return !!ret;
 }
 
-static const char * const cmd_rm_dev_usage[] = {
-	"btrfs device delete <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, const char * const *usagestr)
 {
 	char	*mntpnt;
 	int	i, fdmnt, ret=0, e;
 	DIR	*dirstream = NULL;
 
 	if (check_argc_min(argc, 3))
-		usage(cmd_rm_dev_usage);
+		usage(usagestr);
 
 	mntpnt = argv[argc - 1];
 
@@ -198,6 +192,28 @@ static int cmd_rm_dev(int argc, char **argv)
 	return !!ret;
 }
 
+static const char * const cmd_rm_dev_usage[] = {
+	"btrfs device remove <device> [<device>...] <path>",
+	"Remove a device from a filesystem",
+	NULL
+};
+
+static int cmd_rm_dev(int argc, char **argv)
+{
+	return _cmd_rm_dev(argc, argv, cmd_rm_dev_usage);
+}
+
+static const char * const cmd_del_dev_usage[] = {
+	"btrfs device delete <device> [<device>...] <path>",
+	"Remove a device from a filesystem",
+	NULL
+};
+
+static int cmd_del_dev(int argc, char **argv)
+{
+	return _cmd_rm_dev(argc, argv, cmd_del_dev_usage);
+}
+
 static const char * const cmd_scan_dev_usage[] = {
 	"btrfs device scan [(-d|--all-devices)|<device> [<device>...]]",
 	"Scan devices for a btrfs filesystem",
@@ -586,7 +602,8 @@ 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 },
+		{ "delete", cmd_del_dev, cmd_del_dev_usage, NULL, CMD_ALIAS },
+		{ "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 },
diff --git a/commands.h b/commands.h
index 42d31781f1a3..90bbd05a3542 100644
--- a/commands.h
+++ b/commands.h
@@ -19,6 +19,7 @@
 
 enum {
 	CMD_HIDDEN = (1 << 0),	/* should not be in help listings */
+	CMD_ALIAS = (1 << 1),	/* alias of next command in cmd_group */
 };
 
 struct cmd_struct {
diff --git a/help.c b/help.c
index 34754c16e6fe..6ecf01d57b83 100644
--- a/help.c
+++ b/help.c
@@ -81,11 +81,13 @@ static int do_usage_one_command(const char * const *usagestr,
 
 static int usage_command_internal(const char * const *usagestr,
 				  const char *token, int full, int lst,
-				  FILE *outf)
+				  int alias, FILE *outf)
 {
-	unsigned int flags = USAGE_SHORT;
+	unsigned int flags = 0;
 	int ret;
 
+	if (!alias)
+		flags |= USAGE_SHORT;
 	if (full)
 		flags |= USAGE_LONG | USAGE_OPTIONS;
 	if (lst)
@@ -110,7 +112,7 @@ static void usage_command_usagestr(const char * const *usagestr,
 	FILE *outf = err ? stderr : stdout;
 	int ret;
 
-	ret = usage_command_internal(usagestr, token, full, 0, outf);
+	ret = usage_command_internal(usagestr, token, full, 0, 0, outf);
 	if (!ret)
 		fputc('\n', outf);
 }
@@ -146,7 +148,7 @@ static void usage_command_group_internal(const struct cmd_group *grp, int full,
 			}
 
 			usage_command_internal(cmd->usagestr, cmd->token, full,
-					       1, outf);
+					       1, cmd->flags & CMD_ALIAS, outf);
 			continue;
 		}
 
-- 
2.4.4

Re: [PATCH 0/2] btrfs device remove alias

[Duncan]

Omar Sandoval posted on Wed, 24 Jun 2015 09:09:15 -0700 as excerpted:

> The opposite of btrfs device add is btrfs device delete. This really
> should be btrfs device remove.

What about btrfs device subtract?  That's what _I_'d call the opposite of 
add.  Otherwise, add/remove instead of add/delete seems fine to me. Six 
of one, half dozen of the other.  [shrug]

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH 0/2] btrfs device remove alias

[David Sterba]

On Wed, Jun 24, 2015 at 09:09:15AM -0700, Omar Sandoval wrote:
> The opposite of btrfs device add is btrfs device delete. This really
> should be btrfs device remove.

I think people got used to the 'delete' command over time, but for
convenience I don't mind to add the alias. Also you delete files by 'rm'
which is short for 'remove' and probably don't mind either.

Re: [PATCH 0/2] btrfs device remove alias

[Chris Mason]

On Thu, Jun 25, 2015 at 03:41:51PM +0200, David Sterba wrote:
> On Wed, Jun 24, 2015 at 09:09:15AM -0700, Omar Sandoval wrote:
> > The opposite of btrfs device add is btrfs device delete. This really
> > should be btrfs device remove.
> 
> I think people got used to the 'delete' command over time, but for
> convenience I don't mind to add the alias. Also you delete files by 'rm'
> which is short for 'remove' and probably don't mind either.

I do agree that people got used to delete, but its one of those things
that new users are likely to trip over.  And since we're highly unlikely
to ever use 'rm' for something other than deletion, it makes sense to
just alias them.

-chris

Re: [PATCH 0/2] btrfs device remove alias

[Anand Jain]

while on this. its also good idea to create alias for

   btrfs replace start -> btrfs device replace.

any comments ?


On 06/25/2015 12:09 AM, Omar Sandoval wrote:
> The opposite of btrfs device add is btrfs device delete. This really
> should be btrfs device remove.
>
> Changes from v1:
> - Add support for flags to cmd_struct and a CMD_ALIAS flag which only
>    prints the one-line usage string
> - Rearrange the command wrappers in a way that could be made generic if
>    needed
>
> Thanks!
>
> Omar Sandoval (2):
>    btrfs-progs: replace struct cmd_group->hidden with flags
>    btrfs-progs: alias btrfs device delete to btrfs device remove
>
>   Documentation/btrfs-device.asciidoc |  5 ++++-
>   cmds-device.c                       | 35 ++++++++++++++++++++++++++---------
>   cmds-filesystem.c                   |  2 +-
>   commands.h                          |  9 +++++++--
>   help.c                              | 12 +++++++-----
>   5 files changed, 45 insertions(+), 18 deletions(-)
>

Re: [PATCH 0/2] btrfs device remove alias

[Duncan]

Anand Jain posted on Fri, 26 Jun 2015 09:10:56 +0800 as excerpted:

> while on this. its also good idea to create alias for
> 
>    btrfs replace start -> btrfs device replace.
> 
> any comments ?

That's actually the one that makes more sense to me.  Delete/remove/
subtract, all about the same to me, so while I'm not opposed to alias for 
that, I don't really see the need.  But with btrfs device add/remove, 
having btrfs replace instead of btrfs device replace, makes absolutely no 
sense to me, so I'm all for that alias.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH 0/2] btrfs device remove alias

[David Sterba]

On Fri, Jun 26, 2015 at 09:10:56AM +0800, Anand Jain wrote:
> while on this. its also good idea to create alias for
> 
>    btrfs replace start -> btrfs device replace.

This was asked for back then, and briefly discussed on irc (11/2012).
The preference was not to do too much typing, although the command
hierarchy would become less structured and can cause some trouble when
looking for docs.

I'm slightly worried about adding more aliases as it can cause confusion
when writing documentaiton and recommending how to do things.  But I
understand the motivations to make the interface more consistent or
convenient to use.

In this case it's moving replace to the expected place. I'm not against
it but more feedback would help.

Re: [PATCH 1/2] btrfs-progs: replace struct cmd_group->hidden with flags

[David Sterba]

On Wed, Jun 24, 2015 at 09:09:16AM -0700, Omar Sandoval wrote:
> We're also going to want to support aliases, so rather than adding
> another member, replace "hidden" with a "flags" member.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Fixed compilation breakage and applied, thanks.

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

[David Sterba]

On Wed, Jun 24, 2015 at 09:09:17AM -0700, Omar Sandoval wrote:
> 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>

Applied, thanks.

Re: [PATCH 0/2] btrfs device remove alias

[Goffredo Baroncelli]

On 2015-06-26 15:33, David Sterba wrote:
> On Fri, Jun 26, 2015 at 09:10:56AM +0800, Anand Jain wrote:
>> while on this. its also good idea to create alias for
>>
>>    btrfs replace start -> btrfs device replace.
> 
> This was asked for back then, and briefly discussed on irc (11/2012).
> The preference was not to do too much typing, although the command
> hierarchy would become less structured and can cause some trouble when
> looking for docs.
> 
> I'm slightly worried about adding more aliases as it can cause confusion
> when writing documentaiton and recommending how to do things.  But I
> understand the motivations to make the interface more consistent or
> convenient to use.
> 
> In this case it's moving replace to the expected place. I'm not against
> it but more feedback would help.

I agree that btrfs device replace makes sense. The point is what about btrfs
 replace status and cancel. 

We could add these subcommand to device too, also because it makes sense to
extend these to support the remove/delete command. I am not suggesting to
do that now, but our decision has to take in account also these (possible ?)
further alias.

Goffredo



-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5