Re: [PATCH 4/5] mkfs: hook up suboption parsing to ini files

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Oct 15, 2020 at 02:29:24PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Now we have the config file parsing hooked up and feeding in
> parameters to mkfs, wire the parameters up to the existing CLI
> option parsing functions. THis gives the config file exactly the
> same capabilities and constraints as the command line option
> specification.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  mkfs/xfs_mkfs.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 80 insertions(+), 15 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 99ce0dc48d3b..370ac6194e2f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -143,6 +143,13 @@ enum {
>   * name MANDATORY
>   *   Name is a single char, e.g., for '-d file', name is 'd'.
>   *
> + * ini_name MANDATORY
> + *   Name is a string, not longer than MAX_INI_NAME_LEN, that is used as the
> + *   section name for this option set in INI format config files. The only
> + *   option set this is not required for is the command line config file
> + *   specification options, everything else must be configurable via config
> + *   files.

I don't understand this last sentence.

OH, I get it:

"This field is required to connect each opt_params (that is to say, each
option class) to a section in the config file."

(and maybe a note in copts that config files cannot specify other config
files, so ini_name is not necessary there.)

> + *
>   * subopts MANDATORY
>   *   Subopts is a list of strings naming suboptions. In the example above,
>   *   it would contain "file". The last entry of this list has to be NULL.
> @@ -201,6 +208,8 @@ enum {
>   */
>  struct opt_params {
>  	const char	name;
> +#define MAX_INI_NAME_LEN	32
> +	const char	ini_name[MAX_INI_NAME_LEN];

How about "ini_section" ?

--D

>  	const char	*subopts[MAX_SUBOPTS];
>  
>  	struct subopt_param {
> @@ -228,6 +237,7 @@ static struct opt_params sopts;
>  
>  static struct opt_params bopts = {
>  	.name = 'b',
> +	.ini_name = "block",
>  	.subopts = {
>  		[B_SIZE] = "size",
>  	},
> @@ -267,6 +277,7 @@ static struct opt_params copts = {
>  
>  static struct opt_params dopts = {
>  	.name = 'd',
> +	.ini_name = "data",
>  	.subopts = {
>  		[D_AGCOUNT] = "agcount",
>  		[D_FILE] = "file",
> @@ -411,6 +422,7 @@ static struct opt_params dopts = {
>  
>  static struct opt_params iopts = {
>  	.name = 'i',
> +	.ini_name = "inode",
>  	.subopts = {
>  		[I_ALIGN] = "align",
>  		[I_MAXPCT] = "maxpct",
> @@ -472,6 +484,7 @@ static struct opt_params iopts = {
>  
>  static struct opt_params lopts = {
>  	.name = 'l',
> +	.ini_name = "log",
>  	.subopts = {
>  		[L_AGNUM] = "agnum",
>  		[L_INTERNAL] = "internal",
> @@ -571,6 +584,7 @@ static struct opt_params lopts = {
>  
>  static struct opt_params nopts = {
>  	.name = 'n',
> +	.ini_name = "naming",
>  	.subopts = {
>  		[N_SIZE] = "size",
>  		[N_VERSION] = "version",
> @@ -602,6 +616,7 @@ static struct opt_params nopts = {
>  
>  static struct opt_params ropts = {
>  	.name = 'r',
> +	.ini_name = "realtime",
>  	.subopts = {
>  		[R_EXTSIZE] = "extsize",
>  		[R_SIZE] = "size",
> @@ -652,6 +667,7 @@ static struct opt_params ropts = {
>  
>  static struct opt_params sopts = {
>  	.name = 's',
> +	.ini_name = "sector",
>  	.subopts = {
>  		[S_SIZE] = "size",
>  		[S_SECTSIZE] = "sectsize",
> @@ -682,6 +698,7 @@ static struct opt_params sopts = {
>  
>  static struct opt_params mopts = {
>  	.name = 'm',
> +	.ini_name = "metadata",
>  	.subopts = {
>  		[M_CRC] = "crc",
>  		[M_FINOBT] = "finobt",
> @@ -982,6 +999,17 @@ unknown(
>  	usage();
>  }
>  
> +static void
> +invalid_cfgfile_opt(
> +	const char	*filename,
> +	const char	*section,
> +	const char	*name,
> +	const char	*value)
> +{
> +	fprintf(stderr, _("%s: invalid config file option: [%s]:%s=%s\n"),
> +		filename, section, name, value);
> +}
> +
>  static void
>  check_device_type(
>  	const char	*name,
> @@ -1696,23 +1724,22 @@ sector_opts_parser(
>  }
>  
>  static struct subopts {
> -	char		opt;
>  	struct opt_params *opts;
>  	int		(*parser)(struct opt_params	*opts,
>  				  int			subopt,
>  				  const char		*value,
>  				  struct cli_params	*cli);
>  } subopt_tab[] = {
> -	{ 'b', &bopts, block_opts_parser },
> -	{ 'c', &copts, cfgfile_opts_parser },
> -	{ 'd', &dopts, data_opts_parser },
> -	{ 'i', &iopts, inode_opts_parser },
> -	{ 'l', &lopts, log_opts_parser },
> -	{ 'm', &mopts, meta_opts_parser },
> -	{ 'n', &nopts, naming_opts_parser },
> -	{ 'r', &ropts, rtdev_opts_parser },
> -	{ 's', &sopts, sector_opts_parser },
> -	{ '\0', NULL, NULL },
> +	{ &bopts, block_opts_parser },
> +	{ &copts, cfgfile_opts_parser },
> +	{ &dopts, data_opts_parser },
> +	{ &iopts, inode_opts_parser },
> +	{ &lopts, log_opts_parser },
> +	{ &mopts, meta_opts_parser },
> +	{ &nopts, naming_opts_parser },
> +	{ &ropts, rtdev_opts_parser },
> +	{ &sopts, sector_opts_parser },
> +	{ NULL, NULL },
>  };
>  
>  static void
> @@ -1726,7 +1753,7 @@ parse_subopts(
>  	int		ret = 0;
>  
>  	while (sop->opts) {
> -		if (sop->opt == opt)
> +		if (opt && sop->opts->name == opt)
>  			break;
>  		sop++;
>  	}
> @@ -1749,6 +1776,45 @@ parse_subopts(
>  	}
>  }
>  
> +static bool
> +parse_cfgopt(
> +	const char	*section,
> +	const char	*name,
> +	const char	*value,
> +	struct cli_params *cli)
> +{
> +	struct subopts	*sop = &subopt_tab[0];
> +	char		**subopts;
> +	int		ret = 0;
> +	int		i;
> +
> +	while (sop->opts) {
> +		if (sop->opts->ini_name[0] != '\0' &&
> +		    strcasecmp(section, sop->opts->ini_name) == 0)
> +			break;
> +		sop++;
> +	}
> +
> +	/* Config files with unknown sections get caught here. */
> +	if (!sop->opts)
> +		goto invalid_opt;
> +
> +	subopts = (char **)sop->opts->subopts;
> +	for (i = 0; i < MAX_SUBOPTS; i++) {
> +		if (!subopts[i])
> +			break;
> +		if (strcasecmp(name, subopts[i]) == 0) {
> +			ret = (sop->parser)(sop->opts, i, value, cli);
> +			if (ret)
> +				goto invalid_opt;
> +			return true;
> +		}
> +	}
> +invalid_opt:
> +	invalid_cfgfile_opt(cli->cfgfile, section, name, value);
> +	return false;
> +}
> +
>  static void
>  validate_sectorsize(
>  	struct mkfs_params	*cfg,
> @@ -3630,9 +3696,8 @@ cfgfile_parse_ini(
>  {
>  	struct cli_params	*cli = user;
>  
> -	fprintf(stderr, "Ini debug: file %s, section %s, name %s, value %s\n",
> -		cli->cfgfile, section, name, value);
> -
> +	if (!parse_cfgopt(section, name, value, cli))
> +		return 0;
>  	return 1;
>  }
>  
> -- 
> 2.28.0
>