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

[Omar Sandoval <osandov@fb.com>]

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