| Message ID | 83c7d294299d9b66238f7369b1171c24d35b9294.1434508872.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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) -- 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
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!
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. -- 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/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 } };
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(-)