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

[Hugo Mills <hugo@carfax.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

On Fri, Jul 01, 2011 at 12:13:30PM +0200, Andreas Philipp wrote:
> 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?

   Could you send a follow-up patch with just the second hunk, please?
I screwed up the process with this (processing patches too quickly to
catch the review), and I've already published the patch with the first
hunk, above, into the for-chris branch.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
    --- It's not so much an afterlife, more a sort of après vie. ---    

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]