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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
