Re: [PATCH] xfsprogs: cleanup size/log setting flags of mkfs

[Dave Chinner <david@fromorbit.com>]

On Sun, Sep 29, 2013 at 05:12:51PM +0800, Li Zhong wrote:
> As Eric suggested, we could set both of the size/log flags after we have
> parsed the options - and from there on it simply means "manually set".
> 
> After that, we could use just one flag, e.g. *sflag, to check whether 
> the corresponding value is manually set or not.

It's a start, but I'm not sure that it is an improvement or not.
i.e. you're adding yet another piece of logic to the already
tortured argument parsing and flag setting.

This could be done in the argument parsing itself, without needing
separate post-processing code. e.g. changing the parsing code like
so:

	case N_LOG:
		if (!value || *value == '\0')
			reqval('n', nopts, N_LOG);
-		if (nlflag)
+		if (nlflag > 1)
			respec('n', nopts, N_LOG);
		if (nsflag)
			conflict('n', nopts, N_SIZE,
				 N_LOG);
+		nlflag = 2;
		dirblocklog = atoi(value);
		if (dirblocklog <= 0)
			illegal(value, "n log");
+		nsflag = 1;
		dirblocksize = 1 << dirblocklog;
-		nlflag = 1;
		break;

Would acheive exactly the same thing - i.e.
a value of 1 means it was initialised, a value of 2 means it was a
command line parameter...

This means the code checks can be cleaned up as you have done, but
we don't need a separate post-processing step for the arguments to
set flags that weren't set...


> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>  mkfs/xfs_mkfs.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 34bf2ff..aa3f391 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1667,11 +1667,26 @@ main(
>  		dfile = xi.dname;
>  
>  	/*
> +	 * Later code wants to know if the user manually set a value.
> +	 * There are two ways to specify on the cmdline; as size or as a log.
> +	 * if either was used, set both flags - from here on it simply means
> +	 * "manually set"
> +	 */
> +	if (bsflag || blflag)
> +		bsflag = blflag = 1;
> +	if (ssflag || slflag)
> +		ssflag = slflag = 1;
> +	if (isflag || ilflag)
> +		isflag = ilflag = 1;
> +	if (nsflag || nlflag)
> +		nsflag = nlflag = 1;

You missed the log sector size/log flags.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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