xfs
[Top] [All Lists]

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

To: Jan ÅulÃk <jtulak@xxxxxxxxxx>
Subject: Re: [PATCH 10/17] mkfs: add respecification detection to generic parsing
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 26 Jun 2015 13:17:17 -0400
Cc: xfs@xxxxxxxxxxx, Dave Chinner <dchinner@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1434711726-13092-11-git-send-email-jtulak@xxxxxxxxxx>
References: <1434711726-13092-1-git-send-email-jtulak@xxxxxxxxxx> <1434711726-13092-11-git-send-email-jtulak@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Jun 19, 2015 at 01:01:59PM +0200, Jan ÅulÃk wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Add flags to the generic input parameter tables so that
> respecification can be detected in a generic manner.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan ÅulÃk <jtulak@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

>       .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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>