| Message ID | chaz20110701082652.GF28702@seebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;