Skip to contentSkip to conversation

btrfs device remove alias

PATCH Applied Omar Sandoval 6 participants 12 messages Jun 24, 2015 – Jun 27, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover btrfs device remove alias

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(-)
Signature · 1 line
2.4.4
#1 lore raw

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

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

---
 cmds-filesystem.c | 2 +-
 commands.h        | 8 ++++++--
 help.c            | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)
cmds-filesystem.c +1 -1
index b93bb33028f2..284dc87bc464 100644
@@ -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,
commands.h +6 -2
index bd23340d45ae..42d31781f1a3 100644
@@ -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}
help.c +1 -1
index 56aaf9c3c64c..34754c16e6fe 100644
@@ -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)
Signature · 1 line
2.4.4
#2 lore raw

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

Quoted 4 lines · Omar Sandoval

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

Fixed compilation breakage and applied, thanks.

#3 lore raw

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

In reply to Omar Sandoval

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

---
 Documentation/btrfs-device.asciidoc |  5 ++++-
 cmds-device.c                       | 35 ++++++++++++++++++++++++++---------
 commands.h                          |  1 +
 help.c                              | 10 ++++++----
 4 files changed, 37 insertions(+), 14 deletions(-)
Documentation/btrfs-device.asciidoc +4 -1
index c56cf5ef48fb..2827598a37f5 100644
@@ -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.
 
cmds-device.c +26 -9
index 1022656988c2..6972156fdf70 100644
@@ -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 },
commands.h +1 -0
index 42d31781f1a3..90bbd05a3542 100644
@@ -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 {
help.c +6 -4
index 34754c16e6fe..6ecf01d57b83 100644
@@ -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;
 		}
 
Signature · 1 line
2.4.4
Applied
#4 lore raw

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

Quoted 4 lines · Omar Sandoval

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

Applied, thanks.

#5 lore raw
In reply to Omar Sandoval

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

Quoted 2 lines

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]

Signature · 3 lines
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
#6 lore raw
In reply to Omar Sandoval
Quoted 2 lines · Omar Sandoval

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.

#7 lore raw
Quoted 7 lines · David Sterba
Quoted 2 lines · Omar Sandoval

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

#8 lore raw
In reply to Omar Sandoval

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

btrfs replace start -> btrfs device replace.

any comments ?

Quoted 22 lines · 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(-)
#9 lore raw

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

Quoted 5 lines

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.

Signature · 3 lines
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
#10 lore raw
In reply to Anand Jain
Quoted 3 lines · Anand Jain

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.

#11 lore raw
Quoted 10 lines · David Sterba
Quoted 3 lines · Anand Jain

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.

Quoted 7 lines

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

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