| Message ID | 1422371153-17355-1-git-send-email-hugo@carfax.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jan 27, 2015 at 03:05:52PM +0000, Hugo Mills wrote: > The current approach to option parsing, where long-only options are > selected on the basis of their position in the long_options array is > fragile and painful to modify if options are to be inserted into the > list, rather than appended. > > Instead, use the last field of struct option to return a value which > cannot be a char (and hence a short option), and simply switch on those > within the case statement. > > Signed-off-by: Hugo Mills <hugo@carfax.org.uk> Both applied, thanks. -- 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
-------- Original Message -------- Subject: [PATCH 1/2] btrfs-progs: Make option parsing more robust to code modifications From: Hugo Mills <hugo@carfax.org.uk> To: <linux-btrfs@vger.kernel.org>, <dsterba@suse.cz> Date: 2015?01?27? 23:05 > The current approach to option parsing, where long-only options are > selected on the basis of their position in the long_options array is > fragile and painful to modify if options are to be inserted into the > list, rather than appended. > > Instead, use the last field of struct option to return a value which > cannot be a char (and hence a short option), and simply switch on those > within the case statement. This is much much better than original immediate number. The original way always takes me extra time to count the number. > > Signed-off-by: Hugo Mills <hugo@carfax.org.uk> > --- > cmds-check.c | 49 +++++++++++++++++++++++++++---------------------- > 1 file changed, 27 insertions(+), 22 deletions(-) > > diff --git a/cmds-check.c b/cmds-check.c > index f06e029..a1226c6 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -8403,13 +8403,15 @@ out: > return bad_roots; > } > > +enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM }; > + I'm a little interested in why assigning 257 to OPT_REPAIR. Does it have any extra meaning? > static struct option long_options[] = { > { "super", 1, NULL, 's' }, > - { "repair", 0, NULL, 0 }, > - { "init-csum-tree", 0, NULL, 0 }, > - { "init-extent-tree", 0, NULL, 0 }, > - { "check-data-csum", 0, NULL, 0 }, > - { "backup", 0, NULL, 0 }, > + { "repair", 0, NULL, OPT_REPAIR }, > + { "init-csum-tree", 0, NULL, OPT_INIT_CSUM }, > + { "init-extent-tree", 0, NULL, OPT_INIT_EXTENT }, > + { "check-data-csum", 0, NULL, OPT_CHECK_CSUM }, > + { "backup", 0, NULL, 'b' }, > { "subvol-extents", 1, NULL, 'E' }, > { "qgroup-report", 0, NULL, 'Q' }, > { "tree-root", 1, NULL, 'r' }, > @@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv) > case '?': > case 'h': > usage(cmd_check_usage); > - } > - if (option_index == 1) { > - printf("enabling repair mode\n"); > - repair = 1; > - ctree_flags |= OPEN_CTREE_WRITES; > - } else if (option_index == 2) { > - printf("Creating a new CRC tree\n"); > - init_csum_tree = 1; > - repair = 1; > - ctree_flags |= OPEN_CTREE_WRITES; > - } else if (option_index == 3) { > - init_extent_tree = 1; > - ctree_flags |= (OPEN_CTREE_WRITES | > - OPEN_CTREE_NO_BLOCK_GROUPS); > - repair = 1; > - } else if (option_index == 4) { > - check_data_csum = 1; > + case OPT_REPAIR: > + printf("enabling repair mode\n"); > + repair = 1; > + ctree_flags |= OPEN_CTREE_WRITES; > + break; > + case OPT_INIT_CSUM: > + printf("Creating a new CRC tree\n"); > + init_csum_tree = 1; > + repair = 1; > + ctree_flags |= OPEN_CTREE_WRITES; > + break; > + case OPT_INIT_EXTENT: > + init_extent_tree = 1; > + ctree_flags |= (OPEN_CTREE_WRITES | > + OPEN_CTREE_NO_BLOCK_GROUPS); > + repair = 1; > + break; > + case OPT_CHECK_CSUM: > + check_data_csum = 1; > + break; > } > } > argc = argc - optind; The rest looks good to me. Thanks, Qu -- 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 Wed, Jan 28, 2015 at 09:15:42AM +0800, Qu Wenruo wrote: > -------- Original Message -------- > Subject: [PATCH 1/2] btrfs-progs: Make option parsing more robust to > code modifications > From: Hugo Mills <hugo@carfax.org.uk> > To: <linux-btrfs@vger.kernel.org>, <dsterba@suse.cz> > Date: 2015?01?27? 23:05 > >The current approach to option parsing, where long-only options are > >selected on the basis of their position in the long_options array is > >fragile and painful to modify if options are to be inserted into the > >list, rather than appended. > > > >Instead, use the last field of struct option to return a value which > >cannot be a char (and hence a short option), and simply switch on those > >within the case statement. > This is much much better than original immediate number. > The original way always takes me extra time to count the number. > > > >Signed-off-by: Hugo Mills <hugo@carfax.org.uk> > >--- > > cmds-check.c | 49 +++++++++++++++++++++++++++---------------------- > > 1 file changed, 27 insertions(+), 22 deletions(-) > > > >diff --git a/cmds-check.c b/cmds-check.c > >index f06e029..a1226c6 100644 > >--- a/cmds-check.c > >+++ b/cmds-check.c > >@@ -8403,13 +8403,15 @@ out: > > return bad_roots; > > } > >+enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM }; > >+ > I'm a little interested in why assigning 257 to OPT_REPAIR. > Does it have any extra meaning? getopt_long returns the character code of the option, for short options, and returns the struct option->val value for a long option. If you want to avoid handling options twice (once for the short, and once for the long), it makes sense to return the short character code as the "val" for the long options, where one exists. Where it doesn't exist, we need some other value which isn't going to cause clashes with the single-character namespace. Hence starting above the value range of char. In hindsight, starting at 256 would have been good, but it's actually completely irrelevant, as long as the value is strictly larger than 255. Hugo. > > static struct option long_options[] = { > > { "super", 1, NULL, 's' }, > >- { "repair", 0, NULL, 0 }, > >- { "init-csum-tree", 0, NULL, 0 }, > >- { "init-extent-tree", 0, NULL, 0 }, > >- { "check-data-csum", 0, NULL, 0 }, > >- { "backup", 0, NULL, 0 }, > >+ { "repair", 0, NULL, OPT_REPAIR }, > >+ { "init-csum-tree", 0, NULL, OPT_INIT_CSUM }, > >+ { "init-extent-tree", 0, NULL, OPT_INIT_EXTENT }, > >+ { "check-data-csum", 0, NULL, OPT_CHECK_CSUM }, > >+ { "backup", 0, NULL, 'b' }, > > { "subvol-extents", 1, NULL, 'E' }, > > { "qgroup-report", 0, NULL, 'Q' }, > > { "tree-root", 1, NULL, 'r' }, > >@@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv) > > case '?': > > case 'h': > > usage(cmd_check_usage); > >- } > >- if (option_index == 1) { > >- printf("enabling repair mode\n"); > >- repair = 1; > >- ctree_flags |= OPEN_CTREE_WRITES; > >- } else if (option_index == 2) { > >- printf("Creating a new CRC tree\n"); > >- init_csum_tree = 1; > >- repair = 1; > >- ctree_flags |= OPEN_CTREE_WRITES; > >- } else if (option_index == 3) { > >- init_extent_tree = 1; > >- ctree_flags |= (OPEN_CTREE_WRITES | > >- OPEN_CTREE_NO_BLOCK_GROUPS); > >- repair = 1; > >- } else if (option_index == 4) { > >- check_data_csum = 1; > >+ case OPT_REPAIR: > >+ printf("enabling repair mode\n"); > >+ repair = 1; > >+ ctree_flags |= OPEN_CTREE_WRITES; > >+ break; > >+ case OPT_INIT_CSUM: > >+ printf("Creating a new CRC tree\n"); > >+ init_csum_tree = 1; > >+ repair = 1; > >+ ctree_flags |= OPEN_CTREE_WRITES; > >+ break; > >+ case OPT_INIT_EXTENT: > >+ init_extent_tree = 1; > >+ ctree_flags |= (OPEN_CTREE_WRITES | > >+ OPEN_CTREE_NO_BLOCK_GROUPS); > >+ repair = 1; > >+ break; > >+ case OPT_CHECK_CSUM: > >+ check_data_csum = 1; > >+ break; > > } > > } > > argc = argc - optind; > The rest looks good to me. > > Thanks, > Qu
diff --git a/cmds-check.c b/cmds-check.c index f06e029..a1226c6 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -8403,13 +8403,15 @@ out: return bad_roots; } +enum { OPT_REPAIR = 257, OPT_INIT_CSUM, OPT_INIT_EXTENT, OPT_CHECK_CSUM }; + static struct option long_options[] = { { "super", 1, NULL, 's' }, - { "repair", 0, NULL, 0 }, - { "init-csum-tree", 0, NULL, 0 }, - { "init-extent-tree", 0, NULL, 0 }, - { "check-data-csum", 0, NULL, 0 }, - { "backup", 0, NULL, 0 }, + { "repair", 0, NULL, OPT_REPAIR }, + { "init-csum-tree", 0, NULL, OPT_INIT_CSUM }, + { "init-extent-tree", 0, NULL, OPT_INIT_EXTENT }, + { "check-data-csum", 0, NULL, OPT_CHECK_CSUM }, + { "backup", 0, NULL, 'b' }, { "subvol-extents", 1, NULL, 'E' }, { "qgroup-report", 0, NULL, 'Q' }, { "tree-root", 1, NULL, 'r' }, @@ -8483,23 +8485,26 @@ int cmd_check(int argc, char **argv) case '?': case 'h': usage(cmd_check_usage); - } - if (option_index == 1) { - printf("enabling repair mode\n"); - repair = 1; - ctree_flags |= OPEN_CTREE_WRITES; - } else if (option_index == 2) { - printf("Creating a new CRC tree\n"); - init_csum_tree = 1; - repair = 1; - ctree_flags |= OPEN_CTREE_WRITES; - } else if (option_index == 3) { - init_extent_tree = 1; - ctree_flags |= (OPEN_CTREE_WRITES | - OPEN_CTREE_NO_BLOCK_GROUPS); - repair = 1; - } else if (option_index == 4) { - check_data_csum = 1; + case OPT_REPAIR: + printf("enabling repair mode\n"); + repair = 1; + ctree_flags |= OPEN_CTREE_WRITES; + break; + case OPT_INIT_CSUM: + printf("Creating a new CRC tree\n"); + init_csum_tree = 1; + repair = 1; + ctree_flags |= OPEN_CTREE_WRITES; + break; + case OPT_INIT_EXTENT: + init_extent_tree = 1; + ctree_flags |= (OPEN_CTREE_WRITES | + OPEN_CTREE_NO_BLOCK_GROUPS); + repair = 1; + break; + case OPT_CHECK_CSUM: + check_data_csum = 1; + break; } } argc = argc - optind;
The current approach to option parsing, where long-only options are selected on the basis of their position in the long_options array is fragile and painful to modify if options are to be inserted into the list, rather than appended. Instead, use the last field of struct option to return a value which cannot be a char (and hence a short option), and simply switch on those within the case statement. Signed-off-by: Hugo Mills <hugo@carfax.org.uk> --- cmds-check.c | 49 +++++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 22 deletions(-)