Re: [PATCH] Re: [btrfs-progs integration] incorrect argument checking for "btrfs sub snap -r"

[Andreas Philipp <philipp.andreas@gmail.com>]

On 01.07.2011 10:26, Stephane Chazelas wrote:
> 2011-06-30 22:55:15 +0200, Andreas Philipp:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>  
>> On 30.06.2011 14:34, Stephane Chazelas wrote:
>>> Looks like this was missing in integration-20110626 for the
>>> readonly snapshot patch:
>>>
>>> diff --git a/btrfs.c b/btrfs.c
>>> index e117172..be6ece5 100644
>>> --- a/btrfs.c
>>> +++ b/btrfs.c
>>> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>>> /*
>>> avoid short commands different for the case only
>>> */
>>> - { do_clone, 2,
>>> + { do_clone, -1,
>>> "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>>> "Create a writable/readonly snapshot of the subvolume <source> with\n"
>>> "the name <name> in the <dest> directory.",
>>>
>>> Without that, "btrfs sub snap -r x y" would fail as it's not *2*
>>> arguments.
>> Unfortunately, this is not correct either. "-1" means that the minimum
>> number of arguments is 1 and since we need at least <source> and
>> <name> this is 2. So the correct version should be -2.
> [...]
>
> Sorry, without looking closely at the source, I assumed -1 meant
> defer the checking to the subcommand handler.
>
> do_clone will indeed return an error if the number of arguments
> is less than expected (so with -2, you'll get a different error
> message if you do "btrfs sub snap -r foo" or "btrfs sub snap
> foo") , but will not if it's more than expected by the way.
>
> So the patch should probably be:
>
> diff --git a/btrfs.c b/btrfs.c
> index e117172..b50c58a 100644
> --- a/btrfs.c
> +++ b/btrfs.c
> @@ -49,7 +49,7 @@ static struct Command commands[] = {
>  	/*
>  		avoid short commands different for the case only
>  	*/
> -	{ do_clone, 2,
> +	{ do_clone, -2,
>  	  "subvolume snapshot", "[-r] <source> [<dest>/]<name>\n"
>  		"Create a writable/readonly snapshot of the subvolume <source> with\n"
>  		"the name <name> in the <dest> directory.",
> diff --git a/btrfs_cmds.c b/btrfs_cmds.c
> index 1d18c59..3415afc 100644
> --- a/btrfs_cmds.c
> +++ b/btrfs_cmds.c
> @@ -355,7 +355,7 @@ int do_clone(int argc, char **argv)
>  			return 1;
>  		}
>  	}
> -	if (argc - optind < 2) {
> +	if (argc - optind != 2) {
>  		fprintf(stderr, "Invalid arguments for subvolume snapshot\n");
>  		free(argv);
>  		return 1;
>
Thanks for having another look at this. You are perfectly right. Should
we patch my patch or should I rework a corrected version? What do you
think Hugo?

Cheers,
Andreas