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

From: Brian Foster <bfoster@redhat.com>
To: "Jan Ťulák" <jtulak@redhat.com>
Cc: Dave Chinner <dchinner@redhat.com>, xfs@oss.sgi.com
Subject: Re: [PATCH 13/17] mkfs: encode conflicts into parsing table
Date: Fri, 26 Jun 2015 13:17:31 -0400	[thread overview]
Message-ID: <20150626171730.GI40750@bfoster.bfoster> (raw)
In-Reply-To: <1434711726-13092-14-git-send-email-jtulak@redhat.com>

On Fri, Jun 19, 2015 at 01:02:02PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs