Re: [PATCH 10/17] mkfs: add respecification detection to generic parsing

Brian Foster
2015-06-26T17:17:21Z

On Fri, Jun 19, 2015 at 01:01:59PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Add flags to the generic input parameter tables so that
> respecification can be detected in a generic manner.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 68 +++++++++++++++------------------------------------------
> 1 file changed, 18 insertions(+), 50 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 332c491..0bd8998 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -57,15 +57,17 @@ static long long cvtnum(unsigned int blocksize,
> struct opt_params {
> const char name;
> const char *subopts[MAX_SUBOPTS];
> +
> struct subopt_param {
> int index;
> + bool seen;
> long long minval;
> long long maxval;
> long long defaultval;
> } subopt_params[MAX_SUBOPTS];
> };
>
> -const struct opt_params bopts = {
> +struct opt_params bopts = {
> .name = 'b',
> .subopts = {
> #define B_LOG 0
> @@ -88,7 +90,7 @@ const struct opt_params bopts = {
> },
> };
>
> -const struct opt_params dopts = {
> +struct opt_params dopts = {
> .name = 'd',
> .subopts = {
> #define D_AGCOUNT 0
> @@ -197,7 +199,7 @@ const struct opt_params dopts = {
> };
>
>
> -const struct opt_params iopts = {
> +struct opt_params iopts = {
> .name = 'i',
> .subopts = {
> #define I_ALIGN 0
> @@ -257,7 +259,7 @@ const struct opt_params iopts = {
> },
> };
>
> -const struct opt_params lopts = {
> +struct opt_params lopts = {
> .name = 'l',
> .subopts = {
> #define L_AGNUM 0
> @@ -342,7 +344,7 @@ const struct opt_params lopts = {
> },
> };
>
> -const struct opt_params nopts = {
> +struct opt_params nopts = {
> .name = 'n',
> .subopts = {
> #define N_LOG 0
> @@ -379,7 +381,7 @@ const struct opt_params nopts = {
> },
> };
>
> -const struct opt_params ropts = {
> +struct opt_params ropts = {
> .name = 'r',
> .subopts = {
> #define R_EXTSIZE 0
> @@ -418,7 +420,7 @@ const struct opt_params ropts = {
> },
> };
>
> -const struct opt_params sopts = {
> +struct opt_params sopts = {

It would be nice to avoid all this noise by dropping the const
specifiers from the earlier patch. Otherwise, looks fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> .name = 's',
> .subopts = {
> #define S_LOG 0
> @@ -455,7 +457,7 @@ const struct opt_params sopts = {
> },
> };
>
> -const struct opt_params mopts = {
> +struct opt_params mopts = {
> .name = 'm',
> .subopts = {
> #define M_CRC 0
> @@ -1192,7 +1194,6 @@ struct sb_feat_args {
> int dir_version;
> int spinodes;
> int finobt;
> - bool finobtflag;
> bool inode_align;
> bool nci;
> bool lazy_sb_counters;
> @@ -1323,10 +1324,10 @@ illegal_option(
> static int
> getnum_checked(
> const char *str,
> - const struct opt_params *opts,
> + struct opt_params *opts,
> int index)
> {
> - const struct subopt_param *sp = &opts->subopt_params[index];
> + struct subopt_param *sp = &opts->subopt_params[index];
> long long c;
>
> if (sp->index != index) {
> @@ -1336,6 +1337,11 @@ getnum_checked(
> reqval(opts->name, (char **)opts->subopts, index);
> }
>
> + /* check for respecification of the option */
> + if (sp->seen)
> + respec(opts->name, (char **)opts->subopts, index);
> + sp->seen = true;
> +
> if (!str || *str == '\0') {
> if (sp->defaultval == SUBOPT_NEEDS_VAL)
> reqval(opts->name, (char **)opts->subopts, index);
> @@ -1446,7 +1452,6 @@ main(
> struct fs_topology ft;
> struct sb_feat_args sb_feat = {
> .finobt = 1,
> - .finobtflag = false,
> .spinodes = 0,
> .log_version = 2,
> .attr_version = 2,
> @@ -1506,8 +1511,6 @@ main(
> switch (getsubopt(&p, (constpp)subopts,
> &value)) {
> case B_LOG:
> - if (blflag)
> - respec('b', subopts, B_LOG);
> if (bsflag)
> conflict('b', subopts, B_SIZE,
> B_LOG);
> @@ -1546,9 +1549,6 @@ main(
> switch (getsubopt(&p, (constpp)subopts,
> &value)) {
> case D_AGCOUNT:
> - if (daflag)
> - respec('d', subopts, D_AGCOUNT);
> -
> agcount = getnum_checked(value, &dopts,
> D_AGCOUNT);
> daflag = 1;
> @@ -1585,8 +1585,6 @@ main(
> dsize = value;
> break;
> case D_SUNIT:
> - if (dsunit)
> - respec('d', subopts, D_SUNIT);
> if (nodsflag)
> conflict('d', subopts, D_NOALIGN,
> D_SUNIT);
> @@ -1594,8 +1592,6 @@ main(
> D_SUNIT);
> break;
> case D_SWIDTH:
> - if (dswidth)
> - respec('d', subopts, D_SWIDTH);
> if (nodsflag)
> conflict('d', subopts, D_NOALIGN,
> D_SWIDTH);
> @@ -1616,8 +1612,6 @@ main(
> illegal(value, "d su");
> break;
> case D_SW:
> - if (dsw)
> - respec('d', subopts, D_SW);
> if (nodsflag)
> conflict('d', subopts, D_NOALIGN,
> D_SW);
> @@ -1640,8 +1634,6 @@ main(
> nodsflag = 1;
> break;
> case D_SECTLOG:
> - if (slflag)
> - respec('d', subopts, D_SECTLOG);
> if (ssflag)
> conflict('d', subopts, D_SECTSIZE,
> D_SECTLOG);
> @@ -1704,8 +1696,6 @@ main(
> value, &iopts, I_ALIGN);
> break;
> case I_LOG:
> - if (ilflag)
> - respec('i', subopts, I_LOG);
> if (ipflag)
> conflict('i', subopts, I_PERBLOCK,
> I_LOG);
> @@ -1718,8 +1708,6 @@ main(
> ilflag = 1;
> break;
> case I_MAXPCT:
> - if (imflag)
> - respec('i', subopts, I_MAXPCT);
> imaxpct = getnum_checked(value, &iopts,
> I_MAXPCT);
> imflag = 1;
> @@ -1728,8 +1716,6 @@ main(
> if (ilflag)
> conflict('i', subopts, I_LOG,
> I_PERBLOCK);
> - if (ipflag)
> - respec('i', subopts, I_PERBLOCK);
> if (isflag)
> conflict('i', subopts, I_SIZE,
> I_PERBLOCK);
> @@ -1746,8 +1732,6 @@ main(
> if (ipflag)
> conflict('i', subopts, I_PERBLOCK,
> I_SIZE);
> - if (isflag)
> - respec('i', subopts, I_SIZE);
> isize = getnum_checked(value, &iopts,
> I_SIZE);
> if (!ispow2(isize))
> @@ -1786,8 +1770,6 @@ main(
> switch (getsubopt(&p, (constpp)subopts,
> &value)) {
> case L_AGNUM:
> - if (laflag)
> - respec('l', subopts, L_AGNUM);
> if (ldflag)
> conflict('l', subopts, L_AGNUM, L_DEV);
> logagno = getnum_checked(value, &lopts,
> @@ -1809,8 +1791,6 @@ main(
> if (xi.lisfile)
> conflict('l', subopts, L_FILE,
> L_INTERNAL);
> - if (liflag)
> - respec('l', subopts, L_INTERNAL);
>
> loginternal = getnum_checked(value,
> &lopts, L_INTERNAL);
> @@ -1828,8 +1808,6 @@ main(
> lsuflag = 1;
> break;
> case L_SUNIT:
> - if (lsunit)
> - respec('l', subopts, L_SUNIT);
> lsunit = getnum_checked(value, &lopts,
> L_SUNIT);
> lsunitflag = 1;
> @@ -1850,8 +1828,6 @@ main(
> xi.logname = value;
> break;
> case L_VERSION:
> - if (lvflag)
> - respec('l', subopts, L_VERSION);
> sb_feat.log_version =
> getnum_checked(value, &lopts,
> L_VERSION);
> @@ -1866,8 +1842,6 @@ main(
> lsflag = 1;
> break;
> case L_SECTLOG:
> - if (lslflag)
> - respec('l', subopts, L_SECTLOG);
> if (lssflag)
> conflict('l', subopts, L_SECTSIZE,
> L_SECTLOG);
> @@ -1950,8 +1924,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> switch (getsubopt(&p, (constpp)subopts,
> &value)) {
> case N_LOG:
> - if (nlflag)
> - respec('n', subopts, N_LOG);
> if (nsflag)
> conflict('n', subopts, N_SIZE,
> N_LOG);
> @@ -1994,8 +1966,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> nvflag = 1;
> break;
> case N_FTYPE:
> - if (nftype)
> - respec('n', subopts, N_FTYPE);
> if (sb_feat.crcs_enabled) {
> fprintf(stderr,
> _("cannot specify both -m crc=1 and -n ftype\n"));
> @@ -2078,8 +2048,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
> &value)) {
> case S_LOG:
> case S_SECTLOG:
> - if (slflag || lslflag)
> - respec('s', subopts, S_SECTLOG);
> if (ssflag || lssflag)
> conflict('s', subopts,
> S_SECTSIZE, S_SECTLOG);
> @@ -2296,7 +2264,7 @@ _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
> * tried to use crc=0,finobt=1, then issue a warning before
> * turning them off.
> */
> - if (sb_feat.finobt && sb_feat.finobtflag) {
> + if (sb_feat.finobt && mopts.subopt_params[M_FINOBT].seen) {
> fprintf(stderr,
> _("warning: finobt not supported without CRC support, disabled.\n"));
> sb_feat.finobt = 0;
> --
> 2.1.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs