Re: [PATCH 08/12] mkfs: replace variables with opts table: -b,d,s options

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Sun, Apr 23, 2017 at 08:54:59PM +0200, Jan Tulak wrote:
> Remove variables that can be replaced with a direct access to the opts table,
> so we have it all in a single place, acessible from anywhere.
> 
> In future, we can remove some passing of values forth and back, but now limit
> the changes to simple replacement without a change in the logic.

What do you mean passing of values here, as an example with bopts ?

> This is first of multiple similar patches that do the same, but for other
> options.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 814 ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 503 insertions(+), 311 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 362c9b4..6857c30 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1485,15 +1480,12 @@ main(
>  	int			argc,
>  	char			**argv)
>  {
> -	uint64_t		agcount;
>  	xfs_agf_t		*agf;
>  	xfs_agi_t		*agi;
>  	xfs_agnumber_t		agno;
> -	uint64_t		agsize;
>  	xfs_alloc_rec_t		*arec;
>  	struct xfs_btree_block	*block;
>  	bool			blflag;

Note: blflag

> -	uint64_t		blocklog;
>  	bool			bsflag;

Note: bsflag

>  	memset(&fsx, 0, sizeof(fsx));
> @@ -1629,6 +1613,7 @@ main(
>  			break;
>  		case 'b':
>  			p = optarg;
> +			uint64_t tmp;
>  			while (*p != '\0') {
>  				char	**subopts =
>  						(char **)opts[OPT_B].subopts;
> @@ -1636,19 +1621,17 @@ main(
>  
>  				switch (getsubopt(&p, subopts, &value)) {
>  				case B_LOG:
> -					blocklog = parse_conf_val(OPT_B, B_LOG,
> -								  value);
> -					blocksize = 1 << blocklog;
> +					tmp = parse_conf_val(OPT_B, B_LOG,
> +							     value);
> +					set_conf_val(OPT_B, B_SIZE, 1 << tmp);
>  					blflag = 1;

This is a collateral variable set when blog is set. If we have to parse the
blog again, it means similar code would be needed.

> -					set_conf_val(OPT_B, B_SIZE, blocksize);
>  					break;
>  				case B_SIZE:
> -					blocksize = parse_conf_val(OPT_B,
> -								   B_SIZE,
> -								   value);
> -					blocklog = libxfs_highbit32(blocksize);
> +					tmp = parse_conf_val(OPT_B, B_SIZE,
> +							     value);
> +					set_conf_val(OPT_B, B_LOG,
> +						libxfs_highbit32(tmp));
>  					bsflag = 1;

Likewise for bsflag.

> -					set_conf_val(OPT_B, B_LOG, blocklog);
>  					break;
>  				default:
>  					unknown('b', value);

What I'm questioning is whether or not it makes sense instead for parse_conf_val()
to return void and do all this meddling for us, this would require stuffing any
collateral variables into a struct and passing that to parse_conf_val and using
it on main() as well.

  Luis