xfs
[Top] [All Lists]

Re: [PATCH 13/17] mkfs: encode conflicts into parsing table

To: Jan ÅulÃk <jtulak@xxxxxxxxxx>
Subject: Re: [PATCH 13/17] mkfs: encode conflicts into parsing table
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Fri, 26 Jun 2015 13:17:31 -0400
Cc: xfs@xxxxxxxxxxx, Dave Chinner <dchinner@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1434711726-13092-14-git-send-email-jtulak@xxxxxxxxxx>
References: <1434711726-13092-1-git-send-email-jtulak@xxxxxxxxxx> <1434711726-13092-14-git-send-email-jtulak@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Jun 19, 2015 at 01:02:02PM +0200, Jan ÅulÃk wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Many options conflict, so we need to specify which options conflict
> with each other in a generic manner. We already have a "seen"
> variable used for respecification detection, so we can also use this
> code conflict detection. Hence add a "conflicts" array to the sub
> options parameter definition.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> Signed-off-by: Jan ÅulÃk <jtulak@xxxxxxxxxx>
> ---
>  mkfs/xfs_mkfs.c | 248 
> ++++++++++++++++++++++++++++----------------------------
>  1 file changed, 125 insertions(+), 123 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index e52fd4e..1d80188 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -58,6 +58,9 @@ unsigned int                sectorsize;
>  
>  #define MAX_SUBOPTS  16
>  #define SUBOPT_NEEDS_VAL     (-1LL)
> +
> +#define MAX_CONFLICTS        8
> +#define LAST_CONFLICT        (-1)
>  struct opt_params {
>       const char      name;
>       const char      *subopts[MAX_SUBOPTS];
...
>  #ifdef ENABLE_BLKID
> @@ -1360,6 +1433,16 @@ getnum(
>               respec(opts->name, (char **)opts->subopts, index);
>       sp->seen = true;
>  
> +     /* check for conflicts with the option */
> +     for (c = 0; c < MAX_CONFLICTS; c++) {
> +             int conflict_opt = sp->conflicts[c];
> +
> +             if (conflict_opt == LAST_CONFLICT)
> +                     break;
> +             if (opts->subopt_params[conflict_opt].seen)
> +                     conflict(opts->name, (char **)opts->subopts, 
> conflict_opt, index);

Long line.

> +     }
> +
>       /* empty strings might just return a default value */
>       if (!str || *str == '\0') {
>               if (sp->defaultval == SUBOPT_NEEDS_VAL)
...
> @@ -2020,7 +2027,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>                                                 &value)) {
>                               case S_LOG:
>                               case S_SECTLOG:
> -                                     if (ssflag || lssflag)
> +                                     if (lssflag)
>                                               conflict('s', subopts,
>                                                        S_SECTSIZE, S_SECTLOG);
>                                       sectorlog = getnum(value, &sopts,
> @@ -2032,7 +2039,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>                                       break;
>                               case S_SIZE:
>                               case S_SECTSIZE:
> -                                     if (slflag || lslflag)
> +                                     if (lslflag)
>                                               conflict('s', subopts, 
> S_SECTLOG,
>                                                        S_SECTSIZE);

Hmm.. so is the limitation here that we can't do generic conflict
detection across different option structs? If so, I suppose that's not
the end of the world. The cleanup is still well worth it. I wonder if we
still need to set lslflag/lssflag in either of the above cases, though.
It seems like the generic detection should handle it..?

Also, I'd expect some documentation somewhere up where the option struct
is defined to clearly state any such rules.

Brian

>                                       sectorsize = getnum(value, &sopts,
> @@ -2257,11 +2264,6 @@ _("warning: sparse inodes not supported without CRC 
> support, disabled.\n"));
>               dirblocksize = 1 << dirblocklog;
>       }
>  
> -     if (daflag && dasize) {
> -             fprintf(stderr,
> -     _("both -d agcount= and agsize= specified, use one or the other\n"));
> -             usage();
> -     }
>  
>       if (xi.disfile && (!dsize || !xi.dname)) {
>               fprintf(stderr,
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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