On Fri, Jun 19, 2015 at 01:01:56PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Passing large number of parameters around to number conversion
> functions is painful. Add a structure to encapsulate the constant
> parameters that are passed, and convert getnum_checked to use it.
>
> This is the first real step towards a table driven option parser.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan Ťulák <jtulak@xxxxxxxxxx>
> ---
> mkfs/xfs_mkfs.c | 602 ++++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 392 insertions(+), 210 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6f0aa55..d3d1e11 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -52,145 +52,322 @@ static int ispow2(unsigned int i);
> static long long cvtnum(unsigned int blocksize,
> unsigned int sectorsize, const char *s);
>
...
>
> -char *iopts[] = {
> +
> +const struct opt_params iopts = {
> + .name = 'i',
> + .subopts = {
> #define I_ALIGN 0
> - "align",
> + "align",
> #define I_LOG 1
> - "log",
> + "log",
> #define I_MAXPCT 2
> - "maxpct",
> + "maxpct",
> #define I_PERBLOCK 3
> - "perblock",
> + "perblock",
> #define I_SIZE 4
> - "size",
> + "size",
> #define I_ATTR 5
> - "attr",
> + "attr",
> #define I_PROJID32BIT 6
> "projid32bit",
> #define I_SPINODES 7
> "sparse",
> NULL
Nit... the last few strings in the above aren't aligned properly. Looks
like they weren't touched, so perhaps just an artifact of forward
porting the series.
> + },
> + .subopt_params = {
> + { .index = I_ALIGN,
> + },
> + { .index = I_LOG,
> + .minval = XFS_DINODE_MIN_LOG,
> + .maxval = XFS_DINODE_MAX_LOG,
> + },
> + { .index = I_MAXPCT,
> + },
> + { .index = I_PERBLOCK,
> + },
> + { .index = I_SIZE,
> + },
> + { .index = I_ATTR,
> + },
> + { .index = I_PROJID32BIT,
> + },
Missing I_SPINODES here.
> + },
> };
>
...
> +
> static int
> getnum_checked(
> const char *str,
> - long long min_val,
> - long long max_val,
> - const char *illegal_str,
> - char reqval_char,
> - char *reqval_opts[],
> - int reqval_optind)
> + const struct opt_params *opts,
> + int index)
> {
> long long c;
>
> if (!str || *str == '\0')
> - reqval(reqval_char, reqval_opts, reqval_optind);
> + reqval(opts->name, (char **)opts->subopts, index);
>
> c = getnum(str, 0, 0, false);
> - if (c < min_val || c > max_val)
> - illegal(str, illegal_str);
> + if (c < opts->subopt_params[index].minval ||
> + c > opts->subopt_params[index].maxval)
> + illegal_option(str, opts, index);
Some kind of error, assert or updated logic to ensure minval and maxval
are specified would be nice here so failures to use the correct
validation function (or to specify the values) are more obvious.
> return c;
> }
>
...
> @@ -1662,9 +1842,11 @@ main(
> case 'm':
> p = optarg;
> while (*p != '\0') {
> + char **subopts = (char **)mopts.subopts;
> char *value;
>
> - switch (getsubopt(&p, (constpp)mopts, &value)) {
> + switch (getsubopt(&p, (constpp)subopts,
> + &value)) {
> case M_CRC:
> sb_feat.crcs_enabled = getbool(
> value, "m crc", false);
> @@ -1678,7 +1860,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> break;
> case M_FINOBT:
> if (!value || *value == '\0')
> - reqval('m', mopts, M_CRC);
> + reqval('m', (char**)mopts.subopts, M_CRC);
Can probably use subopts here..?
Brian
> c = atoi(value);
> if (c < 0 || c > 1)
> illegal(value, "m finobt");
> @@ -1692,30 +1874,29 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> case 'n':
> p = optarg;
> while (*p != '\0') {
> + char **subopts = (char **)nopts.subopts;
> char *value;
>
> - switch (getsubopt(&p, (constpp)nopts, &value)) {
> + switch (getsubopt(&p, (constpp)subopts,
> + &value)) {
> case N_LOG:
> if (nlflag)
> - respec('n', nopts, N_LOG);
> + respec('n', subopts, N_LOG);
> if (nsflag)
> - conflict('n', nopts, N_SIZE,
> + conflict('n', subopts, N_SIZE,
> N_LOG);
> dirblocklog = getnum_checked(value,
> - XFS_MIN_REC_DIRSIZE,
> - XFS_MAX_BLOCKSIZE_LOG,
> - "n log", 'n', nopts,
> - N_LOG);
> + &nopts, N_LOG);
> dirblocksize = 1 << dirblocklog;
> nlflag = 1;
> break;
> case N_SIZE:
> if (!value || *value == '\0')
> - reqval('n', nopts, N_SIZE);
> + reqval('n', subopts, N_SIZE);
> if (nsflag)
> - respec('n', nopts, N_SIZE);
> + respec('n', subopts, N_SIZE);
> if (nlflag)
> - conflict('n', nopts, N_LOG,
> + conflict('n', subopts, N_LOG,
> N_SIZE);
> dirblocksize = getnum(value, blocksize,
> sectorsize, true);
> @@ -1728,9 +1909,9 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> break;
> case N_VERSION:
> if (!value || *value == '\0')
> - reqval('n', nopts, N_VERSION);
> + reqval('n', subopts, N_VERSION);
> if (nvflag)
> - respec('n', nopts, N_VERSION);
> + respec('n', subopts, N_VERSION);
> if (!strcasecmp(value, "ci")) {
> /* ASCII CI mode */
> sb_feat.nci = true;
> @@ -1745,7 +1926,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> break;
> case N_FTYPE:
> if (nftype)
> - respec('n', nopts, N_FTYPE);
> + respec('n', subopts, N_FTYPE);
> if (sb_feat.crcs_enabled) {
> fprintf(stderr,
> _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -1777,14 +1958,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> case 'r':
> p = optarg;
> while (*p != '\0') {
> + char **subopts = (char **)ropts.subopts;
> char *value;
>
> - switch (getsubopt(&p, (constpp)ropts, &value)) {
> + switch (getsubopt(&p, (constpp)subopts,
> + &value)) {
> case R_EXTSIZE:
> if (!value || *value == '\0')
> - reqval('r', ropts, R_EXTSIZE);
> + reqval('r', subopts, R_EXTSIZE);
> if (rtextsize)
> - respec('r', ropts, R_EXTSIZE);
> + respec('r', subopts, R_EXTSIZE);
> rtextsize = value;
> break;
> case R_FILE:
> @@ -1796,16 +1979,16 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> case R_NAME:
> case R_DEV:
> if (!value || *value == '\0')
> - reqval('r', ropts, R_NAME);
> + reqval('r', subopts, R_NAME);
> if (xi.rtname)
> - respec('r', ropts, R_NAME);
> + respec('r', subopts, R_NAME);
> xi.rtname = value;
> break;
> case R_SIZE:
> if (!value || *value == '\0')
> - reqval('r', ropts, R_SIZE);
> + reqval('r', subopts, R_SIZE);
> if (rtsize)
> - respec('r', ropts, R_SIZE);
> + respec('r', subopts, R_SIZE);
> rtsize = value;
> break;
> case R_NOALIGN:
> @@ -1819,21 +2002,20 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> case 's':
> p = optarg;
> while (*p != '\0') {
> + char **subopts = (char **)sopts.subopts;
> char *value;
>
> - switch (getsubopt(&p, (constpp)sopts, &value)) {
> + switch (getsubopt(&p, (constpp)subopts,
> + &value)) {
> case S_LOG:
> case S_SECTLOG:
> if (slflag || lslflag)
> - respec('s', sopts, S_SECTLOG);
> + respec('s', subopts, S_SECTLOG);
> if (ssflag || lssflag)
> - conflict('s', sopts, S_SECTSIZE,
> - S_SECTLOG);
> - sectorlog = getnum_checked(value,
> - XFS_MIN_SECTORSIZE_LOG,
> - XFS_MAX_SECTORSIZE_LOG,
> - "s sectlog", 's', sopts,
> - S_SECTLOG);
> + conflict('s', subopts,
> + S_SECTSIZE, S_SECTLOG);
> + sectorlog = getnum_checked(value, &sopts,
> + S_SECTLOG);
> lsectorlog = sectorlog;
> sectorsize = 1 << sectorlog;
> lsectorsize = sectorsize;
> @@ -1842,11 +2024,11 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> case S_SIZE:
> case S_SECTSIZE:
> if (!value || *value == '\0')
> - reqval('s', sopts, S_SECTSIZE);
> + reqval('s', subopts, S_SECTSIZE);
> if (ssflag || lssflag)
> - respec('s', sopts, S_SECTSIZE);
> + respec('s', subopts, S_SECTSIZE);
> if (slflag || lslflag)
> - conflict('s', sopts, S_SECTLOG,
> + conflict('s', subopts, S_SECTLOG,
> S_SECTSIZE);
> sectorsize = getnum(value, blocksize,
> sectorsize, true);
> --
> 2.1.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
