[v2,5/5] mkfs.xfs: add configuration file parsing support using our own parser

Commit Message

Luis Chamberlain May 17, 2018, 7:27 p.m. UTC

You may want to stick to specific set of configuration options when
creating filesystems with mkfs.xfs -- sometimes due to pure technical
reasons, but some other times to ensure systems remain compatible as
new features are introduced with older kernels, or if you always want
to take advantage of some new feature which would otherwise typically
be disruptive.

This adds support for parsing a configuration file to override defaults
parameters to be used for mkfs.xfs.

We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
different configuration files, if none is specified we look for the
default configuration file, /etc/mkfs.xfs.d/default. You can override
with -c.  For instance, if you specify:

	mkfs.xfs -c experimental -f /dev/loop0

The file /etc/mkfs.xfs.d/experimental will be used as your configuration
file. If you really need to override the full path of the configuration
file you may use the MKFS_XFS_CONFIG environment variable.

To verify what configuration file is used on a system use the typical:

  mkfs.xfs -N

There is only a subset of options allowed to be set on the configuration
file, and currently only 1 or 0 are acceptable values. The default
parameters you can override on a configuration file and their current
built-in default settings are:

[data]
noalign=0

[inode]
align=1
projid32bit=1
sparse=0

[log]
lazy-count=1

[metadata]
crc=1
finobt=1
rmapbt=0
reflink=0

[naming]
ftype=1

[rtdev]
noalign=0

Signed-off-by: Luis R. Rodriguez <[e-mail]>
---
 include/builddefs.in   |   2 +
 man/man5/mkfs.xfs.d.5  | 121 ++++++++++
 man/man8/mkfs.xfs.8    |  26 +++
 mkfs/Makefile          |   2 +-
 mkfs/xfs_mkfs.c        |  47 +++-
 mkfs/xfs_mkfs_common.h |  12 +
 mkfs/xfs_mkfs_config.c | 591 +++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/xfs_mkfs_config.h |  11 +
 8 files changed, 801 insertions(+), 11 deletions(-)
 create mode 100644 man/man5/mkfs.xfs.d.5
 create mode 100644 mkfs/xfs_mkfs_config.c
 create mode 100644 mkfs/xfs_mkfs_config.h

Comments

Darrick J. Wong May 17, 2018, 9:31 p.m. UTC | #1

On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
> 
> This adds support for parsing a configuration file to override defaults
> parameters to be used for mkfs.xfs.
> 
> We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
> different configuration files, if none is specified we look for the
> default configuration file, /etc/mkfs.xfs.d/default. You can override
> with -c.  For instance, if you specify:
> 
> 	mkfs.xfs -c experimental -f /dev/loop0
> 
> The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> file. If you really need to override the full path of the configuration
> file you may use the MKFS_XFS_CONFIG environment variable.
> 
> To verify what configuration file is used on a system use the typical:
> 
>   mkfs.xfs -N
> 
> There is only a subset of options allowed to be set on the configuration
> file, and currently only 1 or 0 are acceptable values. The default
> parameters you can override on a configuration file and their current
> built-in default settings are:
> 
> [data]
> noalign=0
> 
> [inode]
> align=1
> projid32bit=1
> sparse=0
> 
> [log]
> lazy-count=1
> 
> [metadata]
> crc=1
> finobt=1
> rmapbt=0
> reflink=0
> 
> [naming]
> ftype=1
> 
> [rtdev]
> noalign=0

Separate patch, but should there be a way to spit out the current mkfs
settings as a config file?

> Signed-off-by: Luis R. Rodriguez <[e-mail]>
> ---
>  include/builddefs.in   |   2 +
>  man/man5/mkfs.xfs.d.5  | 121 ++++++++++
>  man/man8/mkfs.xfs.8    |  26 +++
>  mkfs/Makefile          |   2 +-
>  mkfs/xfs_mkfs.c        |  47 +++-
>  mkfs/xfs_mkfs_common.h |  12 +
>  mkfs/xfs_mkfs_config.c | 591 +++++++++++++++++++++++++++++++++++++++++++++++++
>  mkfs/xfs_mkfs_config.h |  11 +
>  8 files changed, 801 insertions(+), 11 deletions(-)
>  create mode 100644 man/man5/mkfs.xfs.d.5
>  create mode 100644 mkfs/xfs_mkfs_config.c
>  create mode 100644 mkfs/xfs_mkfs_config.h
> 
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 8aac06cf90dc..e1ee9f7ba086 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -62,6 +62,7 @@ PKG_LIB_DIR	= @libdir@@libdirsuffix@
>  PKG_INC_DIR	= @includedir@/xfs
>  DK_INC_DIR	= @includedir@/disk
>  PKG_MAN_DIR	= @mandir@
> +PKG_ETC_DIR	= @sysconfdir@
>  PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
>  PKG_LOCALE_DIR	= @datadir@/locale
>  
> @@ -196,6 +197,7 @@ endif
>  
>  GCFLAGS = $(DEBUG) \
>  	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
> +	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
>  	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
>  
>  ifeq ($(ENABLE_GETTEXT),yes)
> diff --git a/man/man5/mkfs.xfs.d.5 b/man/man5/mkfs.xfs.d.5
> new file mode 100644
> index 000000000000..5424f730a2e5
> --- /dev/null
> +++ b/man/man5/mkfs.xfs.d.5
> @@ -0,0 +1,121 @@
> +.TH mkfs.xfs.d 5
> +.SH NAME
> +mkfs.xfs.d \- mkfs.xfs configuration directory
> +.SH DESCRIPTION
> +.B mkfs.xfs (8)
> +uses a set of initial default parameters for configuration. These defaults
> +are conservatively decided by the community as xfsprogs and features for XFS
> +in the kernel advance. One can override these defaults on the

I'm not sure what the sentence "These defaults...in the kernel advance."
means... is this close to what you meant?:

"The builtin mkfs defaults are decided by the XFS community.  New features
are only enabled by default when the community consider it stable."

> +.B mkfs.xfs (8)
> +command line, but there are cases where it is desirable for sensible defaults
> +to always be specified by the system where

"...where it is desirable for the distro or the system administrator to
establish their own supported defaults in a uniform manner." ?

> +.B mkfs.xfs (8)
> +runs on. This may desirable for example on systems with old kernels where the
> +built-in default parameters on
> +.B mkfs.xfs (8)
> +may not be able to create a filesystem which the old kernel supports and it

"...where the built-in default mkfs.xfs parameters create a filesystem
that is not supported by the old kernel."

> +would be unclear what parameters are needed to produce a compatible filesystem.
> +Overriding
> +.B mkfs.xfs (8)
> +built-in defaults may also be desirable if you have a series of systems with
> +different kernels and want to be able to create filesystems which all systems
> +are able to support properly.
> +.PP
> +The XFS configuration directory
> +.B mkfs.xfs.d (5)
> +can be used to define different configuration files which can be used to
> +override the built-in default parameters by
> +.B mkfs.xfs (8).
> +Different configuration files are supported, the default
> +configuration file,
> +.I /etc/mkfs.xfs.d/default

The "/etc" part of the path should be sysconfdir and replaced by the
build script to match whatever we're encoding into the mkfs.xfs binary.

> +, will be looked for first and if present will be used to override
> +.B mkfs.xfs (8)
> +built-in default parameters. You can override the configuration file by
> +specifying its name when using
> +.B mkfs.xfs (8)
> +by using the
> +.B -c
> +parameter. For example:
> +.I mkfs.xfs -c experimental -f /dev/sda1
> +will make
> +.B mkfs.xfs (8)
> +look for and use the configuration file
> +.I /etc/mkfs.xfs.d/experimental
> +to override
> +.B mkfs.xfs (8)
> +default parameters. If you need to override the full path for a configuration
> +file you can use the
> +.I MKFS_XFS_CONFIG
> +environment variable prior to calling
> +.B mkfs.xfs (8)
> +to define the
> +full path to the configuration file to be used.

Does the environment variable override -c, or the other way around?

> If you used the
> +.B -c
> +parameter or if you set the
> +.I MKFS_XFS_CONFIG
> +environment variable the configuration file must be present and should parse

"...must parse..."

> +correctly.
> +.PP
> +Parameters passed to to the

"...passed to the..."

> +.B mkfs.xfs (8)
> +command line always override any defaults set on the configuration file used.
> +.PP
> +.B mkfs.xfs (8)
> +will always describe what configuration file was used, if any
> +was used at all. To verify which configuration file would be used prior to
> +execution of
> +.B mkfs.xfs (8)
> +you can use
> +.I mkfs.xfs -N.
> +.PP
> +.SH DEFAULT PARAMETERS
> +Default parameters for
> +.B mkfs.xfs (8)
> +consists of a small subset of the parameters one can set with on the command
> +line. Currently all default parameters can only be either enabled or disabled,
> +you can set their value to 1 to enable or 0 to disable. Below we list the
> +different supported default parameters which can be defined on configuration
> +files, along with the current built-in setting.
> +.PP
> +.BI [data]
> +.br
> +.BI noalign=0
> +.PP
> +.BI [inode]
> +.br
> +.BI align=1
> +.br
> +.BI projid32bit=1
> +.br
> +.BI sparse=0
> +.PP
> +.BI [log]
> +.br
> +.BI lazy-count=1
> +.PP
> +.BI [metadata]
> +.br
> +.BI crc=1
> +.br
> +.BI finobt=1
> +.br
> +.BI rmapbt=0
> +.br
> +.BI reflink=0
> +.PP
> +.BI [naming]
> +.br
> +.BI ftype=1
> +.PP
> +.BI [rtdev]
> +.br
> +.BI noalign=0
> +.PP
> +.SH SEE ALSO
> +.BR mkfs.xfs (8),
> +.BR xfsctl (3),
> +.BR xfs_info (8),
> +.BR xfs_admin (8),
> +.BR xfsdump (8),
> +.BR xfsrestore (8).
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 4b8c78c37806..fadc4c589a8c 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -83,6 +83,26 @@ and
>  .B \-l internal \-l size=10m
>  are equivalent.
>  .PP
> +An optional XFS configuration file directory
> +.B mkfs.xfs.d (5)

%sysconfigdir%/mkfs.xfs.d ?

> +exists to help fine tune default parameters which can be used when calling
> +.B mkfs.xfs (8).
> +If present, the default file /etc/mkfs.xfs.d/default
> +will be used to override system build-in defaults. Refer to mkfs.xfs.d (5)
> +for a list of current defaults.
> +Command line arguments directly passed to
> +.B mkfs.xfs (8)
> +will always override parameters set in the configuration file.
> +You can override the configuration file used within the
> +.B mkfs.xfs.d (5)
> +directory by using the -c parameter. Alternatively
> +you can set and use the MKFS_XFS_CONFIG environment variable to override
> +the default full path of the configuration file which
> +.B mkfs.xfs (8)
> +looks for.
> +If you use -c the configuration file must be present under
> +.B mkfs.xfs.d (8).
> +.PP
>  In the descriptions below, sizes are given in sectors, bytes, blocks,
>  kilobytes, megabytes, gigabytes, etc.
>  Sizes are treated as hexadecimal if prefixed by 0x or 0X,
> @@ -123,6 +143,11 @@ Many feature options allow an optional argument of 0 or 1, to explicitly
>  disable or enable the functionality.
>  .SH OPTIONS
>  .TP
> +.BI \-c " configuration-file"
> +Override the configuration file used under the
> +.B mkfs.xfs.d
> +directory.
> +.TP
>  .BI \-b " block_size_options"
>  This option specifies the fundamental block size of the filesystem.
>  The valid
> @@ -923,6 +948,7 @@ Prints the version number and exits.
>  .SH SEE ALSO
>  .BR xfs (5),
>  .BR mkfs (8),
> +.BR mkfs.xfs.d (5),
>  .BR mount (8),
>  .BR xfs_info (8),
>  .BR xfs_admin (8).
> diff --git a/mkfs/Makefile b/mkfs/Makefile
> index c84f9b6ae63b..7906b1d4e67d 100644
> --- a/mkfs/Makefile
> +++ b/mkfs/Makefile
> @@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs
>  LTCOMMAND = mkfs.xfs
>  
>  HFILES =
> -CFILES = proto.c xfs_mkfs.c
> +CFILES = proto.c xfs_mkfs.c xfs_mkfs_config.c
>  
>  LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
>  	$(LIBUUID)
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index cb549be89835..aaf3f71a93cf 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -21,6 +21,7 @@
>  #include "xfs_multidisk.h"
>  #include "libxcmd.h"
>  #include "xfs_mkfs_common.h"
> +#include "xfs_mkfs_config.h"
>  #include "xfs_mkfs_cli.h"
>  
>  
> @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
>  	struct mkfs_params	*cfg,
>  	char			*dfile,
>  	char			*logfile,
> -	char			*rtfile)
> +	char			*rtfile,
> +	struct mkfs_default_params *dft)
>  {
>  	struct sb_feat_args	*fp = &cfg->sb_feat;
>  
>  	printf(_(
> +"config-file=%-22s\n"
>  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
>  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
>  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
>  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
>  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
>  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
>  		dfile, cfg->inodesize, (long long)cfg->agcount,
>  			(long long)cfg->agsize,
>  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,

FWIW all this geometry printing was refactored into libfrog.

Though, you already print where we got the configuration data, so just
print dft->config_file there.

> @@ -3665,6 +3669,7 @@ rewrite_secondary_superblocks(
>  /* build time defaults */
>  struct mkfs_default_params	built_in_dft = {
>  	.type = DEFAULTS_BUILTIN,
> +	.config_file = MKFS_XFS_CONF_DIR "default",
>  	.sectorsize = XFS_MIN_SECTORSIZE,
>  	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
>  	.sb_feat = {
> @@ -3714,6 +3719,13 @@ static void process_defaults(
>  	struct mkfs_default_params	*dft,
>  	struct cli_params		*cli)
>  {
> +	int ret;
> +
> +	ret = parse_config_init(dft);
> +
> +	if (ret && dft->type != DEFAULTS_BUILTIN)
> +		usage();
> +
>  	install_defaults(dft, cli);
>  }
>  
> @@ -3750,6 +3762,8 @@ main(
>  	};
>  	struct mkfs_params	cfg = {};
>  	struct mkfs_default_params	dft;
> +	char *tmp_config;
> +	char custom_config[PATH_MAX];
>  
>  	reset_defaults_and_cli(&dft, &cli);
>  
> @@ -3760,21 +3774,36 @@ main(
>  	textdomain(PACKAGE);
>  
>  	/*
> -	 * TODO: Sourcing defaults from a config file
> -	 *
>  	 * Before anything else, see if there's a config file with different
> -	 * defaults. If a file exists in <package location>, read in the new
> +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
>  	 * default values and overwrite them in the &dft structure. This way the
>  	 * new defaults will apply before we parse the CLI, and the CLI will
>  	 * still be able to override them. When more than one source is
>  	 * implemented, emit a message to indicate where the defaults being
>  	 * used came from.
>  	 */
> +	tmp_config = getenv("MKFS_XFS_CONFIG");
> +	if (tmp_config != NULL) {
> +		dft.config_file = tmp_config;
> +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> +	}
>  
>  	process_defaults(&dft, &cli);
>  
> -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
>  		switch (c) {
> +		case 'c':
> +			reset_defaults_and_cli(&dft, &cli);
> +
> +			memset(custom_config, 0, sizeof(custom_config));
> +			snprintf(custom_config, sizeof(custom_config), "%s%s",
> +				 MKFS_XFS_CONF_DIR, optarg);
> +			dft.config_file = custom_config;
> +			dft.type = DEFAULTS_TYPE_CONFIG;

Ok, so we parse $MKFS_XFS_CONFIG but then if the user specifies -c we
reset all that and parse that config file.

Just for fun I decided to run:

$ cat > /tmp/butts.lol
[gugugugug]
bye = 1
[metadata]
hork = 1
$ ./build-x64/mkfs/mkfs.xfs -c ../../../../../../../tmp/butts.lol -N /dev/sda
Error parsine line: bye=1

I think we need to error out on unrecognized section names:

"Unrecognized section name 'gugugugug'."

And probably report the section name for unrecognized keys:

"Unrecognized name 'metadata.hork"."

> +
> +			process_defaults(&dft, &cli);
> +
> +			break;
>  		case 'C':
>  		case 'f':
>  			force_overwrite = 1;
> @@ -3823,10 +3852,8 @@ main(
>  	} else
>  		dfile = xi.dname;
>  
> -	/*
> -	 * printf(_("Default configuration sourced from %s\n"),
> -	 *	  default_type_str(dft.type));
> -	 */
> +	printf(_("Default configuration sourced from %s\n"),
> +	       default_type_str(dft.type));
>  
>  	protostring = setup_proto(protofile);
>  
> @@ -3902,7 +3929,7 @@ main(
>  	calculate_log_size(&cfg, &cli, mp);
>  
>  	if (!quiet || dry_run) {
> -		print_mkfs_cfg(&cfg, dfile, logfile, rtfile);
> +		print_mkfs_cfg(&cfg, dfile, logfile, rtfile, &dft);
>  		if (dry_run)
>  			exit(0);
>  	}
> diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
> index d867ab377185..028bbf96017f 100644
> --- a/mkfs/xfs_mkfs_common.h
> +++ b/mkfs/xfs_mkfs_common.h
> @@ -46,9 +46,20 @@ struct sb_feat_args {
>   * These are the different possibilities by which you can end up parsing
>   * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
>   * file parsed and we are using the built-in defaults on this code.
> + * DEFAULTS_CONFIG means the default configuration file was found and used.
> + * DEFAULTS_TYPE_CONFIG means the user asked for a custom configuration type
> + * and it was used, this means the default directory for mkfs.xfs
> + * configuration files was used to look for the type specified. If you need
> + * to override the default mkfs.xfs directory for configuration file you can
> + * use the MKFS_XFS_CONFIG environment variable to set the absolute path to
> + * your custom configuration file, when this is set the type is set to
> + * DEFAULTS_ENVIRONMENT_CONFIG.
>   */
>  enum default_params_type {
>  	DEFAULTS_BUILTIN = 0,
> +	DEFAULTS_CONFIG,
> +	DEFAULTS_ENVIRONMENT_CONFIG,
> +	DEFAULTS_TYPE_CONFIG,
>  };
>  
>  /*
> @@ -61,6 +72,7 @@ enum default_params_type {
>   */
>  struct mkfs_default_params {
>  	enum default_params_type type; /* where the defaults came from */
> +	const char *config_file;
>  
>  	int	sectorsize;
>  	int	blocksize;
> diff --git a/mkfs/xfs_mkfs_config.c b/mkfs/xfs_mkfs_config.c
> new file mode 100644
> index 000000000000..d554638982ff
> --- /dev/null
> +++ b/mkfs/xfs_mkfs_config.c
> @@ -0,0 +1,591 @@
> +/*
> + * Copyright (c) 2018 Luis R. Rodriguez <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include "xfs_mkfs_common.h"
> +#include "xfs_mkfs_config.h"
> +
> +#define CONFIG_MAX_KEY		1024
> +#define CONFIG_MAX_VALUE	PATH_MAX

That's a pretty big size considering we only allow 0 and 1...

> +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> +
> +/*
> + * Enums for each configuration option. All these currently match the CLI
> + * parameters for now but this may change later, so we keep all this code
> + * and definitions separate. The rules for configuration parameters may also
> + * differ.
> + */

So... I think these enums should be shared between cli and config file
processing, or if the config file parser retains its own private
definitions then it should only have the options that we support in the
config file.  It's weird how things like data.sunit are specified here
but the config file doesn't actually do anything with it?

$ cat > /tmp/butts.lol
[data]
sunit = 5
$ ./build-x64/mkfs/mkfs.xfs -c ../../../tmp/butts.lol -N /dev/sda | grep sunit
         =                       sunit=0      swidth=0 blks

That really ought to error out, since we don't support setting sunit?

> +
> +enum {
> +	B_SIZE = 0,
> +	B_MAX_OPTS,
> +};
> +
> +enum {
> +	D_AGCOUNT = 0,
> +	D_FILE,
> +	D_NAME,
> +	D_SIZE,
> +	D_SUNIT,
> +	D_SWIDTH,
> +	D_AGSIZE,
> +	D_SU,
> +	D_SW,
> +	D_SECTSIZE,
> +	D_NOALIGN,
> +	D_RTINHERIT,
> +	D_PROJINHERIT,
> +	D_EXTSZINHERIT,
> +	D_COWEXTSIZE,
> +	D_MAX_OPTS,
> +};
> +
> +enum {
> +	I_ALIGN = 0,
> +	I_MAXPCT,
> +	I_PERBLOCK,
> +	I_SIZE,
> +	I_ATTR,
> +	I_PROJID32BIT,
> +	I_SPINODES,
> +	I_MAX_OPTS,
> +};
> +
> +enum {
> +	L_AGNUM = 0,
> +	L_INTERNAL,
> +	L_SIZE,
> +	L_VERSION,
> +	L_SUNIT,
> +	L_SU,
> +	L_DEV,
> +	L_SECTSIZE,
> +	L_FILE,
> +	L_NAME,
> +	L_LAZYSBCNTR,
> +	L_MAX_OPTS,
> +};
> +
> +enum {
> +	N_SIZE = 0,
> +	N_VERSION,
> +	N_FTYPE,
> +	N_MAX_OPTS,
> +};
> +
> +enum {
> +	R_EXTSIZE = 0,
> +	R_SIZE,
> +	R_DEV,
> +	R_FILE,
> +	R_NAME,
> +	R_NOALIGN,
> +	R_MAX_OPTS,
> +};
> +
> +enum {
> +	S_SIZE = 0,
> +	S_SECTSIZE,
> +	S_MAX_OPTS,
> +};
> +
> +enum {
> +	M_CRC = 0,
> +	M_FINOBT,
> +	M_UUID,
> +	M_RMAPBT,
> +	M_REFLINK,
> +	M_MAX_OPTS,
> +};
> +
> +/* Just define the max options array size manually right now */
> +#define MAX_SUBOPTS	D_MAX_OPTS
> +
> +enum mkfs_config_section {
> +	MKFS_CONFIG_SECTION_BLOCK = 0,
> +	MKFS_CONFIG_SECTION_DATA,
> +	MKFS_CONFIG_SECTION_INODE,
> +	MKFS_CONFIG_SECTION_LOG,
> +	MKFS_CONFIG_SECTION_METADATA,
> +	MKFS_CONFIG_SECTION_NAMING,
> +	MKFS_CONFIG_SECTION_RTDEV,
> +	MKFS_CONFIG_SECTION_SECTOR,
> +
> +	MKFS_CONFIG_SECTION_INVALID,
> +};
> +
> +struct opt_params {
> +	const enum mkfs_config_section section;
> +	const char	*subopts[MAX_SUBOPTS];
> +};
> +
> +extern struct opt_params config_sopts;
> +
> +struct opt_params config_bopts = {
> +	.section = 'b',
> +	.subopts = {
> +		[B_SIZE] = "size",
> +	},
> +};
> +
> +struct opt_params config_dopts = {
> +	.section = 'd',
> +	.subopts = {
> +		[D_AGCOUNT] = "agcount",
> +		[D_FILE] = "file",
> +		[D_NAME] = "name",
> +		[D_SIZE] = "size",
> +		[D_SUNIT] = "sunit",
> +		[D_SWIDTH] = "swidth",
> +		[D_AGSIZE] = "agsize",
> +		[D_SU] = "su",
> +		[D_SW] = "sw",
> +		[D_SECTSIZE] = "sectsize",
> +		[D_NOALIGN] = "noalign",
> +		[D_RTINHERIT] = "rtinherit",
> +		[D_PROJINHERIT] = "projinherit",
> +		[D_EXTSZINHERIT] = "extszinherit",
> +		[D_COWEXTSIZE] = "cowextsize",
> +	},
> +};
> +
> +struct opt_params config_iopts = {
> +	.section = 'i',
> +	.subopts = {
> +		[I_ALIGN] = "align",
> +		[I_MAXPCT] = "maxpct",
> +		[I_PERBLOCK] = "perblock",
> +		[I_SIZE] = "size",
> +		[I_ATTR] = "attr",
> +		[I_PROJID32BIT] = "projid32bit",
> +		[I_SPINODES] = "sparse",
> +	},
> +};
> +
> +struct opt_params config_lopts = {
> +	.section = 'l',
> +	.subopts = {
> +		[L_AGNUM] = "agnum",
> +		[L_INTERNAL] = "internal",
> +		[L_SIZE] = "size",
> +		[L_VERSION] = "version",
> +		[L_SUNIT] = "sunit",
> +		[L_SU] = "su",
> +		[L_DEV] = "logdev",
> +		[L_SECTSIZE] = "sectsize",
> +		[L_FILE] = "file",
> +		[L_NAME] = "name",
> +		[L_LAZYSBCNTR] = "lazy-count",
> +	},
> +};
> +
> +struct opt_params config_nopts = {
> +	.section = 'n',
> +	.subopts = {
> +		[N_SIZE] = "size",
> +		[N_VERSION] = "version",
> +		[N_FTYPE] = "ftype",
> +	},
> +};
> +
> +struct opt_params config_ropts = {
> +	.section = 'r',
> +	.subopts = {
> +		[R_EXTSIZE] = "extsize",
> +		[R_SIZE] = "size",
> +		[R_DEV] = "rtdev",
> +		[R_FILE] = "file",
> +		[R_NAME] = "name",
> +		[R_NOALIGN] = "noalign",
> +	},
> +};
> +
> +struct opt_params config_sopts = {
> +	.section = 's',
> +	.subopts = {
> +		[S_SIZE] = "size",
> +		[S_SECTSIZE] = "sectsize",
> +	},
> +};
> +
> +struct opt_params config_mopts = {
> +	.section = 'm',
> +	.subopts = {
> +		[M_CRC] = "crc",
> +		[M_FINOBT] = "finobt",
> +		[M_UUID] = "uuid",
> +		[M_RMAPBT] = "rmapbt",
> +		[M_REFLINK] = "reflink",
> +	},
> +};
> +
> +const char *default_type_str(enum default_params_type type)

const char *
default_type_str(
	enum default_params_type	type)
{
	...
}

Please follow regular xfs formatting conventions.

> +{
> +	switch (type) {
> +	case DEFAULTS_BUILTIN:
> +		return _("package built-in definitions");
> +	case DEFAULTS_CONFIG:
> +		return _("default configuration system file");
> +	case DEFAULTS_ENVIRONMENT_CONFIG:
> +		return _("custom configuration file set by environment");
> +	case DEFAULTS_TYPE_CONFIG:
> +		return _("custom configuration type file");
> +	}
> +	return _("Unkown\n");

"Unknown"

> +}
> +
> +static int block_config_parser(struct mkfs_default_params *dft, int subopt,
> +			       bool value)
> +{
> +	return -EINVAL;
> +}
> +
> +static int data_config_parser(struct mkfs_default_params *dft, int subopt,
> +			      bool value)
> +{
> +	switch (subopt) {
> +	case D_NOALIGN:
> +		dft->sb_feat.nodalign = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int inode_config_parser(struct mkfs_default_params *dft, int subopt,
> +			       bool value)
> +{
> +	switch (subopt) {
> +	case I_ALIGN:
> +		dft->sb_feat.inode_align = value;
> +		break;
> +	case I_PROJID32BIT:
> +		dft->sb_feat.projid32bit = value;
> +		break;
> +	case I_SPINODES:
> +		dft->sb_feat.spinodes = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int log_config_parser(struct mkfs_default_params *dft, int subopt,
> +			     bool value)
> +{
> +	switch (subopt) {
> +	case L_LAZYSBCNTR:
> +		dft->sb_feat.lazy_sb_counters = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int meta_config_parser(struct mkfs_default_params *dft, int subopt,
> +			      bool value)
> +{
> +	switch (subopt) {
> +	case M_CRC:
> +		dft->sb_feat.crcs_enabled = value;
> +		if (dft->sb_feat.crcs_enabled)
> +			dft->sb_feat.dirftype = true;
> +		break;
> +	case M_FINOBT:
> +		dft->sb_feat.finobt = value;
> +		break;
> +	case M_RMAPBT:
> +		dft->sb_feat.rmapbt = value;
> +		break;
> +	case M_REFLINK:
> +		dft->sb_feat.reflink = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int naming_config_parser(struct mkfs_default_params *dft, int subopt,
> +				bool value)
> +{
> +	switch (subopt) {
> +	case N_FTYPE:
> +		dft->sb_feat.dirftype = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int rtdev_config_parser(struct mkfs_default_params *dft, int subopt,
> +			       bool value)
> +{
> +	switch (subopt) {
> +	case R_NOALIGN:
> +		dft->sb_feat.nortalign = value;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int sector_config_parser(struct mkfs_default_params *dft, int subopt,
> +				bool value)
> +{
> +	return -EINVAL;
> +}
> +
> +struct config_subopts {
> +	enum mkfs_config_section	section;
> +	struct opt_params		*opts;
> +	int (*config_parser)(struct mkfs_default_params *dft,
> +			     int subop, bool value);
> +} config_subopt_tab[] = {
> +	{ MKFS_CONFIG_SECTION_BLOCK, &config_bopts, block_config_parser },
> +	{ MKFS_CONFIG_SECTION_DATA, &config_dopts, data_config_parser },
> +	{ MKFS_CONFIG_SECTION_INODE, &config_iopts, inode_config_parser },
> +	{ MKFS_CONFIG_SECTION_LOG, &config_lopts, log_config_parser },
> +	{ MKFS_CONFIG_SECTION_METADATA, &config_mopts, meta_config_parser },
> +	{ MKFS_CONFIG_SECTION_NAMING, &config_nopts, naming_config_parser },
> +	{ MKFS_CONFIG_SECTION_RTDEV, &config_ropts, rtdev_config_parser },
> +	{ MKFS_CONFIG_SECTION_SECTOR, &config_sopts, sector_config_parser },
> +	{ '\0', NULL },
> +};
> +
> +static int
> +parse_config_subopts(
> +	enum mkfs_config_section	section,
> +	bool				value,
> +	char				*line,
> +	struct mkfs_default_params	*dft)
> +{
> +	struct config_subopts *sop = &config_subopt_tab[0];
> +	char	**subopts = (char **)sop->opts->subopts;
> +	int	subopt;
> +	char *val;
> +
> +	while (sop->opts) {
> +		if (sop->section == section)
> +			break;
> +		sop++;
> +	}
> +
> +	/* should never happen */
> +	if (!sop->opts)
> +		return -EINVAL;
> +
> +	subopts = (char **)sop->opts->subopts;
> +	subopt = getsubopt(&line, subopts, &val);
> +	return (sop->config_parser)(dft, subopt, value);
> +}
> +
> +static enum mkfs_config_section get_config_section(const char *buffer)
> +{
> +	if (strncmp("block", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_BLOCK;
> +	if (strncmp("data", buffer, 4) == 0)
> +		return MKFS_CONFIG_SECTION_DATA;
> +	if (strncmp("inode", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_INODE;
> +	if (strncmp("log", buffer, 3) == 0)
> +		return MKFS_CONFIG_SECTION_LOG;
> +	if (strncmp("metadata", buffer, 8) == 0)
> +		return MKFS_CONFIG_SECTION_METADATA;
> +	if (strncmp("naming", buffer, 6) == 0)
> +		return MKFS_CONFIG_SECTION_NAMING;
> +	if (strncmp("rtdev", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_RTDEV;
> +	if (strncmp("sector", buffer, 6) == 0)
> +		return MKFS_CONFIG_SECTION_SECTOR;
> +
> +	return MKFS_CONFIG_SECTION_INVALID;
> +}
> +
> +static int mkfs_check_config_file_limits(const char *filename,
> +					 const struct stat *sb,
> +					 unsigned int max_entries)
> +{
> +	unsigned long long size_limit;
> +
> +	size_limit = CONFIG_MAX_BUFFER * max_entries;
> +
> +	/*
> +	 * Detect wrap around -- if max_entries * size_limit goves over
> +	 * unsigned long long we detect that there. Note some libraries,
> +	 * like libiniconfig, only groks max INT_MAX entries anyway, so
> +	 * if we ever use a library for parsing we'd be restricted to that
> +	 * limit.
> +	 */
> +	if (size_limit < max_entries || max_entries > INT_MAX)
> +		return -E2BIG;
> +
> +	if (sb->st_size > size_limit) {
> +		fprintf(stderr,
> +			"File %s is too big! File size limit: %llu\n",
> +			filename, size_limit);
> +		return -E2BIG;
> +	}

Not sure why we care about the file size?  If the config file
respecifies options a trillion times then let it.

> +
> +	return 0;
> +}
> +
> +enum parse_line_type {
> +	PARSE_COMMENT = 0,
> +	PARSE_SECTION,
> +	PARSE_TAG_VALUE,
> +	PARSE_INVALID,
> +	PARSE_EOF,
> +};
> +
> +static int parse_line_section(const char *line, char *tag)
> +{
> +	return sscanf(line, " [%[^]]s]", tag);
> +}
> +
> +static int parse_line_tag_value(const char *line, char *tag,
> +				unsigned int *value)
> +{
> +	return sscanf(line, " %[^ \t=]"
> +		      " = "
> +		      "%u ",
> +		      tag, value);
> +}
> +
> +static enum parse_line_type parse_get_line_type(const char *line, char *tag,
> +						bool *value)
> +{
> +	int ret;
> +	unsigned int uint_value;
> +
> +	memset(tag, 0, 80);
> +
> +	if (strlen(line) < 2)
> +		return PARSE_INVALID;
> +
> +	if (line[0] == '#')
> +		return PARSE_COMMENT;
> +
> +	ret = parse_line_section(line, tag);
> +	if (ret == 1)
> +		return  PARSE_SECTION;
> +
> +	if (ret == EOF)
> +		return PARSE_EOF;
> +
> +	ret = parse_line_tag_value(line, tag, &uint_value);
> +	if (ret == 2) {
> +		if (uint_value != 1 && uint_value != 0)
> +			return -EINVAL;

If I set data.sunit = 5 this will return -EINVAL to the switch in
parse_config_init.  However, the switch lacks a default clause so it
silently drops invalid values and proceeds with the format.

> +
> +		*value = !!uint_value;
> +
> +		return PARSE_TAG_VALUE;
> +	}
> +
> +	if (ret == EOF)
> +		return PARSE_EOF;
> +
> +	return PARSE_INVALID;
> +}
> +
> +int parse_config_init(struct mkfs_default_params *dft)
> +{
> +	int ret;
> +	FILE *fp;
> +	struct stat sp;
> +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> +					    sizeof(struct config_subopts) - 1;
> +	char *p;
> +	char line[80], tag[80];

Waitaminute, didn't we set CONFIG_MAX_KEY = 1024 and CONFIG_MAX_VALUE
to PATH_MAX?

> +	bool value;
> +	enum parse_line_type parse_type;
> +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> +
> +	fp = fopen(dft->config_file, "r");
> +	if (!fp) {
> +		if (dft->type != DEFAULTS_BUILTIN) {

Why would we be parsing a config file if dft->type == DEFAULTS_BUILTIN?

> +			fprintf(stderr, _("could not open config file: %s\n"),
> +				dft->config_file);
> +			ret = -ENOENT;
> +		} else
> +			ret = 0;
> +		return ret;
> +	}
> +
> +	/*
> +	 * If we found a file without it being specified on the command line,
> +	 * it would be the default configuration file.
> +	 */
> +	if (dft->type == DEFAULTS_BUILTIN)
> +		dft->type = DEFAULTS_CONFIG;

Oh, I see, type == _BUILTIN really means "we picked up
/etc/mkfs.xfs.d/defaults"... shouldn't the caller set this?

> +
> +	ret = stat(dft->config_file, &sp);

fstat(fileno(fp), &sp); ?

> +	if (ret) {
> +		if (dft->type != DEFAULTS_BUILTIN)

Didn't we just set this to _CONFIG?

> +			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
> +				dft->config_file, strerror(ret));
> +		return ret;

We just leaked fp...

> +	}
> +
> +	if (!sp.st_size)
> +		return 0;

Can't we just keep going?  If the file size is zero we never execute the
loop.

> +	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
> +					    num_subopts_sections);
> +	if (ret)
> +		return ret;
> +
> +	while (!feof(fp)) {
> +		p = fgets(line, sizeof(line), fp);
> +		if (!p)
> +			continue;
> +
> +		parse_type = parse_get_line_type(line, tag, &value);
> +
> +		switch (parse_type) {
> +		case PARSE_COMMENT:
> +		case PARSE_INVALID:
> +		case PARSE_EOF:
> +			break;
> +		case PARSE_SECTION:
> +			section = get_config_section(tag);
> +			if (!section) {
> +				fprintf(stderr, _("Invalid section: %s\n"),
> +						tag);
> +				return -EINVAL;
> +			}
> +			break;
> +		case PARSE_TAG_VALUE:
> +			/* Trims white spaces */
> +			snprintf(line, sizeof(line), "%s=%u", tag, value);

So we reconstruct the line (without the whitespaces) so that we can use
getsubopt to map the tag to an integer value (e.g. D_AGCOUNT)... we
already isolated *tag, so can't we map it directly?

> +			ret = parse_config_subopts(section, value, line, dft);
> +			if (ret) {
> +				fprintf(stderr, _("Error parsine line: %s\n"),
> +						line);
> +				return ret;
> +			}
> +			break;

case -EINVAL?

--D

> +		}
> +	}
> +
> +	return 0;
> +}
> diff --git a/mkfs/xfs_mkfs_config.h b/mkfs/xfs_mkfs_config.h
> new file mode 100644
> index 000000000000..407d37fffb1a
> --- /dev/null
> +++ b/mkfs/xfs_mkfs_config.h
> @@ -0,0 +1,11 @@
> +#ifndef _XFS_MKFS_CONFIG_H
> +#define _XFS_MKFS_CONFIG_H
> +
> +#include "xfs_mkfs_common.h"
> +
> +#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> +
> +const char *default_type_str(enum default_params_type type);
> +int parse_config_init(struct mkfs_default_params *dft);
> +
> +#endif /* _XFS_MKFS_CONFIG_H */
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 18, 2018, 12:29 a.m. UTC | #2

On Thu, May 17, 2018 at 02:31:21PM -0700, Darrick J. Wong wrote:
> On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > You may want to stick to specific set of configuration options when
> > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > reasons, but some other times to ensure systems remain compatible as
> > new features are introduced with older kernels, or if you always want
> > to take advantage of some new feature which would otherwise typically
> > be disruptive.
> > 
> > This adds support for parsing a configuration file to override defaults
> > parameters to be used for mkfs.xfs.
> > 
> > We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for
> > different configuration files, if none is specified we look for the
> > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > with -c.  For instance, if you specify:
> > 
> > 	mkfs.xfs -c experimental -f /dev/loop0
> > 
> > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > file. If you really need to override the full path of the configuration
> > file you may use the MKFS_XFS_CONFIG environment variable.
> > 
> > To verify what configuration file is used on a system use the typical:
> > 
> >   mkfs.xfs -N
> > 
> > There is only a subset of options allowed to be set on the configuration
> > file, and currently only 1 or 0 are acceptable values. The default
> > parameters you can override on a configuration file and their current
> > built-in default settings are:
> > 
> > [data]
> > noalign=0
> > 
> > [inode]
> > align=1
> > projid32bit=1
> > sparse=0
> > 
> > [log]
> > lazy-count=1
> > 
> > [metadata]
> > crc=1
> > finobt=1
> > rmapbt=0
> > reflink=0
> > 
> > [naming]
> > ftype=1
> > 
> > [rtdev]
> > noalign=0
> 
> Separate patch, but should there be a way to spit out the current mkfs
> settings as a config file?

Sure, I have that in my mind but definitely separate patch. As can be
seen, there are still quite a bit of minor things to wrap up. No point
in getting ahead of ourselves.

> > Signed-off-by: Luis R. Rodriguez <[e-mail]>
> > ---
> >  include/builddefs.in   |   2 +
> >  man/man5/mkfs.xfs.d.5  | 121 ++++++++++
> >  man/man8/mkfs.xfs.8    |  26 +++
> >  mkfs/Makefile          |   2 +-
> >  mkfs/xfs_mkfs.c        |  47 +++-
> >  mkfs/xfs_mkfs_common.h |  12 +
> >  mkfs/xfs_mkfs_config.c | 591 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  mkfs/xfs_mkfs_config.h |  11 +
> >  8 files changed, 801 insertions(+), 11 deletions(-)
> >  create mode 100644 man/man5/mkfs.xfs.d.5
> >  create mode 100644 mkfs/xfs_mkfs_config.c
> >  create mode 100644 mkfs/xfs_mkfs_config.h
> > 
> > diff --git a/include/builddefs.in b/include/builddefs.in
> > index 8aac06cf90dc..e1ee9f7ba086 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -62,6 +62,7 @@ PKG_LIB_DIR	= @libdir@@libdirsuffix@
> >  PKG_INC_DIR	= @includedir@/xfs
> >  DK_INC_DIR	= @includedir@/disk
> >  PKG_MAN_DIR	= @mandir@
> > +PKG_ETC_DIR	= @sysconfdir@
> >  PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
> >  PKG_LOCALE_DIR	= @datadir@/locale
> >  
> > @@ -196,6 +197,7 @@ endif
> >  
> >  GCFLAGS = $(DEBUG) \
> >  	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
> > +	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
> >  	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
> >  
> >  ifeq ($(ENABLE_GETTEXT),yes)
> > diff --git a/man/man5/mkfs.xfs.d.5 b/man/man5/mkfs.xfs.d.5
> > new file mode 100644
> > index 000000000000..5424f730a2e5
> > --- /dev/null
> > +++ b/man/man5/mkfs.xfs.d.5
> > @@ -0,0 +1,121 @@
> > +.TH mkfs.xfs.d 5
> > +.SH NAME
> > +mkfs.xfs.d \- mkfs.xfs configuration directory
> > +.SH DESCRIPTION
> > +.B mkfs.xfs (8)
> > +uses a set of initial default parameters for configuration. These defaults
> > +are conservatively decided by the community as xfsprogs and features for XFS
> > +in the kernel advance. One can override these defaults on the
> 
> I'm not sure what the sentence "These defaults...in the kernel advance."
> means... is this close to what you meant?:
> 
> "The builtin mkfs defaults are decided by the XFS community.  New features
> are only enabled by default when the community consider it stable."

That reads much better. Amended.

> > +.B mkfs.xfs (8)
> > +command line, but there are cases where it is desirable for sensible defaults
> > +to always be specified by the system where
> 
> "...where it is desirable for the distro or the system administrator to
> establish their own supported defaults in a uniform manner." ?

Sure, I'll also add at the end:

"regardless of the version of mkfs.xfs used"

> > +.B mkfs.xfs (8)
> > +runs on. This may desirable for example on systems with old kernels where the
> > +built-in default parameters on
> > +.B mkfs.xfs (8)
> > +may not be able to create a filesystem which the old kernel supports and it
> 
> "...where the built-in default mkfs.xfs parameters create a filesystem
> that is not supported by the old kernel."
> 
> > +would be unclear what parameters are needed to produce a compatible filesystem.
> > +Overriding
> > +.B mkfs.xfs (8)
> > +built-in defaults may also be desirable if you have a series of systems with
> > +different kernels and want to be able to create filesystems which all systems
> > +are able to support properly.

I've moved this to another sentence as well then:

In such situations it would also be unclear what parameters are needed to        
produce a compatible filesystem, having a configuration file present ensures    
that if newer versions of                                                       
.B mkfs.xfs (8)                                                                 
are deployed, creating a filesystem will remain compatible.

> > +.PP
> > +The XFS configuration directory
> > +.B mkfs.xfs.d (5)
> > +can be used to define different configuration files which can be used to
> > +override the built-in default parameters by
> > +.B mkfs.xfs (8).
> > +Different configuration files are supported, the default
> > +configuration file,
> > +.I /etc/mkfs.xfs.d/default
> 
> The "/etc" part of the path should be sysconfdir and replaced by the
> build script to match whatever we're encoding into the mkfs.xfs binary.

Alrighty .. and all other "etc" references I take it.. sure.

> > +, will be looked for first and if present will be used to override
> > +.B mkfs.xfs (8)
> > +built-in default parameters. You can override the configuration file by
> > +specifying its name when using
> > +.B mkfs.xfs (8)
> > +by using the
> > +.B -c
> > +parameter. For example:
> > +.I mkfs.xfs -c experimental -f /dev/sda1
> > +will make
> > +.B mkfs.xfs (8)
> > +look for and use the configuration file
> > +.I /etc/mkfs.xfs.d/experimental
> > +to override
> > +.B mkfs.xfs (8)
> > +default parameters. If you need to override the full path for a configuration
> > +file you can use the
> > +.I MKFS_XFS_CONFIG
> > +environment variable prior to calling
> > +.B mkfs.xfs (8)
> > +to define the
> > +full path to the configuration file to be used.
> 
> Does the environment variable override -c, or the other way around?

The other way around.

Will continue the rest tomorrow.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner May 18, 2018, 12:44 a.m. UTC | #3

On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
> 
> This adds support for parsing a configuration file to override defaults
> parameters to be used for mkfs.xfs.

.....

Just looking at code right now...

> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -21,6 +21,7 @@
>  #include "xfs_multidisk.h"
>  #include "libxcmd.h"
>  #include "xfs_mkfs_common.h"
> +#include "xfs_mkfs_config.h"
>  #include "xfs_mkfs_cli.h"
>  
>  
> @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
>  	struct mkfs_params	*cfg,
>  	char			*dfile,
>  	char			*logfile,
> -	char			*rtfile)
> +	char			*rtfile,
> +	struct mkfs_default_params *dft)
>  {
>  	struct sb_feat_args	*fp = &cfg->sb_feat;
>  
>  	printf(_(
> +"config-file=%-22s\n"
>  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
>  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
>  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
>  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
>  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
>  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
>  		dfile, cfg->inodesize, (long long)cfg->agcount,
>  			(long long)cfg->agsize,
>  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,

Haven't we already printed where the config was sourced from? Why do
we need to print it again here?

> @@ -3665,6 +3669,7 @@ rewrite_secondary_superblocks(
>  /* build time defaults */
>  struct mkfs_default_params	built_in_dft = {
>  	.type = DEFAULTS_BUILTIN,
> +	.config_file = MKFS_XFS_CONF_DIR "default",
>  	.sectorsize = XFS_MIN_SECTORSIZE,
>  	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
>  	.sb_feat = {
> @@ -3714,6 +3719,13 @@ static void process_defaults(
>  	struct mkfs_default_params	*dft,
>  	struct cli_params		*cli)
>  {
> +	int ret;
> +
> +	ret = parse_config_init(dft);
> +
> +	if (ret && dft->type != DEFAULTS_BUILTIN)
> +		usage();

That doesn't make any sense in isolation.

>  	install_defaults(dft, cli);
>  }
>  
> @@ -3750,6 +3762,8 @@ main(
>  	};
>  	struct mkfs_params	cfg = {};
>  	struct mkfs_default_params	dft;
> +	char *tmp_config;
> +	char custom_config[PATH_MAX];
>  
>  	reset_defaults_and_cli(&dft, &cli);
>  
> @@ -3760,21 +3774,36 @@ main(
>  	textdomain(PACKAGE);
>  
>  	/*
> -	 * TODO: Sourcing defaults from a config file
> -	 *
>  	 * Before anything else, see if there's a config file with different
> -	 * defaults. If a file exists in <package location>, read in the new
> +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
>  	 * default values and overwrite them in the &dft structure. This way the
>  	 * new defaults will apply before we parse the CLI, and the CLI will
>  	 * still be able to override them. When more than one source is
>  	 * implemented, emit a message to indicate where the defaults being
>  	 * used came from.
>  	 */

This comment makes no mention of environment variables, or how they
interact with other sources of config files.

> +	tmp_config = getenv("MKFS_XFS_CONFIG");
> +	if (tmp_config != NULL) {
> +		dft.config_file = tmp_config;
> +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> +	}
>  
>  	process_defaults(&dft, &cli);

So why are we processing the defaults before we've checked if a
default file is specified on the command line? All this should do is
set up the config file to be read, and set the dft.source =
_("Environment Variable");

>  
> -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
>  		switch (c) {
> +		case 'c':
> +			reset_defaults_and_cli(&dft, &cli);

This will trash the existing CLI inputs that have already been
parsed. All this code here should do is set up the config file
to be read.

> +
> +			memset(custom_config, 0, sizeof(custom_config));
> +			snprintf(custom_config, sizeof(custom_config), "%s%s",
> +				 MKFS_XFS_CONF_DIR, optarg);
> +			dft.config_file = custom_config;
> +			dft.type = DEFAULTS_TYPE_CONFIG;
> +
> +			process_defaults(&dft, &cli);

Again, what happens if the user specifies multiple config files?
The second config file will trash everything from the first, as well
as any other options between them.

I think that we need to pull the "-c <file>" option from the command
line and process all the defaults /before/ we enter the main command
line processing loop.

IOWs:
	config_file = getenv("MKFS_XFS_CONFIG");
	if (config_file)
		dft.source = _("Environment Variable");

	/*
	 * Pull config line options from command line
	 */
	while ((c = getopt(argc, argv, "c:")) != EOF) {
		switch (c) {
		case 'c':
			/* XXX: fail if already seen a CLI option */
			config_file = optarg;
			dft.source = _("Command Line");
			break;
		default:
			continue;
		}
	}

	if (config_file) {
		/*
		 * parse_defaults_file() knows about config file
		 * search paths, so just pass in the raw,
		 * unvalidated filename and let it do all the work
		 * finding the file containing the default values.
		 */
		error = parse_defaults_file(&dft, config_file);
		if (error) {
			/* fail! */
		}
	}
	printf(_("Default configuration sourced from %s\n"), dft.source);

	/*
	 * Done parsing defaults now, so memcpy defaults into CLI
	 * structure, reset getopt and start parsing CLI options
	 */
	memcpy(/* sb_feats */);
	memcpy(/* fsxattr */);
	optind = 1;
	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
		switch (c) {
		case 'c':
			/* already validated and parsed, ignore */
			break;
		....


> diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
> index d867ab377185..028bbf96017f 100644
> --- a/mkfs/xfs_mkfs_common.h
> +++ b/mkfs/xfs_mkfs_common.h
> @@ -46,9 +46,20 @@ struct sb_feat_args {
>   * These are the different possibilities by which you can end up parsing
>   * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
>   * file parsed and we are using the built-in defaults on this code.
> + * DEFAULTS_CONFIG means the default configuration file was found and used.
> + * DEFAULTS_TYPE_CONFIG means the user asked for a custom configuration type
> + * and it was used, this means the default directory for mkfs.xfs
> + * configuration files was used to look for the type specified. If you need
> + * to override the default mkfs.xfs directory for configuration file you can
> + * use the MKFS_XFS_CONFIG environment variable to set the absolute path to
> + * your custom configuration file, when this is set the type is set to
> + * DEFAULTS_ENVIRONMENT_CONFIG.
>   */
>  enum default_params_type {
>  	DEFAULTS_BUILTIN = 0,
> +	DEFAULTS_CONFIG,
> +	DEFAULTS_ENVIRONMENT_CONFIG,
> +	DEFAULTS_TYPE_CONFIG,
>  };

Again, I just don't see the need for this...

>  /*
> @@ -61,6 +72,7 @@ enum default_params_type {
>   */
>  struct mkfs_default_params {
>  	enum default_params_type type; /* where the defaults came from */
> +	const char *config_file;

Or this (see above :).

>  
>  	int	sectorsize;
>  	int	blocksize;
> diff --git a/mkfs/xfs_mkfs_config.c b/mkfs/xfs_mkfs_config.c
> new file mode 100644
> index 000000000000..d554638982ff
> --- /dev/null
> +++ b/mkfs/xfs_mkfs_config.c
> @@ -0,0 +1,591 @@
> +/*
> + * Copyright (c) 2018 Luis R. Rodriguez <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +
> +#include "xfs_mkfs_common.h"
> +#include "xfs_mkfs_config.h"
> +
> +#define CONFIG_MAX_KEY		1024
> +#define CONFIG_MAX_VALUE	PATH_MAX
> +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> +
> +/*
> + * Enums for each configuration option. All these currently match the CLI
> + * parameters for now but this may change later, so we keep all this code
> + * and definitions separate. The rules for configuration parameters may also
> + * differ.
> + */

Why define them all when we are only going to parse a small subset?

> +enum mkfs_config_section {
> +	MKFS_CONFIG_SECTION_BLOCK = 0,
> +	MKFS_CONFIG_SECTION_DATA,
> +	MKFS_CONFIG_SECTION_INODE,
> +	MKFS_CONFIG_SECTION_LOG,
> +	MKFS_CONFIG_SECTION_METADATA,
> +	MKFS_CONFIG_SECTION_NAMING,
> +	MKFS_CONFIG_SECTION_RTDEV,
> +	MKFS_CONFIG_SECTION_SECTOR,
> +
> +	MKFS_CONFIG_SECTION_INVALID,
> +};
> +
> +struct opt_params {
> +	const enum mkfs_config_section section;
> +	const char	*subopts[MAX_SUBOPTS];
> +};
> +
> +extern struct opt_params config_sopts;

Why is this declared here?

> +
> +struct opt_params config_bopts = {
> +	.section = 'b',

That's not an enum, looks broken. Why isn't this the ascii name
of the config section in the config file?

> +	.subopts = {
> +		[B_SIZE] = "size",
> +	},
> +};
> +
> +struct opt_params config_dopts = {
> +	.section = 'd',
> +	.subopts = {
> +		[D_AGCOUNT] = "agcount",
> +		[D_FILE] = "file",
> +		[D_NAME] = "name",
> +		[D_SIZE] = "size",
> +		[D_SUNIT] = "sunit",
> +		[D_SWIDTH] = "swidth",
> +		[D_AGSIZE] = "agsize",
> +		[D_SU] = "su",
> +		[D_SW] = "sw",
> +		[D_SECTSIZE] = "sectsize",
> +		[D_NOALIGN] = "noalign",
> +		[D_RTINHERIT] = "rtinherit",
> +		[D_PROJINHERIT] = "projinherit",
> +		[D_EXTSZINHERIT] = "extszinherit",
> +		[D_COWEXTSIZE] = "cowextsize",
> +	},
> +};

So the parser will accept names we can't configure?

[...]

> +const char *default_type_str(enum default_params_type type)
> +{
> +	switch (type) {
> +	case DEFAULTS_BUILTIN:
> +		return _("package built-in definitions");
> +	case DEFAULTS_CONFIG:
> +		return _("default configuration system file");
> +	case DEFAULTS_ENVIRONMENT_CONFIG:
> +		return _("custom configuration file set by environment");
> +	case DEFAULTS_TYPE_CONFIG:
> +		return _("custom configuration type file");
> +	}
> +	return _("Unkown\n");
> +}

This function can go away.

> +static int block_config_parser(struct mkfs_default_params *dft, int subopt,
> +			       bool value)
> +{
> +	return -EINVAL;
> +}

Why is the value passed to the parser a bool type and not a
uint64_t?

> +struct config_subopts {
> +	enum mkfs_config_section	section;
> +	struct opt_params		*opts;
> +	int (*config_parser)(struct mkfs_default_params *dft,
> +			     int subop, bool value);
> +} config_subopt_tab[] = {
> +	{ MKFS_CONFIG_SECTION_BLOCK, &config_bopts, block_config_parser },
> +	{ MKFS_CONFIG_SECTION_DATA, &config_dopts, data_config_parser },
> +	{ MKFS_CONFIG_SECTION_INODE, &config_iopts, inode_config_parser },
> +	{ MKFS_CONFIG_SECTION_LOG, &config_lopts, log_config_parser },
> +	{ MKFS_CONFIG_SECTION_METADATA, &config_mopts, meta_config_parser },
> +	{ MKFS_CONFIG_SECTION_NAMING, &config_nopts, naming_config_parser },
> +	{ MKFS_CONFIG_SECTION_RTDEV, &config_ropts, rtdev_config_parser },
> +	{ MKFS_CONFIG_SECTION_SECTOR, &config_sopts, sector_config_parser },
> +	{ '\0', NULL },
> +};
> +
> +static int
> +parse_config_subopts(
> +	enum mkfs_config_section	section,
> +	bool				value,
> +	char				*line,
> +	struct mkfs_default_params	*dft)
> +{
> +	struct config_subopts *sop = &config_subopt_tab[0];
> +	char	**subopts = (char **)sop->opts->subopts;
> +	int	subopt;
> +	char *val;
> +
> +	while (sop->opts) {
> +		if (sop->section == section)
> +			break;
> +		sop++;
> +	}
> +
> +	/* should never happen */
> +	if (!sop->opts)
> +		return -EINVAL;
> +
> +	subopts = (char **)sop->opts->subopts;
> +	subopt = getsubopt(&line, subopts, &val);
> +	return (sop->config_parser)(dft, subopt, value);
> +}

This seems back to front - why do we walk the table to find the
entry that corresponds to an enum when we've....

> +static enum mkfs_config_section get_config_section(const char *buffer)
> +{
> +	if (strncmp("block", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_BLOCK;

This will match "blocksdfsgdfgd" - strcmp is fine here.

> +	if (strncmp("data", buffer, 4) == 0)
> +		return MKFS_CONFIG_SECTION_DATA;
> +	if (strncmp("inode", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_INODE;
> +	if (strncmp("log", buffer, 3) == 0)
> +		return MKFS_CONFIG_SECTION_LOG;
> +	if (strncmp("metadata", buffer, 8) == 0)
> +		return MKFS_CONFIG_SECTION_METADATA;
> +	if (strncmp("naming", buffer, 6) == 0)
> +		return MKFS_CONFIG_SECTION_NAMING;
> +	if (strncmp("rtdev", buffer, 5) == 0)
> +		return MKFS_CONFIG_SECTION_RTDEV;
> +	if (strncmp("sector", buffer, 6) == 0)
> +		return MKFS_CONFIG_SECTION_SECTOR;
> +
> +	return MKFS_CONFIG_SECTION_INVALID;
> +}

Directly parsed the section name to turn it into an enum? Indeed,
this looks all wrong in terms of structure.

static struct confopts blockopts = {
	.name = "[block]",
	.subopts = {
		[B_SIZE] = "size",
	},
	.parser = block_config_parser,
	.seen = false,
};

struct confopts *
get_confopts(const char *section)
{
	if (strcmp(blockopts.name, section) == 0)
		return &blockopts;
	.....
}

	....
	opts = get_confopts(section);
	if (!opts)
		/* fail, invalid section */
	if (opts->seen)
		/* fail, multiple specification */
	/* loop extracting and processing sbuopts */


And now the whole section enum construct and the
parse_config_subopts() function goes away.

> +static int mkfs_check_config_file_limits(const char *filename,
> +					 const struct stat *sb,
> +					 unsigned int max_entries)
> +{
> +	unsigned long long size_limit;
> +
> +	size_limit = CONFIG_MAX_BUFFER * max_entries;
> +
> +	/*
> +	 * Detect wrap around -- if max_entries * size_limit goves over
> +	 * unsigned long long we detect that there. Note some libraries,
> +	 * like libiniconfig, only groks max INT_MAX entries anyway, so
> +	 * if we ever use a library for parsing we'd be restricted to that
> +	 * limit.
> +	 */
> +	if (size_limit < max_entries || max_entries > INT_MAX)
> +		return -E2BIG;
> +
> +	if (sb->st_size > size_limit) {
> +		fprintf(stderr,
> +			"File %s is too big! File size limit: %llu\n",
> +			filename, size_limit);
> +		return -E2BIG;
> +	}
> +
> +	return 0;
> +}

What is this supposed to protect again? If somone what to put lots
of comments or blank space in the config files, then what's the
problem with that? Seems like you jump through a lot of hoops to do
just this calculation - why not just say "1MB is more than enough
for anyone"?

> +enum parse_line_type {
> +	PARSE_COMMENT = 0,
> +	PARSE_SECTION,
> +	PARSE_TAG_VALUE,
> +	PARSE_INVALID,
> +	PARSE_EOF,
> +};

> +
> +static int parse_line_section(const char *line, char *tag)

I should have mentioned this before now, but it's finally got to me.
:) Function format for XFS code is:

static int
parse_line_section(
	const char	*line,
	char		*tag)
{
....

> +{
> +	return sscanf(line, " [%[^]]s]", tag);
> +}
> +
> +static int parse_line_tag_value(const char *line, char *tag,
> +				unsigned int *value)
> +{
> +	return sscanf(line, " %[^ \t=]"
> +		      " = "
> +		      "%u ",
> +		      tag, value);

Value can be a 64 bit quantity, so needs to be uint64_t....

> +}
> +
> +static enum parse_line_type parse_get_line_type(const char *line, char *tag,
> +						bool *value)
> +{
> +	int ret;
> +	unsigned int uint_value;
> +
> +	memset(tag, 0, 80);

Why? This should be done in the caller, and if there's a fixed
buffer size, then please use a #define.

> +
> +	if (strlen(line) < 2)
> +		return PARSE_INVALID;

So empty lines return PARSE_INVALID?

> +
> +	if (line[0] == '#')
> +		return PARSE_COMMENT;

What about lines starting with whitespace? e.g. " # comment"

> +	ret = parse_line_section(line, tag);
> +	if (ret == 1)
> +		return PARSE_SECTION;
> +
> +	if (ret == EOF)
> +		return PARSE_EOF;
> +
> +	ret = parse_line_tag_value(line, tag, &uint_value);
> +	if (ret == 2) {
> +		if (uint_value != 1 && uint_value != 0)
> +			return -EINVAL;
> +
> +		*value = !!uint_value;
> +
> +		return PARSE_TAG_VALUE;
> +	}

pass the full value back to the caller, let the subopt parser
validate it and squash it to the correct type.

> +
> +	if (ret == EOF)
> +		return PARSE_EOF;
> +
> +	return PARSE_INVALID;
> +}
> +
> +int parse_config_init(struct mkfs_default_params *dft)

int
parse_config_file(
	struct mkfs_default_params	*dft,
	const char 			*config_file)
{

i.e. we only get called if there's a config file configured.

> +{
> +	int ret;
> +	FILE *fp;
> +	struct stat sp;
> +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> +					    sizeof(struct config_subopts) - 1;
> +	char *p;
> +	char line[80], tag[80];
> +	bool value;
> +	enum parse_line_type parse_type;
> +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> +
> +	fp = fopen(dft->config_file, "r");
> +	if (!fp) {
> +		if (dft->type != DEFAULTS_BUILTIN) {
> +			fprintf(stderr, _("could not open config file: %s\n"),
> +				dft->config_file);
> +			ret = -ENOENT;
> +		} else
> +			ret = 0;
> +		return ret;
> +	}

This should be split out into a separate function that takes the
config file and tries to open it. If it's a relative path that was
supplied, then this function can also try all the configured search
paths for config files.

> +	/*
> +	 * If we found a file without it being specified on the command line,
> +	 * it would be the default configuration file.
> +	 */
> +	if (dft->type == DEFAULTS_BUILTIN)
> +		dft->type = DEFAULTS_CONFIG;
> +
> +	ret = stat(dft->config_file, &sp);
> +	if (ret) {
> +		if (dft->type != DEFAULTS_BUILTIN)
> +			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
> +				dft->config_file, strerror(ret));
> +		return ret;
> +	}

This dft->type futzing can all go away - if the file exists, the
mkfs will already know where it came from.

> +
> +	if (!sp.st_size)
> +		return 0;
> +
> +	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
> +					    num_subopts_sections);
> +	if (ret)
> +		return ret;
> +
> +	while (!feof(fp)) {
> +		p = fgets(line, sizeof(line), fp);

What if the line is longer than 80 characters? e.g. a long comment
In that case, we need a line[79] = '/0';, or a memset of line to
clear it before fgets is called. i think I prefer the latter...

> +		if (!p)
> +			continue;
> +
> +		parse_type = parse_get_line_type(line, tag, &value);
> +
> +		switch (parse_type) {
> +		case PARSE_COMMENT:
> +		case PARSE_INVALID:
> +		case PARSE_EOF:
> +			break;
> +		case PARSE_SECTION:
> +			section = get_config_section(tag);
> +			if (!section) {
> +				fprintf(stderr, _("Invalid section: %s\n"),
> +						tag);
> +				return -EINVAL;
> +			}
> +			break;
> +		case PARSE_TAG_VALUE:
> +			/* Trims white spaces */
> +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> +			ret = parse_config_subopts(section, value, line, dft);

We've already got the tag and value as discrete variables - why put
them back into an ascii string to pass to another function for it to
split them back into discrete variables?

This really seems like it could be improved by making this less
abstract. i.e.

	while (!feof(fp)) {
		memset(line, 0, sizeof(line));
		p = fgets(line, sizeof(line), fp);
		if (!p)
			continue;

		token = parse_section(line);
		if (token) {
			opts = get_confopts(section);
			if (!opts)
				/* fail, invalid */;
			if (opts->seen)
				/* fail, respec */ ;
		}
		if (!opts)
			continue;

		token = parse_value(line, &value);
		if (!token)
			continue;
		error = opts->parse(opts, token, value);
		if (error) {
			/* warn, fail, ignore? */ ;
		}
	}

I suspect with this even all the B_SIZE type of enums can go away,
too, because all we need to do is string compares of the token
directly in the suboption parser to know what option we are about to
set....


> diff --git a/mkfs/xfs_mkfs_config.h b/mkfs/xfs_mkfs_config.h
> new file mode 100644
> index 000000000000..407d37fffb1a
> --- /dev/null
> +++ b/mkfs/xfs_mkfs_config.h
> @@ -0,0 +1,11 @@
> +#ifndef _XFS_MKFS_CONFIG_H
> +#define _XFS_MKFS_CONFIG_H
> +
> +#include "xfs_mkfs_common.h"
> +
> +#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d/"

This can go in the C file, as there's no need for anything other
than the file open routine to know this.

Cheers,

Dave.

Eric Sandeen May 18, 2018, 3:24 a.m. UTC | #4

On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> You may want to stick to specific set of configuration options when
> creating filesystems with mkfs.xfs -- sometimes due to pure technical
> reasons, but some other times to ensure systems remain compatible as
> new features are introduced with older kernels, or if you always want
> to take advantage of some new feature which would otherwise typically
> be disruptive.
> 
> This adds support for parsing a configuration file to override defaults
> parameters to be used for mkfs.xfs.
> 
> We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> different configuration files, if none is specified we look for the
> default configuration file, /etc/mkfs.xfs.d/default. You can override
> with -c.  For instance, if you specify:
> 
> 	mkfs.xfs -c experimental -f /dev/loop0
> 
> The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> file. If you really need to override the full path of the configuration
> file you may use the MKFS_XFS_CONFIG environment variable.

I'm swamped under a deadline at work this week so just commenting at a
very high level for now, but I'm curious; why use an env var vs
providing a full path for -c ?  env vars always strike me as magic unexpected
behaviors.

# mkfs.xfs -c /my/fancy/path/to/config

seems much clearer than

# export MKFS_XFS_CONFIG=/my/fancy/path/to/
# mkfs.xfs -c config

i.e. if a full path is specified use it, else use the config directory.

Thoughts?

Thanks,
-Eric

> To verify what configuration file is used on a system use the typical:
> 
>    mkfs.xfs -N
> 
> There is only a subset of options allowed to be set on the configuration
> file, and currently only 1 or 0 are acceptable values. The default
> parameters you can override on a configuration file and their current
> built-in default settings are:
> 
> [data]
> noalign=0
> 
> [inode]
> align=1
> projid32bit=1
> sparse=0
> 
> [log]
> lazy-count=1
> 
> [metadata]
> crc=1
> finobt=1
> rmapbt=0
> reflink=0
> 
> [naming]
> ftype=1
> 
> [rtdev]
> noalign=0
> 
> Signed-off-by: Luis R. Rodriguez<[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong May 18, 2018, 3:46 a.m. UTC | #5

On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> > You may want to stick to specific set of configuration options when
> > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > reasons, but some other times to ensure systems remain compatible as
> > new features are introduced with older kernels, or if you always want
> > to take advantage of some new feature which would otherwise typically
> > be disruptive.
> > 
> > This adds support for parsing a configuration file to override defaults
> > parameters to be used for mkfs.xfs.
> > 
> > We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> > different configuration files, if none is specified we look for the
> > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > with -c.  For instance, if you specify:
> > 
> > 	mkfs.xfs -c experimental -f /dev/loop0
> > 
> > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > file. If you really need to override the full path of the configuration
> > file you may use the MKFS_XFS_CONFIG environment variable.
> 
> I'm swamped under a deadline at work this week so just commenting at a
> very high level for now, but I'm curious; why use an env var vs
> providing a full path for -c ?  env vars always strike me as magic unexpected
> behaviors.
> 
> # mkfs.xfs -c /my/fancy/path/to/config
> 
> seems much clearer than
> 
> # export MKFS_XFS_CONFIG=/my/fancy/path/to/
> # mkfs.xfs -c config
> 
> i.e. if a full path is specified use it, else use the config directory.
> 
> Thoughts?

In this case your choices are:
MKFS_XFS_CONFIG=/my/fancy/path/to/config mkfs.xfs

or:
cp /my/fancy/path/to/config /etc/mkfs.xfs.d/hoogah
mkfs.xfs -c hooga

<shrug> Bikeshedding more, what if either option accepted either an
absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ?  And the -c
option can be specified once to override the environment variable /
builtin detaults?

--D

> 
> Thanks,
> -Eric
> 
> > To verify what configuration file is used on a system use the typical:
> > 
> >    mkfs.xfs -N
> > 
> > There is only a subset of options allowed to be set on the configuration
> > file, and currently only 1 or 0 are acceptable values. The default
> > parameters you can override on a configuration file and their current
> > built-in default settings are:
> > 
> > [data]
> > noalign=0
> > 
> > [inode]
> > align=1
> > projid32bit=1
> > sparse=0
> > 
> > [log]
> > lazy-count=1
> > 
> > [metadata]
> > crc=1
> > finobt=1
> > rmapbt=0
> > reflink=0
> > 
> > [naming]
> > ftype=1
> > 
> > [rtdev]
> > noalign=0
> > 
> > Signed-off-by: Luis R. Rodriguez<[e-mail]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 18, 2018, 3:38 p.m. UTC | #6

On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> > On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> > > You may want to stick to specific set of configuration options when
> > > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > > reasons, but some other times to ensure systems remain compatible as
> > > new features are introduced with older kernels, or if you always want
> > > to take advantage of some new feature which would otherwise typically
> > > be disruptive.
> > > 
> > > This adds support for parsing a configuration file to override defaults
> > > parameters to be used for mkfs.xfs.
> > > 
> > > We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> > > different configuration files, if none is specified we look for the
> > > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > > with -c.  For instance, if you specify:
> > > 
> > > 	mkfs.xfs -c experimental -f /dev/loop0
> > > 
> > > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > > file. If you really need to override the full path of the configuration
> > > file you may use the MKFS_XFS_CONFIG environment variable.
> > 
> > I'm swamped under a deadline at work this week so just commenting at a
> > very high level for now, but I'm curious; why use an env var vs
> > providing a full path for -c ?  env vars always strike me as magic unexpected
> > behaviors.
> > 
> > # mkfs.xfs -c /my/fancy/path/to/config
> > 
> > seems much clearer than
> > 
> > # export MKFS_XFS_CONFIG=/my/fancy/path/to/
> > # mkfs.xfs -c config
> > 
> > i.e. if a full path is specified use it, else use the config directory.
> > 
> > Thoughts?
> 
> In this case your choices are:
> MKFS_XFS_CONFIG=/my/fancy/path/to/config mkfs.xfs
> 
> or:
> cp /my/fancy/path/to/config /etc/mkfs.xfs.d/hoogah
> mkfs.xfs -c hooga
> 
> <shrug> Bikeshedding more, what if either option accepted either an
> absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ?

Let us recall that the reason for -c set in stone on /@sysconfigdir@/mkfs.xfs.d/
was to allow clean simple code.

The MKFS_XFS_CONFIG is simply a comopromise to allow flexibility on a
full path in case you cannot use the /@sysconfigdir@/mkfs.xfs.d/
directory.

Supporting both remains simple.

If we wanted to support what you suggest, if a user specified -c hoogah
we'd have to treat multiple possibilities in code, increasing complexity:

  a) Did the user mean the hoogah in the present directory?
  b) Did the user mean hoogah in /@sysconfigdir@/mkfs.xfs.d/

Granted, if the user specified a -c /tmp/hoogah its clearer that the
full path would be desirable. So, I think what you suggest makes sense
provided we get rid of a) option and require only an alternative path
*iff* the first character in the path is '/'. Does this work for all
cases we wish to support a full path in?

> And the -c
> option can be specified once to override the environment variable /
> builtin detaults?

The -c option as-is in my patch series always overrides the environment
variable, but it is currently always tied to
/@sysconfigdir@/mkfs.xfs.d/. So if you specify both the -c wins. Its
also why I implemented the reset feature / mechanism.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Sandeen May 18, 2018, 5:09 p.m. UTC | #7

On 5/18/18 10:38 AM, Luis R. Rodriguez wrote:
> On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
>> On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:

...
  
> Let us recall that the reason for -c set in stone on /@sysconfigdir@/mkfs.xfs.d/
> was to allow clean simple code.
> 
> The MKFS_XFS_CONFIG is simply a comopromise to allow flexibility on a
> full path in case you cannot use the /@sysconfigdir@/mkfs.xfs.d/
> directory.
> 
> Supporting both remains simple.
> 
> If we wanted to support what you suggest, if a user specified -c hoogah
> we'd have to treat multiple possibilities in code, increasing complexity:
> 
>    a) Did the user mean the hoogah in the present directory?
>    b) Did the user mean hoogah in /@sysconfigdir@/mkfs.xfs.d/
> 
> Granted, if the user specified a -c /tmp/hoogah its clearer that the
> full path would be desirable. So, I think what you suggest makes sense
> provided we get rid of a) option and require only an alternative path
> *iff* the first character in the path is '/'. Does this work for all
> cases we wish to support a full path in?

Possibly include "./" as well. i.e.  mkfs.xfs -c ./configdir/myconfig

so:

mkfs.xfs -c hoogah		searches /@sysconfigdir@/mkfs.xfs.d/
mkfs.xfs -c ./hoogah		or
mkfs.xfs -c /path/to/hoogah	do exactly what you'd expect.

>> And the -c
>> option can be specified once to override the environment variable /
>> builtin detaults?

I'd like to drop the environment variable altogether.  If there is a strong
case to be made for keeping it, please make it. :)  (I don't consider the
existence of MKE2FS_CONFIG to be a strong argument, FWIW, because our
semantics & mechanisms are quite different.)

Thanks,
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 18, 2018, 11:56 p.m. UTC | #8

On Fri, May 18, 2018 at 12:09:23PM -0500, Eric Sandeen wrote:
> On 5/18/18 10:38 AM, Luis R. Rodriguez wrote:
> > On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> > > On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> 
> ...
> > Let us recall that the reason for -c set in stone on /@sysconfigdir@/mkfs.xfs.d/
> > was to allow clean simple code.
> > 
> > The MKFS_XFS_CONFIG is simply a comopromise to allow flexibility on a
> > full path in case you cannot use the /@sysconfigdir@/mkfs.xfs.d/
> > directory.
> > 
> > Supporting both remains simple.
> > 
> > If we wanted to support what you suggest, if a user specified -c hoogah
> > we'd have to treat multiple possibilities in code, increasing complexity:
> > 
> >    a) Did the user mean the hoogah in the present directory?
> >    b) Did the user mean hoogah in /@sysconfigdir@/mkfs.xfs.d/
> > 
> > Granted, if the user specified a -c /tmp/hoogah its clearer that the
> > full path would be desirable. So, I think what you suggest makes sense
> > provided we get rid of a) option and require only an alternative path
> > *iff* the first character in the path is '/'. Does this work for all
> > cases we wish to support a full path in?
> 
> Possibly include "./" as well. i.e.  mkfs.xfs -c ./configdir/myconfig
> 
> so:
> 
> mkfs.xfs -c hoogah		searches /@sysconfigdir@/mkfs.xfs.d/
> mkfs.xfs -c ./hoogah		or
> mkfs.xfs -c /path/to/hoogah	do exactly what you'd expect.
> 
> > > And the -c
> > > option can be specified once to override the environment variable /
> > > builtin detaults?
> 
> I'd like to drop the environment variable altogether.  If there is a strong
> case to be made for keeping it, please make it. :)  (I don't consider the
> existence of MKE2FS_CONFIG to be a strong argument, FWIW, because our
> semantics & mechanisms are quite different.)

Sounds good. I'll drop the environment variable junk and only support
the above 3 cases.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 19, 2018, 1:32 a.m. UTC | #9

On Fri, May 18, 2018 at 10:44:04AM +1000, Dave Chinner wrote:
> On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
> >  	struct mkfs_params	*cfg,
> >  	char			*dfile,
> >  	char			*logfile,
> > -	char			*rtfile)
> > +	char			*rtfile,
> > +	struct mkfs_default_params *dft)
> >  {
> >  	struct sb_feat_args	*fp = &cfg->sb_feat;
> >  
> >  	printf(_(
> > +"config-file=%-22s\n"
> >  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> >  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> >  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> > @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
> >  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> >  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> >  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> > +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
> >  		dfile, cfg->inodesize, (long long)cfg->agcount,
> >  			(long long)cfg->agsize,
> >  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
> 
> Haven't we already printed where the config was sourced from? Why do
> we need to print it again here?

The other print describes the enum, this print prints out the *used*
config file path if a config file was actually used. Without the other
print this would just print either the config file or empty.

Since we are going to remove the environment variable option, then
I think the other print is now not needed anymore and my preference
is to keep it here.

Thoughts?

> > @@ -3665,6 +3669,7 @@ rewrite_secondary_superblocks(
> >  /* build time defaults */
> >  struct mkfs_default_params	built_in_dft = {
> >  	.type = DEFAULTS_BUILTIN,
> > +	.config_file = MKFS_XFS_CONF_DIR "default",
> >  	.sectorsize = XFS_MIN_SECTORSIZE,
> >  	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
> >  	.sb_feat = {
> > @@ -3714,6 +3719,13 @@ static void process_defaults(
> >  	struct mkfs_default_params	*dft,
> >  	struct cli_params		*cli)
> >  {
> > +	int ret;
> > +
> > +	ret = parse_config_init(dft);
> > +
> > +	if (ret && dft->type != DEFAULTS_BUILTIN)
> > +		usage();
> 
> That doesn't make any sense in isolation.

Not sure what you meant at first, but I see that you prefer this
to be hidden from the caller. Will change.

> >  	install_defaults(dft, cli);
> >  }
> >  
> > @@ -3750,6 +3762,8 @@ main(
> >  	};
> >  	struct mkfs_params	cfg = {};
> >  	struct mkfs_default_params	dft;
> > +	char *tmp_config;
> > +	char custom_config[PATH_MAX];
> >  
> >  	reset_defaults_and_cli(&dft, &cli);
> >  
> > @@ -3760,21 +3774,36 @@ main(
> >  	textdomain(PACKAGE);
> >  
> >  	/*
> > -	 * TODO: Sourcing defaults from a config file
> > -	 *
> >  	 * Before anything else, see if there's a config file with different
> > -	 * defaults. If a file exists in <package location>, read in the new
> > +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
> >  	 * default values and overwrite them in the &dft structure. This way the
> >  	 * new defaults will apply before we parse the CLI, and the CLI will
> >  	 * still be able to override them. When more than one source is
> >  	 * implemented, emit a message to indicate where the defaults being
> >  	 * used came from.
> >  	 */
> 
> This comment makes no mention of environment variables, or how they
> interact with other sources of config files.

As per Eric's request, We're getting rid of the environment variable option, so
mentioning it is no longer needed.

> > +	tmp_config = getenv("MKFS_XFS_CONFIG");
> > +	if (tmp_config != NULL) {
> > +		dft.config_file = tmp_config;
> > +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> > +	}
> >  
> >  	process_defaults(&dft, &cli);
> 
> So why are we processing the defaults before we've checked if a
> default file is specified on the command line? All this should do is
> set up the config file to be read, and set the dft.source =
> _("Environment Variable");

This was being done *here* since I previously has setup to process the
arguments *early* and check if a -c was specified, and if so use the config
file for the defaults, otherwise the environment variable if set. But since
the way I had implemented processing the arguments early relied on not well
guaranteed mechanisms I resorted to avoid that approach.

This was the alternative I came up with.

The environment variable stuff is going away so this should be much simpler
now, however the question of argument processing early still remains.

> >  
> > -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> >  		switch (c) {
> > +		case 'c':
> > +			reset_defaults_and_cli(&dft, &cli);
> 
> This will trash the existing CLI inputs that have already been
> parsed. 

Exactly.

> All this code here should do is set up the config file
> to be read.

Consider the case of /etc/mkfs.d/default existing and it having

[metadata]
crc = 1
[naming]
ftype=1

And suppose the user passed -c foo which had:

[metadata]
crc = 0

Without the reset we'd make /etc/mkfs.d/default the system wide defaults onto
which -c parameters overlay on top of. I figured that was not desirable.

With built-in defaults as *base* and with the compromise that if -c is used
it will reset to built-in defaults, we remove that overlay problem.

> > +
> > +			memset(custom_config, 0, sizeof(custom_config));
> > +			snprintf(custom_config, sizeof(custom_config), "%s%s",
> > +				 MKFS_XFS_CONF_DIR, optarg);
> > +			dft.config_file = custom_config;
> > +			dft.type = DEFAULTS_TYPE_CONFIG;
> > +
> > +			process_defaults(&dft, &cli);
> 
> Again, what happens if the user specifies multiple config files?

The reset strategy lets the user set -c as many times as they wish and also
starts from a clean base always.

> The second config file will trash everything from the first, as well
> as any other options between them.

Yes! By design as we couldn't easily parse arguments early first and then
choose just one strategy.

I liked the simplicity that this brings.

Would you really want multiple -c options to work as overlays, one on top of
the other?

> I think that we need to pull the "-c <file>" option from the command
> line and process all the defaults /before/ we enter the main command
> line processing loop.

That is what I had done originally but ran into that snag of processing
arguments twice and the undocumented functionality I found that worked.

> IOWs:
> 	config_file = getenv("MKFS_XFS_CONFIG");
> 	if (config_file)
> 		dft.source = _("Environment Variable");
> 
> 	/*
> 	 * Pull config line options from command line
> 	 */
> 	while ((c = getopt(argc, argv, "c:")) != EOF) {
> 		switch (c) {
> 		case 'c':
> 			/* XXX: fail if already seen a CLI option */

BTW isn't my enum here sufficient to check for this easily in a clean
short and easy way? Granted we'd have only built-in and config, but
still... simple and sweet, no?

And do we want to support multiple -c options?

> 			config_file = optarg;
> 			dft.source = _("Command Line");
> 			break;
> 		default:
> 			continue;
> 		}
> 	}
> 
> 	if (config_file) {
> 		/*
> 		 * parse_defaults_file() knows about config file
> 		 * search paths, so just pass in the raw,
> 		 * unvalidated filename and let it do all the work
> 		 * finding the file containing the default values.
> 		 */
> 		error = parse_defaults_file(&dft, config_file);
> 		if (error) {
> 			/* fail! */
> 		}
> 	}
> 	printf(_("Default configuration sourced from %s\n"), dft.source);
> 
> 	/*
> 	 * Done parsing defaults now, so memcpy defaults into CLI
> 	 * structure, reset getopt and start parsing CLI options
> 	 */
> 	memcpy(/* sb_feats */);
> 	memcpy(/* fsxattr */);
> 	optind = 1;

My getopt(3) *does* not make mention of any of this. This man page however does:

http://man7.org/linux/man-pages/man3/getopt.3.html

NOTES
       A program that scans multiple argument vectors, or rescans the same
       vector more than once, and wants to make use of GNU extensions such
       as '+' and '-' at the start of optstring, or changes the value of
       POSIXLY_CORRECT between scans, must reinitialize getopt() by
       resetting optind to 0, rather than the traditional value of 1.
       (Resetting to 0 forces the invocation of an internal initialization
       routine that rechecks POSIXLY_CORRECT and checks for GNU extensions
       in optstring.)

But... my man page does say:

	The  variable  optind is the index of the next element of the argv[]
	vector to be processed. It shall be initialized to 1 by the system, and
	getopt() shall update it when it finishes with each element of
	argv[]. If the application sets optind to zero before calling getopt(),
	the behavior is unspecified.

So as per the above man page URL, setting it to 1 seems OK... but my man page
just lacks any documentation over this. I don't think we use the GNU extensions
mentioned of '+' or '-'.

POSIXLY_CORRECT seems to be an used on the environment, and helps Bash determine
if it should enter into POSIX mode, before reading startup files.

We don't set or use POSIXLY_CORRECT anywhere so I think we're fine with the:

optind = 1;

However perhaps a big fat NOTE is worthy?

> 	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> 		switch (c) {
> 		case 'c':
> 			/* already validated and parsed, ignore */


> 			break;
> 		....

I can live with the above.

> 
> > diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
> > index d867ab377185..028bbf96017f 100644
> > --- a/mkfs/xfs_mkfs_common.h
> > +++ b/mkfs/xfs_mkfs_common.h
> > @@ -46,9 +46,20 @@ struct sb_feat_args {
> >   * These are the different possibilities by which you can end up parsing
> >   * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
> >   * file parsed and we are using the built-in defaults on this code.
> > + * DEFAULTS_CONFIG means the default configuration file was found and used.
> > + * DEFAULTS_TYPE_CONFIG means the user asked for a custom configuration type
> > + * and it was used, this means the default directory for mkfs.xfs
> > + * configuration files was used to look for the type specified. If you need
> > + * to override the default mkfs.xfs directory for configuration file you can
> > + * use the MKFS_XFS_CONFIG environment variable to set the absolute path to
> > + * your custom configuration file, when this is set the type is set to
> > + * DEFAULTS_ENVIRONMENT_CONFIG.
> >   */
> >  enum default_params_type {
> >  	DEFAULTS_BUILTIN = 0,
> > +	DEFAULTS_CONFIG,
> > +	DEFAULTS_ENVIRONMENT_CONFIG,
> > +	DEFAULTS_TYPE_CONFIG,
> >  };
> 
> Again, I just don't see the need for this...

Given what has decided so far, only documentation and checking for it being
set already.

> >  /*
> > @@ -61,6 +72,7 @@ enum default_params_type {
> >   */
> >  struct mkfs_default_params {
> >  	enum default_params_type type; /* where the defaults came from */
> > +	const char *config_file;
> 
> Or this (see above :).

Alright.

> >  
> >  	int	sectorsize;
> >  	int	blocksize;
> > diff --git a/mkfs/xfs_mkfs_config.c b/mkfs/xfs_mkfs_config.c
> > new file mode 100644
> > index 000000000000..d554638982ff
> > --- /dev/null
> > +++ b/mkfs/xfs_mkfs_config.c
> > @@ -0,0 +1,591 @@
> > +/*
> > + * Copyright (c) 2018 Luis R. Rodriguez <[e-mail]>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it would be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write the Free Software Foundation,
> > + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + */
> > +
> > +#include "xfs_mkfs_common.h"
> > +#include "xfs_mkfs_config.h"
> > +
> > +#define CONFIG_MAX_KEY		1024
> > +#define CONFIG_MAX_VALUE	PATH_MAX
> > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > +
> > +/*
> > + * Enums for each configuration option. All these currently match the CLI
> > + * parameters for now but this may change later, so we keep all this code
> > + * and definitions separate. The rules for configuration parameters may also
> > + * differ.
> > + */
> 
> Why define them all when we are only going to parse a small subset?

I liked enums over random 1 characters which have no meaning for configuration
file parsing. I just needed an index symbol.

> > +enum mkfs_config_section {
> > +	MKFS_CONFIG_SECTION_BLOCK = 0,
> > +	MKFS_CONFIG_SECTION_DATA,
> > +	MKFS_CONFIG_SECTION_INODE,
> > +	MKFS_CONFIG_SECTION_LOG,
> > +	MKFS_CONFIG_SECTION_METADATA,
> > +	MKFS_CONFIG_SECTION_NAMING,
> > +	MKFS_CONFIG_SECTION_RTDEV,
> > +	MKFS_CONFIG_SECTION_SECTOR,
> > +
> > +	MKFS_CONFIG_SECTION_INVALID,
> > +};
> > +
> > +struct opt_params {
> > +	const enum mkfs_config_section section;
> > +	const char	*subopts[MAX_SUBOPTS];
> > +};
> > +
> > +extern struct opt_params config_sopts;
> 
> Why is this declared here?

Nuked.

> > +
> > +struct opt_params config_bopts = {
> > +	.section = 'b',
> 
> That's not an enum, looks broken. Why isn't this the ascii name
> of the config section in the config file?

Sorry, the entire .section crap can be removed. Its not needed. I had
already indexed the array with the above enums.

Yeap, it is not needed.

I forgot to nuke it as I tossed things which were not needed out the
window. I forgot the lamp.

> > +	.subopts = {
> > +		[B_SIZE] = "size",
> > +	},
> > +};
> > +
> > +struct opt_params config_dopts = {
> > +	.section = 'd',
> > +	.subopts = {
> > +		[D_AGCOUNT] = "agcount",
> > +		[D_FILE] = "file",
> > +		[D_NAME] = "name",
> > +		[D_SIZE] = "size",
> > +		[D_SUNIT] = "sunit",
> > +		[D_SWIDTH] = "swidth",
> > +		[D_AGSIZE] = "agsize",
> > +		[D_SU] = "su",
> > +		[D_SW] = "sw",
> > +		[D_SECTSIZE] = "sectsize",
> > +		[D_NOALIGN] = "noalign",
> > +		[D_RTINHERIT] = "rtinherit",
> > +		[D_PROJINHERIT] = "projinherit",
> > +		[D_EXTSZINHERIT] = "extszinherit",
> > +		[D_COWEXTSIZE] = "cowextsize",
> > +	},
> > +};
> 
> So the parser will accept names we can't configure?

No, it will not, it will depend on the individual parser to handle
it on its switch. Default is to return -EINVAL by each of them.

I could remove all unused ones, but figured there was no harm to keep
what we know exists.

I'll yield to whatever is desired.

> [...]
> 
> > +const char *default_type_str(enum default_params_type type)
> > +{
> > +	switch (type) {
> > +	case DEFAULTS_BUILTIN:
> > +		return _("package built-in definitions");
> > +	case DEFAULTS_CONFIG:
> > +		return _("default configuration system file");
> > +	case DEFAULTS_ENVIRONMENT_CONFIG:
> > +		return _("custom configuration file set by environment");
> > +	case DEFAULTS_TYPE_CONFIG:
> > +		return _("custom configuration type file");
> > +	}
> > +	return _("Unkown\n");
> > +}
> 
> This function can go away.

Well depends, if we keep the enum the no ?

> > +static int block_config_parser(struct mkfs_default_params *dft, int subopt,
> > +			       bool value)
> > +{
> > +	return -EINVAL;
> > +}
> 
> Why is the value passed to the parser a bool type and not a
> uint64_t?

Because we decided supporting bool right now is OK in my last iteration.
And be we, I mean Eric was fine with this as a first incarnation.

Passing the uint64_t is fine and make that change on the next iteration.

> > +struct config_subopts {
> > +	enum mkfs_config_section	section;
> > +	struct opt_params		*opts;
> > +	int (*config_parser)(struct mkfs_default_params *dft,
> > +			     int subop, bool value);
> > +} config_subopt_tab[] = {
> > +	{ MKFS_CONFIG_SECTION_BLOCK, &config_bopts, block_config_parser },
> > +	{ MKFS_CONFIG_SECTION_DATA, &config_dopts, data_config_parser },
> > +	{ MKFS_CONFIG_SECTION_INODE, &config_iopts, inode_config_parser },
> > +	{ MKFS_CONFIG_SECTION_LOG, &config_lopts, log_config_parser },
> > +	{ MKFS_CONFIG_SECTION_METADATA, &config_mopts, meta_config_parser },
> > +	{ MKFS_CONFIG_SECTION_NAMING, &config_nopts, naming_config_parser },
> > +	{ MKFS_CONFIG_SECTION_RTDEV, &config_ropts, rtdev_config_parser },
> > +	{ MKFS_CONFIG_SECTION_SECTOR, &config_sopts, sector_config_parser },
> > +	{ '\0', NULL },
> > +};
> > +
> > +static int
> > +parse_config_subopts(
> > +	enum mkfs_config_section	section,
> > +	bool				value,
> > +	char				*line,
> > +	struct mkfs_default_params	*dft)
> > +{
> > +	struct config_subopts *sop = &config_subopt_tab[0];
> > +	char	**subopts = (char **)sop->opts->subopts;
> > +	int	subopt;
> > +	char *val;
> > +
> > +	while (sop->opts) {
> > +		if (sop->section == section)
> > +			break;
> > +		sop++;
> > +	}
> > +
> > +	/* should never happen */
> > +	if (!sop->opts)
> > +		return -EINVAL;
> > +
> > +	subopts = (char **)sop->opts->subopts;
> > +	subopt = getsubopt(&line, subopts, &val);
> > +	return (sop->config_parser)(dft, subopt, value);
> > +}
> 
> This seems back to front - why do we walk the table to find the
> entry that corresponds to an enum when we've....

Yeah you're right this is stupid legacy crap, from the CLI stuff, we can
just do config_subopt_tab[section] provided its < MAX_SECTION  or whatever.

> > +static enum mkfs_config_section get_config_section(const char *buffer)
> > +{
> > +	if (strncmp("block", buffer, 5) == 0)
> > +		return MKFS_CONFIG_SECTION_BLOCK;
> 
> This will match "blocksdfsgdfgd" - strcmp is fine here.

OK!

> > +	if (strncmp("data", buffer, 4) == 0)
> > +		return MKFS_CONFIG_SECTION_DATA;
> > +	if (strncmp("inode", buffer, 5) == 0)
> > +		return MKFS_CONFIG_SECTION_INODE;
> > +	if (strncmp("log", buffer, 3) == 0)
> > +		return MKFS_CONFIG_SECTION_LOG;
> > +	if (strncmp("metadata", buffer, 8) == 0)
> > +		return MKFS_CONFIG_SECTION_METADATA;
> > +	if (strncmp("naming", buffer, 6) == 0)
> > +		return MKFS_CONFIG_SECTION_NAMING;
> > +	if (strncmp("rtdev", buffer, 5) == 0)
> > +		return MKFS_CONFIG_SECTION_RTDEV;
> > +	if (strncmp("sector", buffer, 6) == 0)
> > +		return MKFS_CONFIG_SECTION_SECTOR;
> > +
> > +	return MKFS_CONFIG_SECTION_INVALID;
> > +}
> 
> Directly parsed the section name to turn it into an enum? Indeed,
> this looks all wrong in terms of structure.

Given the above change I noted I should have done for the index, this
will give us a 1-1 mapping to that required array.

> static struct confopts blockopts = {
> 	.name = "[block]",
> 	.subopts = {
> 		[B_SIZE] = "size",
> 	},
> 	.parser = block_config_parser,
> 	.seen = false,
> };
> 
> struct confopts *
> get_confopts(const char *section)
> {
> 	if (strcmp(blockopts.name, section) == 0)
> 		return &blockopts;
> 	.....
> }

Yeah that works too.

> 	....
> 	opts = get_confopts(section);
> 	if (!opts)
> 		/* fail, invalid section */
> 	if (opts->seen)
> 		/* fail, multiple specification */

Good point.

> 	/* loop extracting and processing sbuopts */
> 
> 
> And now the whole section enum construct and the
> parse_config_subopts() function goes away.

Alright.

> > +static int mkfs_check_config_file_limits(const char *filename,
> > +					 const struct stat *sb,
> > +					 unsigned int max_entries)
> > +{
> > +	unsigned long long size_limit;
> > +
> > +	size_limit = CONFIG_MAX_BUFFER * max_entries;
> > +
> > +	/*
> > +	 * Detect wrap around -- if max_entries * size_limit goves over
> > +	 * unsigned long long we detect that there. Note some libraries,
> > +	 * like libiniconfig, only groks max INT_MAX entries anyway, so
> > +	 * if we ever use a library for parsing we'd be restricted to that
> > +	 * limit.
> > +	 */
> > +	if (size_limit < max_entries || max_entries > INT_MAX)
> > +		return -E2BIG;
> > +
> > +	if (sb->st_size > size_limit) {
> > +		fprintf(stderr,
> > +			"File %s is too big! File size limit: %llu\n",
> > +			filename, size_limit);
> > +		return -E2BIG;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> What is this supposed to protect again?

Just a sanity check, it would be silly to start processing more than we allow.

> If somone what to put lots
> of comments or blank space in the config files, then what's the
> problem with that? Seems like you jump through a lot of hoops to do
> just this calculation - why not just say "1MB is more than enough
> for anyone"?

Sure.

> > +enum parse_line_type {
> > +	PARSE_COMMENT = 0,
> > +	PARSE_SECTION,
> > +	PARSE_TAG_VALUE,
> > +	PARSE_INVALID,
> > +	PARSE_EOF,
> > +};
> 
> > +
> > +static int parse_line_section(const char *line, char *tag)
> 
> I should have mentioned this before now, but it's finally got to me.
> :) Function format for XFS code is:
> 
> static int
> parse_line_section(
> 	const char	*line,
> 	char		*tag)
> {
> ....

Alright...

> > +{
> > +	return sscanf(line, " [%[^]]s]", tag);
> > +}
> > +
> > +static int parse_line_tag_value(const char *line, char *tag,
> > +				unsigned int *value)
> > +{
> > +	return sscanf(line, " %[^ \t=]"
> > +		      " = "
> > +		      "%u ",
> > +		      tag, value);
> 
> Value can be a 64 bit quantity, so needs to be uint64_t....

Alright.

> > +}
> > +
> > +static enum parse_line_type parse_get_line_type(const char *line, char *tag,
> > +						bool *value)
> > +{
> > +	int ret;
> > +	unsigned int uint_value;

We're gonna want  uint64_t here too.

> > +
> > +	memset(tag, 0, 80);
> 
> Why? This should be done in the caller, and if there's a fixed
> buffer size, then please use a #define.
> 
> > +
> > +	if (strlen(line) < 2)
> > +		return PARSE_INVALID;
> 
> So empty lines return PARSE_INVALID?

Yes, but that's not fatal, we just discard it.

> > +
> > +	if (line[0] == '#')
> > +		return PARSE_COMMENT;
> 
> What about lines starting with whitespace? e.g. " # comment"

parse_line_section() below would not return 1 and its ignored
in the end as its also not a proper tag/value pair and the
section is not set.

> > +	ret = parse_line_section(line, tag);
> > +	if (ret == 1)
> > +		return PARSE_SECTION;
> > +
> > +	if (ret == EOF)
> > +		return PARSE_EOF;
> > +
> > +	ret = parse_line_tag_value(line, tag, &uint_value);
> > +	if (ret == 2) {
> > +		if (uint_value != 1 && uint_value != 0)
> > +			return -EINVAL;
> > +
> > +		*value = !!uint_value;
> > +
> > +		return PARSE_TAG_VALUE;
> > +	}
> 
> pass the full value back to the caller, let the subopt parser
> validate it and squash it to the correct type.

I wanted parse_get_line_type() to give me what type of line we
are dealing with. What you describe is still possible if I just
change the argument to uint64_t and then the parser squashes it.

> > +
> > +	if (ret == EOF)
> > +		return PARSE_EOF;
> > +
> > +	return PARSE_INVALID;
> > +}
> > +
> > +int parse_config_init(struct mkfs_default_params *dft)
> 
> int
> parse_config_file(
> 	struct mkfs_default_params	*dft,
> 	const char 			*config_file)
> {
> 
> i.e. we only get called if there's a config file configured.

Alright.

> > +{
> > +	int ret;
> > +	FILE *fp;
> > +	struct stat sp;
> > +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> > +					    sizeof(struct config_subopts) - 1;
> > +	char *p;
> > +	char line[80], tag[80];
> > +	bool value;
> > +	enum parse_line_type parse_type;
> > +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> > +
> > +	fp = fopen(dft->config_file, "r");
> > +	if (!fp) {
> > +		if (dft->type != DEFAULTS_BUILTIN) {
> > +			fprintf(stderr, _("could not open config file: %s\n"),
> > +				dft->config_file);
> > +			ret = -ENOENT;
> > +		} else
> > +			ret = 0;
> > +		return ret;
> > +	}
> 
> This should be split out into a separate function that takes the
> config file and tries to open it. If it's a relative path that was
> supplied, then this function can also try all the configured search
> paths for config files.

Eric asked to support 3 cases:

 a) ./ -- current working directory
 b) /  -- full path
 c) no prefix - we use the sysconfigdir/mkfs.d/

but sure, I'll toss it in there.

> > +	/*
> > +	 * If we found a file without it being specified on the command line,
> > +	 * it would be the default configuration file.
> > +	 */
> > +	if (dft->type == DEFAULTS_BUILTIN)
> > +		dft->type = DEFAULTS_CONFIG;
> > +
> > +	ret = stat(dft->config_file, &sp);
> > +	if (ret) {
> > +		if (dft->type != DEFAULTS_BUILTIN)
> > +			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
> > +				dft->config_file, strerror(ret));
> > +		return ret;
> > +	}
> 
> This dft->type futzing can all go away - if the file exists, the
> mkfs will already know where it came from.

Not convinced yet but I will shoot for removing it.

> > +
> > +	if (!sp.st_size)
> > +		return 0;
> > +
> > +	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
> > +					    num_subopts_sections);
> > +	if (ret)
> > +		return ret;
> > +
> > +	while (!feof(fp)) {
> > +		p = fgets(line, sizeof(line), fp);
> 
> What if the line is longer than 80 characters? e.g. a long comment

If its a comment or spaces with a comment, we still only care for
the first 80 characters, no?

> In that case, we need a line[79] = '/0';, or a memset of line to
> clear it before fgets is called. i think I prefer the latter...

I thought I memset() already - bleh, no, ok fixed, thanks!


> > +		if (!p)
> > +			continue;
> > +
> > +		parse_type = parse_get_line_type(line, tag, &value);
> > +
> > +		switch (parse_type) {
> > +		case PARSE_COMMENT:
> > +		case PARSE_INVALID:
> > +		case PARSE_EOF:
> > +			break;
> > +		case PARSE_SECTION:
> > +			section = get_config_section(tag);
> > +			if (!section) {
> > +				fprintf(stderr, _("Invalid section: %s\n"),
> > +						tag);
> > +				return -EINVAL;
> > +			}
> > +			break;
> > +		case PARSE_TAG_VALUE:
> > +			/* Trims white spaces */
> > +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> > +			ret = parse_config_subopts(section, value, line, dft);
> 
> We've already got the tag and value as discrete variables - why put
> them back into an ascii string to pass to another function for it to
> split them back into discrete variables?

The comment above it explains, "Trims white spaces"

> This really seems like it could be improved by making this less
> abstract. i.e.
> 
> 	while (!feof(fp)) {
> 		memset(line, 0, sizeof(line));
> 		p = fgets(line, sizeof(line), fp);
> 		if (!p)
> 			continue;
> 
> 		token = parse_section(line);
> 		if (token) {
> 			opts = get_confopts(section);
> 			if (!opts)
> 				/* fail, invalid */;
> 			if (opts->seen)
> 				/* fail, respec */ ;
> 		}
> 		if (!opts)
> 			continue;
> 
> 		token = parse_value(line, &value);
> 		if (!token)
> 			continue;
> 		error = opts->parse(opts, token, value);
> 		if (error) {
> 			/* warn, fail, ignore? */ ;
> 		}
> 	}
> 
> I suspect with this even all the B_SIZE type of enums can go away,
> too, because all we need to do is string compares of the token
> directly in the suboption parser to know what option we are about to
> set....

I'll give it a shot.

> 
> > diff --git a/mkfs/xfs_mkfs_config.h b/mkfs/xfs_mkfs_config.h
> > new file mode 100644
> > index 000000000000..407d37fffb1a
> > --- /dev/null
> > +++ b/mkfs/xfs_mkfs_config.h
> > @@ -0,0 +1,11 @@
> > +#ifndef _XFS_MKFS_CONFIG_H
> > +#define _XFS_MKFS_CONFIG_H
> > +
> > +#include "xfs_mkfs_common.h"
> > +
> > +#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d/"
> 
> This can go in the C file, as there's no need for anything other
> than the file open routine to know this.

Sure.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner May 20, 2018, 12:16 a.m. UTC | #10

On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> > On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> > > You may want to stick to specific set of configuration options when
> > > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > > reasons, but some other times to ensure systems remain compatible as
> > > new features are introduced with older kernels, or if you always want
> > > to take advantage of some new feature which would otherwise typically
> > > be disruptive.
> > > 
> > > This adds support for parsing a configuration file to override defaults
> > > parameters to be used for mkfs.xfs.
> > > 
> > > We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> > > different configuration files, if none is specified we look for the
> > > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > > with -c.  For instance, if you specify:
> > > 
> > > 	mkfs.xfs -c experimental -f /dev/loop0
> > > 
> > > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > > file. If you really need to override the full path of the configuration
> > > file you may use the MKFS_XFS_CONFIG environment variable.
> > 
> > I'm swamped under a deadline at work this week so just commenting at a
> > very high level for now, but I'm curious; why use an env var vs
> > providing a full path for -c ?  env vars always strike me as magic unexpected
> > behaviors.
> > 
> > # mkfs.xfs -c /my/fancy/path/to/config
> > 
> > seems much clearer than
> > 
> > # export MKFS_XFS_CONFIG=/my/fancy/path/to/
> > # mkfs.xfs -c config
> > 
> > i.e. if a full path is specified use it, else use the config directory.
> > 
> > Thoughts?
> 
> In this case your choices are:
> MKFS_XFS_CONFIG=/my/fancy/path/to/config mkfs.xfs
> 
> or:
> cp /my/fancy/path/to/config /etc/mkfs.xfs.d/hoogah
> mkfs.xfs -c hooga

Sorry, why do we need to copy it to /etc/mkfs.xfs.d/?

> <shrug> Bikeshedding more, what if either option accepted either an
> absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ? 

I kinda assumed that config files could be located anywhere, but we
only searched the sysconfig path if it didn't point at a local
file...

Cheers,

Dave.

Dave Chinner May 21, 2018, 12:14 a.m. UTC | #11

On Sat, May 19, 2018 at 03:32:24AM +0200, Luis R. Rodriguez wrote:
> On Fri, May 18, 2018 at 10:44:04AM +1000, Dave Chinner wrote:
> > On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > > --- a/mkfs/xfs_mkfs.c
> > > +++ b/mkfs/xfs_mkfs.c
> > > @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
> > >  	struct mkfs_params	*cfg,
> > >  	char			*dfile,
> > >  	char			*logfile,
> > > -	char			*rtfile)
> > > +	char			*rtfile,
> > > +	struct mkfs_default_params *dft)
> > >  {
> > >  	struct sb_feat_args	*fp = &cfg->sb_feat;
> > >  
> > >  	printf(_(
> > > +"config-file=%-22s\n"
> > >  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> > >  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> > >  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> > > @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
> > >  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> > >  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> > >  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> > > +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
> > >  		dfile, cfg->inodesize, (long long)cfg->agcount,
> > >  			(long long)cfg->agsize,
> > >  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
> > 
> > Haven't we already printed where the config was sourced from? Why do
> > we need to print it again here?
> 
> The other print describes the enum, this print prints out the *used*
> config file path if a config file was actually used. Without the other
> print this would just print either the config file or empty.

If you look at the changes I proposed, I suggested we change the
initial print out the path of the source file, not how the user
specified the source file. So this becomes redundant. And given this
code is now shared with xfs_info, it has no idea about mkfs config
files, so it's not ideal to be adding mkfs-specific stuff to this
output.

Further....

> Since we are going to remove the environment variable option, then
> I think the other print is now not needed anymore and my preference
> is to keep it here.
> 
> Thoughts?

.... this printout only happens after al input has been validated
and we're about to make the filesystem.  If there's a validation
problem, nobody knows what file the defaults were sourced from.
IOWs, the file the default are sourced from needs to be printed even
before the file is read...

> > >  	.sectorsize = XFS_MIN_SECTORSIZE,
> > >  	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
> > >  	.sb_feat = {
> > > @@ -3714,6 +3719,13 @@ static void process_defaults(
> > >  	struct mkfs_default_params	*dft,
> > >  	struct cli_params		*cli)
> > >  {
> > > +	int ret;
> > > +
> > > +	ret = parse_config_init(dft);
> > > +
> > > +	if (ret && dft->type != DEFAULTS_BUILTIN)
> > > +		usage();
> > 
> > That doesn't make any sense in isolation.
> 
> Not sure what you meant at first, but I see that you prefer this
> to be hidden from the caller. Will change.

I meant I have no idea what this is checking and why there's a usage
call there because usage() is for CLI input, not setting up
defaults before CLI processing. Explanation in comments are needed.

> > >  	install_defaults(dft, cli);
> > >  }
> > >  
> > > @@ -3750,6 +3762,8 @@ main(
> > >  	};
> > >  	struct mkfs_params	cfg = {};
> > >  	struct mkfs_default_params	dft;
> > > +	char *tmp_config;
> > > +	char custom_config[PATH_MAX];
> > >  
> > >  	reset_defaults_and_cli(&dft, &cli);
> > >  
> > > @@ -3760,21 +3774,36 @@ main(
> > >  	textdomain(PACKAGE);
> > >  
> > >  	/*
> > > -	 * TODO: Sourcing defaults from a config file
> > > -	 *
> > >  	 * Before anything else, see if there's a config file with different
> > > -	 * defaults. If a file exists in <package location>, read in the new
> > > +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
> > >  	 * default values and overwrite them in the &dft structure. This way the
> > >  	 * new defaults will apply before we parse the CLI, and the CLI will
> > >  	 * still be able to override them. When more than one source is
> > >  	 * implemented, emit a message to indicate where the defaults being
> > >  	 * used came from.
> > >  	 */
> > 
> > This comment makes no mention of environment variables, or how they
> > interact with other sources of config files.
> 
> As per Eric's request, We're getting rid of the environment variable option, so
> mentioning it is no longer needed.

I thought Darrick wanted it kept?

> > > +	tmp_config = getenv("MKFS_XFS_CONFIG");
> > > +	if (tmp_config != NULL) {
> > > +		dft.config_file = tmp_config;
> > > +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> > > +	}
> > >  
> > >  	process_defaults(&dft, &cli);
> > 
> > So why are we processing the defaults before we've checked if a
> > default file is specified on the command line? All this should do is
> > set up the config file to be read, and set the dft.source =
> > _("Environment Variable");
> 
> This was being done *here* since I previously has setup to process the
> arguments *early* and check if a -c was specified, and if so use the config
> file for the defaults, otherwise the environment variable if set. But since
> the way I had implemented processing the arguments early relied on not well
> guaranteed mechanisms I resorted to avoid that approach.
> 
> This was the alternative I came up with.
> 
> The environment variable stuff is going away so this should be much simpler
> now, however the question of argument processing early still remains.
> 
> > >  
> > > -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > > +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > >  		switch (c) {
> > > +		case 'c':
> > > +			reset_defaults_and_cli(&dft, &cli);
> > 
> > This will trash the existing CLI inputs that have already been
> > parsed. 
> 
> Exactly.
> 
> > All this code here should do is set up the config file
> > to be read.
> 
> Consider the case of /etc/mkfs.d/default existing and it having
> 
> [metadata]
> crc = 1
> [naming]
> ftype=1
> 
> And suppose the user passed -c foo which had:
> 
> [metadata]
> crc = 0
> 
> Without the reset we'd make /etc/mkfs.d/default the system wide defaults onto
> which -c parameters overlay on top of. I figured that was not desirable.

It's not desirable. If the user specified a config file, then
/etc/mkfs.d/default *is never read*. The user config file is used
instead. If the user supplied file does not exist (even after
searching) then we fail, not use some other set of defaults.

> > Again, what happens if the user specifies multiple config files?
> 
> The reset strategy lets the user set -c as many times as they wish and also
> starts from a clean base always.

multiple "-c" options is a user error and should error out.

> > The second config file will trash everything from the first, as well
> > as any other options between them.
> 
> Yes! By design as we couldn't easily parse arguments early first and then
> choose just one strategy.
>
> I liked the simplicity that this brings.
> 
> Would you really want multiple -c options to work as overlays, one on top of
> the other?

I don't want multiple -c option to be supported at all. We source
defaults from a single file only - either the user specified file,
or the default if it exists. Not both, not multiple user specified
files. One file.

> > I think that we need to pull the "-c <file>" option from the command
> > line and process all the defaults /before/ we enter the main command
> > line processing loop.
> 
> That is what I had done originally but ran into that snag of processing
> arguments twice and the undocumented functionality I found that worked.

Multiple pass option parsing is documented as supported and has been
for a long, long time. And to make that clear, we even have a
wrapper in xfsprogs for it:

$ git grep platform_getoptreset
db/command.c:   platform_getoptreset();
include/darwin.h:static __inline__ void platform_getoptreset(void)
include/freebsd.h:static __inline__ void platform_getoptreset(void)
include/gnukfreebsd.h:static __inline__ void platform_getoptreset(void)
include/linux.h:static __inline__ void platform_getoptreset(void)
libxcmd/command.c:      platform_getoptreset();

And we use it in libxcmd so that each function in xfs_io, xfs_db,
xfs_quota, etc can run their own sub-command getopt calls correctly.
It was in the initial commits for xfs_db back in 2001, so we've been
using multiple pass CLI option parsing in xfsprogs since 2001....

> > IOWs:
> > 	config_file = getenv("MKFS_XFS_CONFIG");
> > 	if (config_file)
> > 		dft.source = _("Environment Variable");
> > 
> > 	/*
> > 	 * Pull config line options from command line
> > 	 */
> > 	while ((c = getopt(argc, argv, "c:")) != EOF) {
> > 		switch (c) {
> > 		case 'c':
> > 			/* XXX: fail if already seen a CLI option */
> 
> BTW isn't my enum here sufficient to check for this easily in a clean
> short and easy way?

A local boolean variable is all that is ncessary for this....

[....]

> > 	memcpy(/* sb_feats */);
> > 	memcpy(/* fsxattr */);
> > 	optind = 1;
> 
> My getopt(3) *does* not make mention of any of this. This man page however does:

It's been there as long as I can remember. 2nd paragraph of the
description:

$man 3 getopt
[...]
DESCRIPTION
[...]
       The variable optind is the index of the next element to be
       processed in argv.  The system initializes this value to  1.
       The  caller  can reset it to 1 to restart scanning of the
       same argv, or when scanning a new argument vector.


> optind = 1;
> 
> However perhaps a big fat NOTE is worthy?

Why? It's documented, well known behaviour....

> > [...]
> > 
> > > +const char *default_type_str(enum default_params_type type)
> > > +{
> > > +	switch (type) {
> > > +	case DEFAULTS_BUILTIN:
> > > +		return _("package built-in definitions");
> > > +	case DEFAULTS_CONFIG:
> > > +		return _("default configuration system file");
> > > +	case DEFAULTS_ENVIRONMENT_CONFIG:
> > > +		return _("custom configuration file set by environment");
> > > +	case DEFAULTS_TYPE_CONFIG:
> > > +		return _("custom configuration type file");
> > > +	}
> > > +	return _("Unkown\n");
> > > +}
> > 
> > This function can go away.
> 
> Well depends, if we keep the enum the no ?

That enum needs to die. It's unnecssary abstraction.

> > > +	if (strlen(line) < 2)
> > > +		return PARSE_INVALID;
> > 
> > So empty lines return PARSE_INVALID?
> 
> Yes, but that's not fatal, we just discard it.

That's "ignore", like comments, not "invalid".

> 
> > > +
> > > +	if (line[0] == '#')
> > > +		return PARSE_COMMENT;
> > 
> > What about lines starting with whitespace? e.g. " # comment"
> 
> parse_line_section() below would not return 1 and its ignored
> in the end as its also not a proper tag/value pair and the
> section is not set.

So it silently ignores a valid sections/{tag/value pair} if there's
trailing stuff on the line? Shouldn't that throw an error?

> > > +{
> > > +	int ret;
> > > +	FILE *fp;
> > > +	struct stat sp;
> > > +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> > > +					    sizeof(struct config_subopts) - 1;
> > > +	char *p;
> > > +	char line[80], tag[80];
> > > +	bool value;
> > > +	enum parse_line_type parse_type;
> > > +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> > > +
> > > +	fp = fopen(dft->config_file, "r");
> > > +	if (!fp) {
> > > +		if (dft->type != DEFAULTS_BUILTIN) {
> > > +			fprintf(stderr, _("could not open config file: %s\n"),
> > > +				dft->config_file);
> > > +			ret = -ENOENT;
> > > +		} else
> > > +			ret = 0;
> > > +		return ret;
> > > +	}
> > 
> > This should be split out into a separate function that takes the
> > config file and tries to open it. If it's a relative path that was
> > supplied, then this function can also try all the configured search
> > paths for config files.
> 
> Eric asked to support 3 cases:
> 
>  a) ./ -- current working directory
>  b) /  -- full path
>  c) no prefix - we use the sysconfigdir/mkfs.d/

That's pretty much what I just suggested. :)

> > > +		return ret;
> > > +
> > > +	while (!feof(fp)) {
> > > +		p = fgets(line, sizeof(line), fp);
> > 
> > What if the line is longer than 80 characters? e.g. a long comment
> 
> If its a comment or spaces with a comment, we still only care for
> the first 80 characters, no?

What does fgets() do with the remaining characters on the line? They
are still parked in the input stream, so won't the next fgets() call
read them? And then we parse those bytes as if they are a new config
line?

IOWs, shouldn't we be using a line-based input function here? Say,
getline(3)?

> > > +		if (!p)
> > > +			continue;
> > > +
> > > +		parse_type = parse_get_line_type(line, tag, &value);
> > > +
> > > +		switch (parse_type) {
> > > +		case PARSE_COMMENT:
> > > +		case PARSE_INVALID:
> > > +		case PARSE_EOF:
> > > +			break;
> > > +		case PARSE_SECTION:
> > > +			section = get_config_section(tag);
> > > +			if (!section) {
> > > +				fprintf(stderr, _("Invalid section: %s\n"),
> > > +						tag);
> > > +				return -EINVAL;
> > > +			}
> > > +			break;
> > > +		case PARSE_TAG_VALUE:
> > > +			/* Trims white spaces */
> > > +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> > > +			ret = parse_config_subopts(section, value, line, dft);
> > 
> > We've already got the tag and value as discrete variables - why put
> > them back into an ascii string to pass to another function for it to
> > split them back into discrete variables?
> 
> The comment above it explains, "Trims white spaces"

Why do we need to do that? Doesn't the token parser trim away excess
whitespace around each token it returns?

Cheers,

Dave.

Jan Tulak May 21, 2018, 9:40 a.m. UTC | #12

On Sat, May 19, 2018 at 1:56 AM, Luis R. Rodriguez <[e-mail]> wrote:
> On Fri, May 18, 2018 at 12:09:23PM -0500, Eric Sandeen wrote:
...
>>
>> mkfs.xfs -c hoogah            searches /@sysconfigdir@/mkfs.xfs.d/
>> mkfs.xfs -c ./hoogah          or
>> mkfs.xfs -c /path/to/hoogah   do exactly what you'd expect.
>>
...
>
> Sounds good. I'll drop the environment variable junk and only support
> the above 3 cases.
>

mkfs.xfs -c ../hoogah too, please. :-)

Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong May 21, 2018, 3:30 p.m. UTC | #13

On Mon, May 21, 2018 at 10:14:34AM +1000, Dave Chinner wrote:
> On Sat, May 19, 2018 at 03:32:24AM +0200, Luis R. Rodriguez wrote:
> > On Fri, May 18, 2018 at 10:44:04AM +1000, Dave Chinner wrote:
> > > On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > > > --- a/mkfs/xfs_mkfs.c
> > > > +++ b/mkfs/xfs_mkfs.c
> > > > @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
> > > >  	struct mkfs_params	*cfg,
> > > >  	char			*dfile,
> > > >  	char			*logfile,
> > > > -	char			*rtfile)
> > > > +	char			*rtfile,
> > > > +	struct mkfs_default_params *dft)
> > > >  {
> > > >  	struct sb_feat_args	*fp = &cfg->sb_feat;
> > > >  
> > > >  	printf(_(
> > > > +"config-file=%-22s\n"
> > > >  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> > > >  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> > > >  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> > > > @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
> > > >  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> > > >  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> > > >  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> > > > +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
> > > >  		dfile, cfg->inodesize, (long long)cfg->agcount,
> > > >  			(long long)cfg->agsize,
> > > >  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
> > > 
> > > Haven't we already printed where the config was sourced from? Why do
> > > we need to print it again here?
> > 
> > The other print describes the enum, this print prints out the *used*
> > config file path if a config file was actually used. Without the other
> > print this would just print either the config file or empty.
> 
> If you look at the changes I proposed, I suggested we change the
> initial print out the path of the source file, not how the user
> specified the source file. So this becomes redundant. And given this
> code is now shared with xfs_info, it has no idea about mkfs config
> files, so it's not ideal to be adding mkfs-specific stuff to this
> output.
> 
> Further....
> 
> > Since we are going to remove the environment variable option, then
> > I think the other print is now not needed anymore and my preference
> > is to keep it here.
> > 
> > Thoughts?
> 
> .... this printout only happens after al input has been validated
> and we're about to make the filesystem.  If there's a validation
> problem, nobody knows what file the defaults were sourced from.
> IOWs, the file the default are sourced from needs to be printed even
> before the file is read...
> 
> > > >  	.sectorsize = XFS_MIN_SECTORSIZE,
> > > >  	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
> > > >  	.sb_feat = {
> > > > @@ -3714,6 +3719,13 @@ static void process_defaults(
> > > >  	struct mkfs_default_params	*dft,
> > > >  	struct cli_params		*cli)
> > > >  {
> > > > +	int ret;
> > > > +
> > > > +	ret = parse_config_init(dft);
> > > > +
> > > > +	if (ret && dft->type != DEFAULTS_BUILTIN)
> > > > +		usage();
> > > 
> > > That doesn't make any sense in isolation.
> > 
> > Not sure what you meant at first, but I see that you prefer this
> > to be hidden from the caller. Will change.
> 
> I meant I have no idea what this is checking and why there's a usage
> call there because usage() is for CLI input, not setting up
> defaults before CLI processing. Explanation in comments are needed.
> 
> > > >  	install_defaults(dft, cli);
> > > >  }
> > > >  
> > > > @@ -3750,6 +3762,8 @@ main(
> > > >  	};
> > > >  	struct mkfs_params	cfg = {};
> > > >  	struct mkfs_default_params	dft;
> > > > +	char *tmp_config;
> > > > +	char custom_config[PATH_MAX];
> > > >  
> > > >  	reset_defaults_and_cli(&dft, &cli);
> > > >  
> > > > @@ -3760,21 +3774,36 @@ main(
> > > >  	textdomain(PACKAGE);
> > > >  
> > > >  	/*
> > > > -	 * TODO: Sourcing defaults from a config file
> > > > -	 *
> > > >  	 * Before anything else, see if there's a config file with different
> > > > -	 * defaults. If a file exists in <package location>, read in the new
> > > > +	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
> > > >  	 * default values and overwrite them in the &dft structure. This way the
> > > >  	 * new defaults will apply before we parse the CLI, and the CLI will
> > > >  	 * still be able to override them. When more than one source is
> > > >  	 * implemented, emit a message to indicate where the defaults being
> > > >  	 * used came from.
> > > >  	 */
> > > 
> > > This comment makes no mention of environment variables, or how they
> > > interact with other sources of config files.
> > 
> > As per Eric's request, We're getting rid of the environment variable option, so
> > mentioning it is no longer needed.
> 
> I thought Darrick wanted it kept?

Eric and I argued about this for a while on irc, I think we settled on
allowing a single '-c $foo' arg where we parse openat($foo)...

> > > > +	tmp_config = getenv("MKFS_XFS_CONFIG");
> > > > +	if (tmp_config != NULL) {
> > > > +		dft.config_file = tmp_config;
> > > > +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> > > > +	}
> > > >  
> > > >  	process_defaults(&dft, &cli);
> > > 
> > > So why are we processing the defaults before we've checked if a
> > > default file is specified on the command line? All this should do is
> > > set up the config file to be read, and set the dft.source =
> > > _("Environment Variable");
> > 
> > This was being done *here* since I previously has setup to process the
> > arguments *early* and check if a -c was specified, and if so use the config
> > file for the defaults, otherwise the environment variable if set. But since
> > the way I had implemented processing the arguments early relied on not well
> > guaranteed mechanisms I resorted to avoid that approach.
> > 
> > This was the alternative I came up with.
> > 
> > The environment variable stuff is going away so this should be much simpler
> > now, however the question of argument processing early still remains.
> > 
> > > >  
> > > > -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > > > +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > > >  		switch (c) {
> > > > +		case 'c':
> > > > +			reset_defaults_and_cli(&dft, &cli);
> > > 
> > > This will trash the existing CLI inputs that have already been
> > > parsed. 
> > 
> > Exactly.
> > 
> > > All this code here should do is set up the config file
> > > to be read.
> > 
> > Consider the case of /etc/mkfs.d/default existing and it having
> > 
> > [metadata]
> > crc = 1
> > [naming]
> > ftype=1
> > 
> > And suppose the user passed -c foo which had:
> > 
> > [metadata]
> > crc = 0
> > 
> > Without the reset we'd make /etc/mkfs.d/default the system wide defaults onto
> > which -c parameters overlay on top of. I figured that was not desirable.
> 
> It's not desirable. If the user specified a config file, then
> /etc/mkfs.d/default *is never read*. The user config file is used
> instead. If the user supplied file does not exist (even after
> searching) then we fail, not use some other set of defaults.

Yeah.  Less confusing to trace where a particular knobturn came from if
we only ever parse one config file.

--D

> > > Again, what happens if the user specifies multiple config files?
> > 
> > The reset strategy lets the user set -c as many times as they wish and also
> > starts from a clean base always.
> 
> multiple "-c" options is a user error and should error out.
> 
> > > The second config file will trash everything from the first, as well
> > > as any other options between them.
> > 
> > Yes! By design as we couldn't easily parse arguments early first and then
> > choose just one strategy.
> >
> > I liked the simplicity that this brings.
> > 
> > Would you really want multiple -c options to work as overlays, one on top of
> > the other?
> 
> I don't want multiple -c option to be supported at all. We source
> defaults from a single file only - either the user specified file,
> or the default if it exists. Not both, not multiple user specified
> files. One file.
> 
> > > I think that we need to pull the "-c <file>" option from the command
> > > line and process all the defaults /before/ we enter the main command
> > > line processing loop.
> > 
> > That is what I had done originally but ran into that snag of processing
> > arguments twice and the undocumented functionality I found that worked.
> 
> Multiple pass option parsing is documented as supported and has been
> for a long, long time. And to make that clear, we even have a
> wrapper in xfsprogs for it:
> 
> $ git grep platform_getoptreset
> db/command.c:   platform_getoptreset();
> include/darwin.h:static __inline__ void platform_getoptreset(void)
> include/freebsd.h:static __inline__ void platform_getoptreset(void)
> include/gnukfreebsd.h:static __inline__ void platform_getoptreset(void)
> include/linux.h:static __inline__ void platform_getoptreset(void)
> libxcmd/command.c:      platform_getoptreset();
> 
> And we use it in libxcmd so that each function in xfs_io, xfs_db,
> xfs_quota, etc can run their own sub-command getopt calls correctly.
> It was in the initial commits for xfs_db back in 2001, so we've been
> using multiple pass CLI option parsing in xfsprogs since 2001....
> 
> > > IOWs:
> > > 	config_file = getenv("MKFS_XFS_CONFIG");
> > > 	if (config_file)
> > > 		dft.source = _("Environment Variable");
> > > 
> > > 	/*
> > > 	 * Pull config line options from command line
> > > 	 */
> > > 	while ((c = getopt(argc, argv, "c:")) != EOF) {
> > > 		switch (c) {
> > > 		case 'c':
> > > 			/* XXX: fail if already seen a CLI option */
> > 
> > BTW isn't my enum here sufficient to check for this easily in a clean
> > short and easy way?
> 
> A local boolean variable is all that is ncessary for this....
> 
> [....]
> 
> > > 	memcpy(/* sb_feats */);
> > > 	memcpy(/* fsxattr */);
> > > 	optind = 1;
> > 
> > My getopt(3) *does* not make mention of any of this. This man page however does:
> 
> It's been there as long as I can remember. 2nd paragraph of the
> description:
> 
> $man 3 getopt
> [...]
> DESCRIPTION
> [...]
>        The variable optind is the index of the next element to be
>        processed in argv.  The system initializes this value to  1.
>        The  caller  can reset it to 1 to restart scanning of the
>        same argv, or when scanning a new argument vector.
> 
> 
> > optind = 1;
> > 
> > However perhaps a big fat NOTE is worthy?
> 
> Why? It's documented, well known behaviour....
> 
> > > [...]
> > > 
> > > > +const char *default_type_str(enum default_params_type type)
> > > > +{
> > > > +	switch (type) {
> > > > +	case DEFAULTS_BUILTIN:
> > > > +		return _("package built-in definitions");
> > > > +	case DEFAULTS_CONFIG:
> > > > +		return _("default configuration system file");
> > > > +	case DEFAULTS_ENVIRONMENT_CONFIG:
> > > > +		return _("custom configuration file set by environment");
> > > > +	case DEFAULTS_TYPE_CONFIG:
> > > > +		return _("custom configuration type file");
> > > > +	}
> > > > +	return _("Unkown\n");
> > > > +}
> > > 
> > > This function can go away.
> > 
> > Well depends, if we keep the enum the no ?
> 
> That enum needs to die. It's unnecssary abstraction.
> 
> > > > +	if (strlen(line) < 2)
> > > > +		return PARSE_INVALID;
> > > 
> > > So empty lines return PARSE_INVALID?
> > 
> > Yes, but that's not fatal, we just discard it.
> 
> That's "ignore", like comments, not "invalid".
> 
> > 
> > > > +
> > > > +	if (line[0] == '#')
> > > > +		return PARSE_COMMENT;
> > > 
> > > What about lines starting with whitespace? e.g. " # comment"
> > 
> > parse_line_section() below would not return 1 and its ignored
> > in the end as its also not a proper tag/value pair and the
> > section is not set.
> 
> So it silently ignores a valid sections/{tag/value pair} if there's
> trailing stuff on the line? Shouldn't that throw an error?
> 
> > > > +{
> > > > +	int ret;
> > > > +	FILE *fp;
> > > > +	struct stat sp;
> > > > +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> > > > +					    sizeof(struct config_subopts) - 1;
> > > > +	char *p;
> > > > +	char line[80], tag[80];
> > > > +	bool value;
> > > > +	enum parse_line_type parse_type;
> > > > +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> > > > +
> > > > +	fp = fopen(dft->config_file, "r");
> > > > +	if (!fp) {
> > > > +		if (dft->type != DEFAULTS_BUILTIN) {
> > > > +			fprintf(stderr, _("could not open config file: %s\n"),
> > > > +				dft->config_file);
> > > > +			ret = -ENOENT;
> > > > +		} else
> > > > +			ret = 0;
> > > > +		return ret;
> > > > +	}
> > > 
> > > This should be split out into a separate function that takes the
> > > config file and tries to open it. If it's a relative path that was
> > > supplied, then this function can also try all the configured search
> > > paths for config files.
> > 
> > Eric asked to support 3 cases:
> > 
> >  a) ./ -- current working directory
> >  b) /  -- full path
> >  c) no prefix - we use the sysconfigdir/mkfs.d/
> 
> That's pretty much what I just suggested. :)
> 
> > > > +		return ret;
> > > > +
> > > > +	while (!feof(fp)) {
> > > > +		p = fgets(line, sizeof(line), fp);
> > > 
> > > What if the line is longer than 80 characters? e.g. a long comment
> > 
> > If its a comment or spaces with a comment, we still only care for
> > the first 80 characters, no?
> 
> What does fgets() do with the remaining characters on the line? They
> are still parked in the input stream, so won't the next fgets() call
> read them? And then we parse those bytes as if they are a new config
> line?
> 
> IOWs, shouldn't we be using a line-based input function here? Say,
> getline(3)?
> 
> > > > +		if (!p)
> > > > +			continue;
> > > > +
> > > > +		parse_type = parse_get_line_type(line, tag, &value);
> > > > +
> > > > +		switch (parse_type) {
> > > > +		case PARSE_COMMENT:
> > > > +		case PARSE_INVALID:
> > > > +		case PARSE_EOF:
> > > > +			break;
> > > > +		case PARSE_SECTION:
> > > > +			section = get_config_section(tag);
> > > > +			if (!section) {
> > > > +				fprintf(stderr, _("Invalid section: %s\n"),
> > > > +						tag);
> > > > +				return -EINVAL;
> > > > +			}
> > > > +			break;
> > > > +		case PARSE_TAG_VALUE:
> > > > +			/* Trims white spaces */
> > > > +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> > > > +			ret = parse_config_subopts(section, value, line, dft);
> > > 
> > > We've already got the tag and value as discrete variables - why put
> > > them back into an ascii string to pass to another function for it to
> > > split them back into discrete variables?
> > 
> > The comment above it explains, "Trims white spaces"
> 
> Why do we need to do that? Doesn't the token parser trim away excess
> whitespace around each token it returns?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong May 21, 2018, 3:33 p.m. UTC | #14

On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
> On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> > On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> > > On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> > > > You may want to stick to specific set of configuration options when
> > > > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > > > reasons, but some other times to ensure systems remain compatible as
> > > > new features are introduced with older kernels, or if you always want
> > > > to take advantage of some new feature which would otherwise typically
> > > > be disruptive.
> > > > 
> > > > This adds support for parsing a configuration file to override defaults
> > > > parameters to be used for mkfs.xfs.
> > > > 
> > > > We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> > > > different configuration files, if none is specified we look for the
> > > > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > > > with -c.  For instance, if you specify:
> > > > 
> > > > 	mkfs.xfs -c experimental -f /dev/loop0
> > > > 
> > > > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > > > file. If you really need to override the full path of the configuration
> > > > file you may use the MKFS_XFS_CONFIG environment variable.
> > > 
> > > I'm swamped under a deadline at work this week so just commenting at a
> > > very high level for now, but I'm curious; why use an env var vs
> > > providing a full path for -c ?  env vars always strike me as magic unexpected
> > > behaviors.
> > > 
> > > # mkfs.xfs -c /my/fancy/path/to/config
> > > 
> > > seems much clearer than
> > > 
> > > # export MKFS_XFS_CONFIG=/my/fancy/path/to/
> > > # mkfs.xfs -c config
> > > 
> > > i.e. if a full path is specified use it, else use the config directory.
> > > 
> > > Thoughts?
> > 
> > In this case your choices are:
> > MKFS_XFS_CONFIG=/my/fancy/path/to/config mkfs.xfs
> > 
> > or:
> > cp /my/fancy/path/to/config /etc/mkfs.xfs.d/hoogah
> > mkfs.xfs -c hooga
> 
> Sorry, why do we need to copy it to /etc/mkfs.xfs.d/?

No particular reason, just showing off relative pathname interpretation.
That could have been

mkfs.xfs -c ../../my/fancy/path/to/config

But that's kinda ugly.  $sysconfdir seems to get set to /usr on my
local builds, though I think dpkg overrides that back to /... :)

> > <shrug> Bikeshedding more, what if either option accepted either an
> > absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ? 
> 
> I kinda assumed that config files could be located anywhere, but we
> only searched the sysconfig path if it didn't point at a local
> file...

<shrug> openat() semantics are fine enough with me, I think.

--D

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> [e-mail]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 21, 2018, 4:58 p.m. UTC | #15

On Mon, May 21, 2018 at 10:14:34AM +1000, Dave Chinner wrote:
> On Sat, May 19, 2018 at 03:32:24AM +0200, Luis R. Rodriguez wrote:
> > On Fri, May 18, 2018 at 10:44:04AM +1000, Dave Chinner wrote:
> > > On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > > > --- a/mkfs/xfs_mkfs.c
> > > > +++ b/mkfs/xfs_mkfs.c
> > > > @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
> > > >  	struct mkfs_params	*cfg,
> > > >  	char			*dfile,
> > > >  	char			*logfile,
> > > > -	char			*rtfile)
> > > > +	char			*rtfile,
> > > > +	struct mkfs_default_params *dft)
> > > >  {
> > > >  	struct sb_feat_args	*fp = &cfg->sb_feat;
> > > >  
> > > >  	printf(_(
> > > > +"config-file=%-22s\n"
> > > >  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> > > >  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> > > >  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> > > > @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
> > > >  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> > > >  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> > > >  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> > > > +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
> > > >  		dfile, cfg->inodesize, (long long)cfg->agcount,
> > > >  			(long long)cfg->agsize,
> > > >  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
> > > 
> > > Haven't we already printed where the config was sourced from? Why do
> > > we need to print it again here?
> > 
> > The other print describes the enum, this print prints out the *used*
> > config file path if a config file was actually used. Without the other
> > print this would just print either the config file or empty.
> 
> If you look at the changes I proposed, I suggested we change the
> initial print out the path of the source file, not how the user
> specified the source file. So this becomes redundant. And given this
> code is now shared with xfs_info, it has no idea about mkfs config
> files, so it's not ideal to be adding mkfs-specific stuff to this
> output.

Alright.

> Further....
> 
> > Since we are going to remove the environment variable option, then
> > I think the other print is now not needed anymore and my preference
> > is to keep it here.
> > 
> > Thoughts?
> 
> .... this printout only happens after al input has been validated
> and we're about to make the filesystem.  If there's a validation
> problem, nobody knows what file the defaults were sourced from.
> IOWs, the file the default are sourced from needs to be printed even
> before the file is read...

Good point.

> > > > +	tmp_config = getenv("MKFS_XFS_CONFIG");
> > > > +	if (tmp_config != NULL) {
> > > > +		dft.config_file = tmp_config;
> > > > +		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
> > > > +	}
> > > >  
> > > >  	process_defaults(&dft, &cli);
> > > 
> > > So why are we processing the defaults before we've checked if a
> > > default file is specified on the command line? All this should do is
> > > set up the config file to be read, and set the dft.source =
> > > _("Environment Variable");
> > 
> > This was being done *here* since I previously has setup to process the
> > arguments *early* and check if a -c was specified, and if so use the config
> > file for the defaults, otherwise the environment variable if set. But since
> > the way I had implemented processing the arguments early relied on not well
> > guaranteed mechanisms I resorted to avoid that approach.
> > 
> > This was the alternative I came up with.
> > 
> > The environment variable stuff is going away so this should be much simpler
> > now, however the question of argument processing early still remains.
> > 
> > > >  
> > > > -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > > > +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > > >  		switch (c) {
> > > > +		case 'c':
> > > > +			reset_defaults_and_cli(&dft, &cli);
> > > 
> > > This will trash the existing CLI inputs that have already been
> > > parsed. 
> > 
> > Exactly.
> > 
> > > All this code here should do is set up the config file
> > > to be read.
> > 
> > Consider the case of /etc/mkfs.d/default existing and it having
> > 
> > [metadata]
> > crc = 1
> > [naming]
> > ftype=1
> > 
> > And suppose the user passed -c foo which had:
> > 
> > [metadata]
> > crc = 0
> > 
> > Without the reset we'd make /etc/mkfs.d/default the system wide defaults onto
> > which -c parameters overlay on top of. I figured that was not desirable.
> 
> It's not desirable. If the user specified a config file, then
> /etc/mkfs.d/default *is never read*. The user config file is used
> instead. If the user supplied file does not exist (even after
> searching) then we fail, not use some other set of defaults.

I didn't overlay and will ensure we don't as I change this to read the
command line args first prior to reading any file. The reset should not
be needed once that is done then.

> > > Again, what happens if the user specifies multiple config files?
> > 
> > The reset strategy lets the user set -c as many times as they wish and also
> > starts from a clean base always.
> 
> multiple "-c" options is a user error and should error out.

Great.

> > > The second config file will trash everything from the first, as well
> > > as any other options between them.
> > 
> > Yes! By design as we couldn't easily parse arguments early first and then
> > choose just one strategy.
> >
> > I liked the simplicity that this brings.
> > 
> > Would you really want multiple -c options to work as overlays, one on top of
> > the other?
> 
> I don't want multiple -c option to be supported at all. We source
> defaults from a single file only - either the user specified file,
> or the default if it exists. Not both, not multiple user specified
> files. One file.

OK!

> > > I think that we need to pull the "-c <file>" option from the command
> > > line and process all the defaults /before/ we enter the main command
> > > line processing loop.
> > 
> > That is what I had done originally but ran into that snag of processing
> > arguments twice and the undocumented functionality I found that worked.
> 
> Multiple pass option parsing is documented as supported and has been
> for a long, long time. And to make that clear, we even have a
> wrapper in xfsprogs for it:
> 
> $ git grep platform_getoptreset
> db/command.c:   platform_getoptreset();
> include/darwin.h:static __inline__ void platform_getoptreset(void)
> include/freebsd.h:static __inline__ void platform_getoptreset(void)
> include/gnukfreebsd.h:static __inline__ void platform_getoptreset(void)
> include/linux.h:static __inline__ void platform_getoptreset(void)

That sets optind = 0 but indeed despite my concerns if its used widely
for years, let's go with it.

> libxcmd/command.c:      platform_getoptreset();
> 
> And we use it in libxcmd so that each function in xfs_io, xfs_db,
> xfs_quota, etc can run their own sub-command getopt calls correctly.
> It was in the initial commits for xfs_db back in 2001, so we've been
> using multiple pass CLI option parsing in xfsprogs since 2001....

Alright!

> > > IOWs:
> > > 	config_file = getenv("MKFS_XFS_CONFIG");
> > > 	if (config_file)
> > > 		dft.source = _("Environment Variable");
> > > 
> > > 	/*
> > > 	 * Pull config line options from command line
> > > 	 */
> > > 	while ((c = getopt(argc, argv, "c:")) != EOF) {
> > > 		switch (c) {
> > > 		case 'c':
> > > 			/* XXX: fail if already seen a CLI option */
> > 
> > BTW isn't my enum here sufficient to check for this easily in a clean
> > short and easy way?
> 
> A local boolean variable is all that is ncessary for this....

OK!

> [....]
> 
> > > 	memcpy(/* sb_feats */);
> > > 	memcpy(/* fsxattr */);
> > > 	optind = 1;
> > 
> > My getopt(3) *does* not make mention of any of this. This man page however does:
> 
> It's been there as long as I can remember. 2nd paragraph of the
> description:
> 
> $man 3 getopt
> [...]
> DESCRIPTION
> [...]
>        The variable optind is the index of the next element to be
>        processed in argv.  The system initializes this value to  1.
>        The  caller  can reset it to 1 to restart scanning of the
>        same argv, or when scanning a new argument vector.
> 
> 
> > optind = 1;
> > 
> > However perhaps a big fat NOTE is worthy?
> 
> Why? It's documented, well known behaviour....

Alright.

> > > [...]
> > > 
> > > > +const char *default_type_str(enum default_params_type type)
> > > > +{
> > > > +	switch (type) {
> > > > +	case DEFAULTS_BUILTIN:
> > > > +		return _("package built-in definitions");
> > > > +	case DEFAULTS_CONFIG:
> > > > +		return _("default configuration system file");
> > > > +	case DEFAULTS_ENVIRONMENT_CONFIG:
> > > > +		return _("custom configuration file set by environment");
> > > > +	case DEFAULTS_TYPE_CONFIG:
> > > > +		return _("custom configuration type file");
> > > > +	}
> > > > +	return _("Unkown\n");
> > > > +}
> > > 
> > > This function can go away.
> > 
> > Well depends, if we keep the enum the no ?
> 
> That enum needs to die. It's unnecssary abstraction.

Alrighty.

> > > > +	if (strlen(line) < 2)
> > > > +		return PARSE_INVALID;
> > > 
> > > So empty lines return PARSE_INVALID?
> > 
> > Yes, but that's not fatal, we just discard it.
> 
> That's "ignore", like comments, not "invalid".

Sure that makes sense.

> > 
> > > > +
> > > > +	if (line[0] == '#')
> > > > +		return PARSE_COMMENT;
> > > 
> > > What about lines starting with whitespace? e.g. " # comment"
> > 
> > parse_line_section() below would not return 1 and its ignored
> > in the end as its also not a proper tag/value pair and the
> > section is not set.
> 
> So it silently ignores a valid sections/{tag/value pair} if there's
> trailing stuff on the line? Shouldn't that throw an error?

Will fix.

> > > > +{
> > > > +	int ret;
> > > > +	FILE *fp;
> > > > +	struct stat sp;
> > > > +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> > > > +					    sizeof(struct config_subopts) - 1;
> > > > +	char *p;
> > > > +	char line[80], tag[80];
> > > > +	bool value;
> > > > +	enum parse_line_type parse_type;
> > > > +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> > > > +
> > > > +	fp = fopen(dft->config_file, "r");
> > > > +	if (!fp) {
> > > > +		if (dft->type != DEFAULTS_BUILTIN) {
> > > > +			fprintf(stderr, _("could not open config file: %s\n"),
> > > > +				dft->config_file);
> > > > +			ret = -ENOENT;
> > > > +		} else
> > > > +			ret = 0;
> > > > +		return ret;
> > > > +	}
> > > 
> > > This should be split out into a separate function that takes the
> > > config file and tries to open it. If it's a relative path that was
> > > supplied, then this function can also try all the configured search
> > > paths for config files.
> > 
> > Eric asked to support 3 cases:
> > 
> >  a) ./ -- current working directory
> >  b) /  -- full path
> >  c) no prefix - we use the sysconfigdir/mkfs.d/
> 
> That's pretty much what I just suggested. :)

Good, so only look at the current working directory *iff* "./" is
passed, right?

> > > > +		return ret;
> > > > +
> > > > +	while (!feof(fp)) {
> > > > +		p = fgets(line, sizeof(line), fp);
> > > 
> > > What if the line is longer than 80 characters? e.g. a long comment
> > 
> > If its a comment or spaces with a comment, we still only care for
> > the first 80 characters, no?
> 
> What does fgets() do with the remaining characters on the line? They
> are still parked in the input stream, so won't the next fgets() call
> read them? And then we parse those bytes as if they are a new config
> line?
> 
> IOWs, shouldn't we be using a line-based input function here? Say,
> getline(3)?

I suspect you are right, will look into it, and use it unless I find
something better.

> > > > +		if (!p)
> > > > +			continue;
> > > > +
> > > > +		parse_type = parse_get_line_type(line, tag, &value);
> > > > +
> > > > +		switch (parse_type) {
> > > > +		case PARSE_COMMENT:
> > > > +		case PARSE_INVALID:
> > > > +		case PARSE_EOF:
> > > > +			break;
> > > > +		case PARSE_SECTION:
> > > > +			section = get_config_section(tag);
> > > > +			if (!section) {
> > > > +				fprintf(stderr, _("Invalid section: %s\n"),
> > > > +						tag);
> > > > +				return -EINVAL;
> > > > +			}
> > > > +			break;
> > > > +		case PARSE_TAG_VALUE:
> > > > +			/* Trims white spaces */
> > > > +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> > > > +			ret = parse_config_subopts(section, value, line, dft);
> > > 
> > > We've already got the tag and value as discrete variables - why put
> > > them back into an ascii string to pass to another function for it to
> > > split them back into discrete variables?
> > 
> > The comment above it explains, "Trims white spaces"
> 
> Why do we need to do that? Doesn't the token parser trim away excess
> whitespace around each token it returns?

No, I don't think it did, but will check again.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 21, 2018, 5:05 p.m. UTC | #16

On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
> On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
> > On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> > > On Thu, May 17, 2018 at 10:24:13PM -0500, Eric Sandeen wrote:
> > > > On 5/17/18 2:27 PM, Luis R. Rodriguez wrote:
> > > > > You may want to stick to specific set of configuration options when
> > > > > creating filesystems with mkfs.xfs -- sometimes due to pure technical
> > > > > reasons, but some other times to ensure systems remain compatible as
> > > > > new features are introduced with older kernels, or if you always want
> > > > > to take advantage of some new feature which would otherwise typically
> > > > > be disruptive.
> > > > > 
> > > > > This adds support for parsing a configuration file to override defaults
> > > > > parameters to be used for mkfs.xfs.
> > > > > 
> > > > > We define an XFS configuration directory,/etc/mkfs.xfs.d/  and allow for
> > > > > different configuration files, if none is specified we look for the
> > > > > default configuration file, /etc/mkfs.xfs.d/default. You can override
> > > > > with -c.  For instance, if you specify:
> > > > > 
> > > > > 	mkfs.xfs -c experimental -f /dev/loop0
> > > > > 
> > > > > The file /etc/mkfs.xfs.d/experimental will be used as your configuration
> > > > > file. If you really need to override the full path of the configuration
> > > > > file you may use the MKFS_XFS_CONFIG environment variable.
> > > > 
> > > > I'm swamped under a deadline at work this week so just commenting at a
> > > > very high level for now, but I'm curious; why use an env var vs
> > > > providing a full path for -c ?  env vars always strike me as magic unexpected
> > > > behaviors.
> > > > 
> > > > # mkfs.xfs -c /my/fancy/path/to/config
> > > > 
> > > > seems much clearer than
> > > > 
> > > > # export MKFS_XFS_CONFIG=/my/fancy/path/to/
> > > > # mkfs.xfs -c config
> > > > 
> > > > i.e. if a full path is specified use it, else use the config directory.
> > > > 
> > > > Thoughts?
> > > 
> > > In this case your choices are:
> > > MKFS_XFS_CONFIG=/my/fancy/path/to/config mkfs.xfs
> > > 
> > > or:
> > > cp /my/fancy/path/to/config /etc/mkfs.xfs.d/hoogah
> > > mkfs.xfs -c hooga
> > 
> > Sorry, why do we need to copy it to /etc/mkfs.xfs.d/?
> 
> No particular reason, just showing off relative pathname interpretation.
> That could have been
> 
> mkfs.xfs -c ../../my/fancy/path/to/config
> 
> But that's kinda ugly.  $sysconfdir seems to get set to /usr on my
> local builds, though I think dpkg overrides that back to /... :)

You will need to set:

--sysconfdir=/etc

And the spec file / debian/control file will need to be update to use
this on the configure line.

> > > <shrug> Bikeshedding more, what if either option accepted either an
> > > absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ? 
> > 
> > I kinda assumed that config files could be located anywhere, but we
> > only searched the sysconfig path if it didn't point at a local
> > file...
> 
> <shrug> openat() semantics are fine enough with me, I think.

Well this is a big difference, and I think being clear on this would
be good. If the user specified:

	-c foo
	
and the file 'foo' is present but also exists on
$sysconfdir/etc/mkfs.xfs.d/foo do we use the local file if the user
did not pass ./foo ?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 21, 2018, 6:32 p.m. UTC | #17

On Thu, May 17, 2018 at 02:31:21PM -0700, Darrick J. Wong wrote:
> On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > If you used the
> > +.B -c
> > +parameter or if you set the
> > +.I MKFS_XFS_CONFIG
> > +environment variable the configuration file must be present and should parse
> 
> "...must parse..."

Fixed and dropped the MKFS_XFS_CONFIG environment variable reference.

> > +correctly.
> > +.PP
> > +Parameters passed to to the
> 
> "...passed to the..."

Fixed.

> > diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> > index 4b8c78c37806..fadc4c589a8c 100644
> > --- a/man/man8/mkfs.xfs.8
> > +++ b/man/man8/mkfs.xfs.8
> > @@ -83,6 +83,26 @@ and
> >  .B \-l internal \-l size=10m
> >  are equivalent.
> >  .PP
> > +An optional XFS configuration file directory
> > +.B mkfs.xfs.d (5)
> 
> %sysconfigdir%/mkfs.xfs.d ?

I rather refer to the man page using the short name. When referring to
the full path though I will add the %sysconfigdir%/mkfs.xfs.d and have
this parse.

> > @@ -3077,11 +3078,13 @@ print_mkfs_cfg(
> >  	struct mkfs_params	*cfg,
> >  	char			*dfile,
> >  	char			*logfile,
> > -	char			*rtfile)
> > +	char			*rtfile,
> > +	struct mkfs_default_params *dft)
> >  {
> >  	struct sb_feat_args	*fp = &cfg->sb_feat;
> >  
> >  	printf(_(
> > +"config-file=%-22s\n"
> >  "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
> >  "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
> >  "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
> > @@ -3091,6 +3094,7 @@ print_mkfs_cfg(
> >  "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
> >  "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
> >  "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
> > +		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
> >  		dfile, cfg->inodesize, (long long)cfg->agcount,
> >  			(long long)cfg->agsize,
> >  		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
> 
> FWIW all this geometry printing was refactored into libfrog.
> 
> Though, you already print where we got the configuration data, so just
> print dft->config_file there.

True, will do.

> > -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> > +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> >  		switch (c) {
> > +		case 'c':
> > +			reset_defaults_and_cli(&dft, &cli);
> > +
> > +			memset(custom_config, 0, sizeof(custom_config));
> > +			snprintf(custom_config, sizeof(custom_config), "%s%s",
> > +				 MKFS_XFS_CONF_DIR, optarg);
> > +			dft.config_file = custom_config;
> > +			dft.type = DEFAULTS_TYPE_CONFIG;
> 
> Ok, so we parse $MKFS_XFS_CONFIG but then if the user specifies -c we
> reset all that and parse that config file.

Right.

> Just for fun I decided to run:
> 
> $ cat > /tmp/butts.lol
> [gugugugug]
> bye = 1
> [metadata]
> hork = 1
> $ ./build-x64/mkfs/mkfs.xfs -c ../../../../../../../tmp/butts.lol -N /dev/sda
> Error parsine line: bye=1
> 
> I think we need to error out on unrecognized section names:
> 
> "Unrecognized section name 'gugugugug'."

Will fix.

> And probably report the section name for unrecognized keys:
> 
> "Unrecognized name 'metadata.hork"."

Alright.

> > +#define CONFIG_MAX_KEY		1024
> > +#define CONFIG_MAX_VALUE	PATH_MAX
> 
> That's a pretty big size considering we only allow 0 and 1...

Heh true, as per Chinner will just check for a single max file size for
about 1MiB or so, so we won't need these as it was just used to compute
a max theoretical file size. By using a simple reasonable file size we
remove the need for the above.

> > +#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
> > +
> > +/*
> > + * Enums for each configuration option. All these currently match the CLI
> > + * parameters for now but this may change later, so we keep all this code
> > + * and definitions separate. The rules for configuration parameters may also
> > + * differ.
> > + */
> 
> So... I think these enums should be shared between cli and config file
> processing, 

As per Chinner's request we'd duplicate for now as we may want to
support alternative config options which would differ from the CLI
options. This is why the comment above it notes this.

> or if the config file parser retains its own private
> definitions then it should only have the options that we support in the
> config file.  It's weird how things like data.sunit are specified here
> but the config file doesn't actually do anything with it?

The enum is defined for it, the parser however lacks a switch entry for
it. I'll just trim the enums to what we currently parse only then.

> $ cat > /tmp/butts.lol
> [data]
> sunit = 5
> $ ./build-x64/mkfs/mkfs.xfs -c ../../../tmp/butts.lol -N /dev/sda | grep sunit
>          =                       sunit=0      swidth=0 blks
> 
> That really ought to error out, since we don't support setting sunit?

Will verify and fix.

> > +const char *default_type_str(enum default_params_type type)
> 
> const char *
> default_type_str(
> 	enum default_params_type	type)
> {
> 	...
> }
> 
> Please follow regular xfs formatting conventions.

Will do.

> > +{
> > +	switch (type) {
> > +	case DEFAULTS_BUILTIN:
> > +		return _("package built-in definitions");
> > +	case DEFAULTS_CONFIG:
> > +		return _("default configuration system file");
> > +	case DEFAULTS_ENVIRONMENT_CONFIG:
> > +		return _("custom configuration file set by environment");
> > +	case DEFAULTS_TYPE_CONFIG:
> > +		return _("custom configuration type file");
> > +	}
> > +	return _("Unkown\n");
> 
> "Unknown"

This call is being removed as per Chinner's request.

> > +static int mkfs_check_config_file_limits(const char *filename,
> > +					 const struct stat *sb,
> > +					 unsigned int max_entries)
> > +{
> > +	unsigned long long size_limit;
> > +
> > +	size_limit = CONFIG_MAX_BUFFER * max_entries;
> > +
> > +	/*
> > +	 * Detect wrap around -- if max_entries * size_limit goves over
> > +	 * unsigned long long we detect that there. Note some libraries,
> > +	 * like libiniconfig, only groks max INT_MAX entries anyway, so
> > +	 * if we ever use a library for parsing we'd be restricted to that
> > +	 * limit.
> > +	 */
> > +	if (size_limit < max_entries || max_entries > INT_MAX)
> > +		return -E2BIG;
> > +
> > +	if (sb->st_size > size_limit) {
> > +		fprintf(stderr,
> > +			"File %s is too big! File size limit: %llu\n",
> > +			filename, size_limit);
> > +		return -E2BIG;
> > +	}
> 
> Not sure why we care about the file size?  If the config file
> respecifies options a trillion times then let it.

I'll simplify the check to 1MiB file size alone.

Chinner requested we actually don't allow respecification on the
configuration file. Let me know what you two decide.

> > +
> > +	return 0;
> > +}
> > +
> > +enum parse_line_type {
> > +	PARSE_COMMENT = 0,
> > +	PARSE_SECTION,
> > +	PARSE_TAG_VALUE,
> > +	PARSE_INVALID,
> > +	PARSE_EOF,
> > +};
> > +
> > +static int parse_line_section(const char *line, char *tag)
> > +{
> > +	return sscanf(line, " [%[^]]s]", tag);
> > +}
> > +
> > +static int parse_line_tag_value(const char *line, char *tag,
> > +				unsigned int *value)
> > +{
> > +	return sscanf(line, " %[^ \t=]"
> > +		      " = "
> > +		      "%u ",
> > +		      tag, value);
> > +}
> > +
> > +static enum parse_line_type parse_get_line_type(const char *line, char *tag,
> > +						bool *value)
> > +{
> > +	int ret;
> > +	unsigned int uint_value;
> > +
> > +	memset(tag, 0, 80);
> > +
> > +	if (strlen(line) < 2)
> > +		return PARSE_INVALID;
> > +
> > +	if (line[0] == '#')
> > +		return PARSE_COMMENT;
> > +
> > +	ret = parse_line_section(line, tag);
> > +	if (ret == 1)
> > +		return  PARSE_SECTION;
> > +
> > +	if (ret == EOF)
> > +		return PARSE_EOF;
> > +
> > +	ret = parse_line_tag_value(line, tag, &uint_value);
> > +	if (ret == 2) {
> > +		if (uint_value != 1 && uint_value != 0)
> > +			return -EINVAL;
> 
> If I set data.sunit = 5 this will return -EINVAL to the switch in
> parse_config_init.  However, the switch lacks a default clause so it
> silently drops invalid values and proceeds with the format.

Odd, in this case sunit is a valid tag, so it shoudl have errored out,
but will re-test and also ensure invalid tags fail accordingly.

> > +
> > +		*value = !!uint_value;
> > +
> > +		return PARSE_TAG_VALUE;
> > +	}
> > +
> > +	if (ret == EOF)
> > +		return PARSE_EOF;
> > +
> > +	return PARSE_INVALID;
> > +}
> > +
> > +int parse_config_init(struct mkfs_default_params *dft)
> > +{
> > +	int ret;
> > +	FILE *fp;
> > +	struct stat sp;
> > +	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
> > +					    sizeof(struct config_subopts) - 1;
> > +	char *p;
> > +	char line[80], tag[80];
> 
> Waitaminute, didn't we set CONFIG_MAX_KEY = 1024 and CONFIG_MAX_VALUE
> to PATH_MAX?

Heh yeah, I'll just drop those and keep 80.

> > +	bool value;
> > +	enum parse_line_type parse_type;
> > +	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
> > +
> > +	fp = fopen(dft->config_file, "r");
> > +	if (!fp) {
> > +		if (dft->type != DEFAULTS_BUILTIN) {
> 
> Why would we be parsing a config file if dft->type == DEFAULTS_BUILTIN?

If the user specified an environment variable thent he error should be
fatal, otherwise, if the type is built-in then the default is being
picked up, and in such case an error on the file not existing is
non-fatal.

But all this will be simplified now that we'll just deal with *one*
choice of parsing early.

> > +			fprintf(stderr, _("could not open config file: %s\n"),
> > +				dft->config_file);
> > +			ret = -ENOENT;
> > +		} else
> > +			ret = 0;
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * If we found a file without it being specified on the command line,
> > +	 * it would be the default configuration file.
> > +	 */
> > +	if (dft->type == DEFAULTS_BUILTIN)
> > +		dft->type = DEFAULTS_CONFIG;
> 
> Oh, I see, type == _BUILTIN really means "we picked up
> /etc/mkfs.xfs.d/defaults"... shouldn't the caller set this?

No, it means the built-in defaults were used, but since the config file
exists, it means either the deault config file was found or the user
specified one.

But again, this will all be simplified further.

> > +
> > +	ret = stat(dft->config_file, &sp);
> 
> fstat(fileno(fp), &sp); ?

Sure!

> > +	if (ret) {
> > +		if (dft->type != DEFAULTS_BUILTIN)
> 
> Didn't we just set this to _CONFIG?

Only if it was not built-in, ie, if the environment variable was used
nope.

> > +			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
> > +				dft->config_file, strerror(ret));
> > +		return ret;
> 
> We just leaked fp...

I've sprinkled goto outs, thanks.

> > +	}
> > +
> > +	if (!sp.st_size)
> > +		return 0;
> 
> Can't we just keep going?  If the file size is zero we never execute the
> loop.

Either way is fine, less code is better, so will remove this.

> > +	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
> > +					    num_subopts_sections);
> > +	if (ret)
> > +		return ret;
> > +
> > +	while (!feof(fp)) {
> > +		p = fgets(line, sizeof(line), fp);
> > +		if (!p)
> > +			continue;
> > +
> > +		parse_type = parse_get_line_type(line, tag, &value);
> > +
> > +		switch (parse_type) {
> > +		case PARSE_COMMENT:
> > +		case PARSE_INVALID:
> > +		case PARSE_EOF:
> > +			break;
> > +		case PARSE_SECTION:
> > +			section = get_config_section(tag);
> > +			if (!section) {
> > +				fprintf(stderr, _("Invalid section: %s\n"),
> > +						tag);
> > +				return -EINVAL;
> > +			}
> > +			break;
> > +		case PARSE_TAG_VALUE:
> > +			/* Trims white spaces */
> > +			snprintf(line, sizeof(line), "%s=%u", tag, value);
> 
> So we reconstruct the line (without the whitespaces) so that we can use
> getsubopt to map the tag to an integer value (e.g. D_AGCOUNT)... 

Right.

> we already isolated *tag, so can't we map it directly?

What do you mean by map it directly?

> > +			ret = parse_config_subopts(section, value, line, dft);
> > +			if (ret) {
> > +				fprintf(stderr, _("Error parsine line: %s\n"),
> > +						line);
> > +				return ret;
> > +			}
> > +			break;
> 
> case -EINVAL?

I take it you meant case default: return -EINVAL? Since I use enums on
the return value from parse_get_line_type() and on the switch statement,
the compiler checks for any known enums which were not on the switch and
complains if one is found.

So long as parse_get_line_type() returns an PARSE_INVALID or
PARSE_IGNORE (which I'll have to add) then we're OK without it, and
are better off without a default case to ensure we cover all parse
type conditions we add.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner May 21, 2018, 10:10 p.m. UTC | #18

On Mon, May 21, 2018 at 10:05:30AM -0700, Luis R. Rodriguez wrote:
> On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
> > On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
> > > On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> > > > <shrug> Bikeshedding more, what if either option accepted either an
> > > > absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ? 
> > > 
> > > I kinda assumed that config files could be located anywhere, but we
> > > only searched the sysconfig path if it didn't point at a local
> > > file...
> > 
> > <shrug> openat() semantics are fine enough with me, I think.
> 
> Well this is a big difference, and I think being clear on this would
> be good. If the user specified:
> 
> 	-c foo
> 	
> and the file 'foo' is present but also exists on
> $sysconfdir/etc/mkfs.xfs.d/foo do we use the local file if the user
> did not pass ./foo ?

I would have expected "foo" to be considered the same as "./foo".
It's a relative path.

Cheers,

Dave.

Eric Sandeen May 21, 2018, 10:24 p.m. UTC | #19

On 5/21/18 5:10 PM, Dave Chinner wrote:
> On Mon, May 21, 2018 at 10:05:30AM -0700, Luis R. Rodriguez wrote:
>> On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
>>> On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
>>>> On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
>>>>> <shrug> Bikeshedding more, what if either option accepted either an
>>>>> absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ?
>>>>
>>>> I kinda assumed that config files could be located anywhere, but we
>>>> only searched the sysconfig path if it didn't point at a local
>>>> file...
>>>
>>> <shrug> openat() semantics are fine enough with me, I think.
>>
>> Well this is a big difference, and I think being clear on this would
>> be good. If the user specified:
>>
>> 	-c foo
>> 	
>> and the file 'foo' is present but also exists on
>> $sysconfdir/etc/mkfs.xfs.d/foo do we use the local file if the user
>> did not pass ./foo ?
> 
> I would have expected "foo" to be considered the same as "./foo".
> It's a relative path.

Urgh, so now if foo exists in $PWD /and/ in $sysconfdir/etc/mkfs.xfs.d/foo
we have to have a hierarchy between the two?  :/

Ok. back up.  Honestly I think the only good reason to have config files
outside of $sysconfdir/etc/mkfs.xfs.d/ is to facilitate testing without
perturbing the system's files in order to do so.

In that case maybe we should distill it down to:

1) -c foo looks in $sysconfdir/etc/mkfs.xfs.d
2) -c /absolute/path/to/foo looks there, obviously

and drop relative paths.  Absolute paths are more cumbersome but testing
scripts won't care about that.  mkfs.xfs -c `pwd`/foo isn't that hard either.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner May 22, 2018, 12:38 a.m. UTC | #20

On Mon, May 21, 2018 at 05:24:34PM -0500, Eric Sandeen wrote:
> On 5/21/18 5:10 PM, Dave Chinner wrote:
> >On Mon, May 21, 2018 at 10:05:30AM -0700, Luis R. Rodriguez wrote:
> >>On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
> >>>On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
> >>>>On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> >>>>><shrug> Bikeshedding more, what if either option accepted either an
> >>>>>absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ?
> >>>>
> >>>>I kinda assumed that config files could be located anywhere, but we
> >>>>only searched the sysconfig path if it didn't point at a local
> >>>>file...
> >>>
> >>><shrug> openat() semantics are fine enough with me, I think.
> >>
> >>Well this is a big difference, and I think being clear on this would
> >>be good. If the user specified:
> >>
> >>	-c foo
> >>	
> >>and the file 'foo' is present but also exists on
> >>$sysconfdir/etc/mkfs.xfs.d/foo do we use the local file if the user
> >>did not pass ./foo ?
> >
> >I would have expected "foo" to be considered the same as "./foo".
> >It's a relative path.
> 
> Urgh, so now if foo exists in $PWD /and/ in $sysconfdir/etc/mkfs.xfs.d/foo
> we have to have a hierarchy between the two?  :/

Of course there is. The user may not know anything about admin
configured defaults, and they are well within their rights to have
their own local files that have names that match global admin
default files.

Just about every major app with config files searches relative path
first, then $USER/.<app>/, then $SYS/<app>/. It's a well known,
obvious search path (i.e. local->user default->global default) and
we have no valid excuse for making up our own special snowflake that
is completely different. We don't need the $USER directory (not
every app needs or uses it), but otherwise we should follow
convention....

IMO, making a distinction in behaviour between "-c foo" and "-c
./foo" such that they implicitly direct the app between "local" and
"global" config file search paths is really bad UI design. It's
surprising, and will lead to people complaining about how they
needed to strace mkfs.xfs to work out it isn't actually reading
their local config file.

Apps doing unnecessarily special UI crap is what makes people like
me really, really, really hate software developers.  We should not
be making up our own special snowflake behaviour just because we
think we're special enough we're allowed to do special things that
users don't expect.

> Ok. back up.  Honestly I think the only good reason to have config files
> outside of $sysconfdir/etc/mkfs.xfs.d/ is to facilitate testing without
> perturbing the system's files in order to do so.
>
> In that case maybe we should distill it down to:
> 
> 1) -c foo looks in $sysconfdir/etc/mkfs.xfs.d
> 2) -c /absolute/path/to/foo looks there, obviously
> 
> and drop relative paths.  Absolute paths are more cumbersome but testing
> scripts won't care about that.  mkfs.xfs -c `pwd`/foo isn't that hard either.

IMO, that's just another special snowflake....

Cheers,

Dave.

Luis Chamberlain May 22, 2018, 7:37 p.m. UTC | #21

On Fri, May 18, 2018 at 10:44:04AM +1000, Dave Chinner wrote:
> On Thu, May 17, 2018 at 12:27:00PM -0700, Luis R. Rodriguez wrote:
> > +}
> > +
> > +static enum parse_line_type parse_get_line_type(const char *line, char *tag,
> > +						bool *value)
> > +{
> > +	int ret;
> > +	unsigned int uint_value;
> > +
> > +	memset(tag, 0, 80);
> 
> Why? This should be done in the caller, and if there's a fixed
> buffer size, then please use a #define.

I've modified to use 'm' specifier on scanf().

Using 'm' tells scanf() to allocate a buffer for us, this is easier as
we then enable the parser to do as it sees fit so long as the patterns
match. We just have to later free it after use.

That removes the need to memset() and stick to a predefined buffer sizes
for the tags (section or subopt name).

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 25, 2018, 12:50 a.m. UTC | #22

On Mon, May 21, 2018 at 11:40:30AM +0200, Jan Tulak wrote:
> On Sat, May 19, 2018 at 1:56 AM, Luis R. Rodriguez <[e-mail]> wrote:
> > On Fri, May 18, 2018 at 12:09:23PM -0500, Eric Sandeen wrote:
> ...
> >>
> >> mkfs.xfs -c hoogah            searches /@sysconfigdir@/mkfs.xfs.d/
> >> mkfs.xfs -c ./hoogah          or
> >> mkfs.xfs -c /path/to/hoogah   do exactly what you'd expect.
> >>
> ...
> >
> > Sounds good. I'll drop the environment variable junk and only support
> > the above 3 cases.
> >
> 
> mkfs.xfs -c ../hoogah too, please. :-)

Done.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 25, 2018, 12:51 a.m. UTC | #23

On Tue, May 22, 2018 at 10:38:57AM +1000, Dave Chinner wrote:
> On Mon, May 21, 2018 at 05:24:34PM -0500, Eric Sandeen wrote:
> > On 5/21/18 5:10 PM, Dave Chinner wrote:
> > >On Mon, May 21, 2018 at 10:05:30AM -0700, Luis R. Rodriguez wrote:
> > >>On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
> > >>>On Sun, May 20, 2018 at 10:16:48AM +1000, Dave Chinner wrote:
> > >>>>On Thu, May 17, 2018 at 08:46:00PM -0700, Darrick J. Wong wrote:
> > >>>>><shrug> Bikeshedding more, what if either option accepted either an
> > >>>>>absolute path, or a file in $sysconfdir/etc/mkfs.xfs.d/ ?
> > >>>>
> > >>>>I kinda assumed that config files could be located anywhere, but we
> > >>>>only searched the sysconfig path if it didn't point at a local
> > >>>>file...
> > >>>
> > >>><shrug> openat() semantics are fine enough with me, I think.
> > >>
> > >>Well this is a big difference, and I think being clear on this would
> > >>be good. If the user specified:
> > >>
> > >>	-c foo
> > >>	
> > >>and the file 'foo' is present but also exists on
> > >>$sysconfdir/etc/mkfs.xfs.d/foo do we use the local file if the user
> > >>did not pass ./foo ?
> > >
> > >I would have expected "foo" to be considered the same as "./foo".
> > >It's a relative path.
> > 
> > Urgh, so now if foo exists in $PWD /and/ in $sysconfdir/etc/mkfs.xfs.d/foo
> > we have to have a hierarchy between the two?  :/
> 
> Of course there is. The user may not know anything about admin
> configured defaults, and they are well within their rights to have
> their own local files that have names that match global admin
> default files.

Chinner's got a point here. If felt silly to not support this so I just
added support for it.

Will spin out a new set as I think I've taken care of all comments now.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Luis Chamberlain May 25, 2018, 12:54 a.m. UTC | #24

On Mon, May 21, 2018 at 08:33:54AM -0700, Darrick J. Wong wrote:
> <shrug> openat() semantics are fine enough with me, I think.

I tried.

With openat() we'd have to go through a bit of hoops to figure out which
actual file we ended up using and it was not pretty.  And given the
other snowflake considerations folks wanted to support as that *is* what
typically users support, it turned out much easier to just use good 'ol
open().

New patch set coming.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/builddefs.in b/include/builddefs.in
index 8aac06cf90dc..e1ee9f7ba086 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -62,6 +62,7 @@  PKG_LIB_DIR	= @libdir@@libdirsuffix@
 PKG_INC_DIR	= @includedir@/xfs
 DK_INC_DIR	= @includedir@/disk
 PKG_MAN_DIR	= @mandir@
+PKG_ETC_DIR	= @sysconfdir@
 PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
 PKG_LOCALE_DIR	= @datadir@/locale
 
@@ -196,6 +197,7 @@  endif
 
 GCFLAGS = $(DEBUG) \
 	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
+	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
 	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
 
 ifeq ($(ENABLE_GETTEXT),yes)
diff --git a/man/man5/mkfs.xfs.d.5 b/man/man5/mkfs.xfs.d.5
new file mode 100644
index 000000000000..5424f730a2e5
--- /dev/null
+++ b/man/man5/mkfs.xfs.d.5
@@ -0,0 +1,121 @@ 
+.TH mkfs.xfs.d 5
+.SH NAME
+mkfs.xfs.d \- mkfs.xfs configuration directory
+.SH DESCRIPTION
+.B mkfs.xfs (8)
+uses a set of initial default parameters for configuration. These defaults
+are conservatively decided by the community as xfsprogs and features for XFS
+in the kernel advance. One can override these defaults on the
+.B mkfs.xfs (8)
+command line, but there are cases where it is desirable for sensible defaults
+to always be specified by the system where
+.B mkfs.xfs (8)
+runs on. This may desirable for example on systems with old kernels where the
+built-in default parameters on
+.B mkfs.xfs (8)
+may not be able to create a filesystem which the old kernel supports and it
+would be unclear what parameters are needed to produce a compatible filesystem.
+Overriding
+.B mkfs.xfs (8)
+built-in defaults may also be desirable if you have a series of systems with
+different kernels and want to be able to create filesystems which all systems
+are able to support properly.
+.PP
+The XFS configuration directory
+.B mkfs.xfs.d (5)
+can be used to define different configuration files which can be used to
+override the built-in default parameters by
+.B mkfs.xfs (8).
+Different configuration files are supported, the default
+configuration file,
+.I /etc/mkfs.xfs.d/default
+, will be looked for first and if present will be used to override
+.B mkfs.xfs (8)
+built-in default parameters. You can override the configuration file by
+specifying its name when using
+.B mkfs.xfs (8)
+by using the
+.B -c
+parameter. For example:
+.I mkfs.xfs -c experimental -f /dev/sda1
+will make
+.B mkfs.xfs (8)
+look for and use the configuration file
+.I /etc/mkfs.xfs.d/experimental
+to override
+.B mkfs.xfs (8)
+default parameters. If you need to override the full path for a configuration
+file you can use the
+.I MKFS_XFS_CONFIG
+environment variable prior to calling
+.B mkfs.xfs (8)
+to define the
+full path to the configuration file to be used. If you used the
+.B -c
+parameter or if you set the
+.I MKFS_XFS_CONFIG
+environment variable the configuration file must be present and should parse
+correctly.
+.PP
+Parameters passed to to the
+.B mkfs.xfs (8)
+command line always override any defaults set on the configuration file used.
+.PP
+.B mkfs.xfs (8)
+will always describe what configuration file was used, if any
+was used at all. To verify which configuration file would be used prior to
+execution of
+.B mkfs.xfs (8)
+you can use
+.I mkfs.xfs -N.
+.PP
+.SH DEFAULT PARAMETERS
+Default parameters for
+.B mkfs.xfs (8)
+consists of a small subset of the parameters one can set with on the command
+line. Currently all default parameters can only be either enabled or disabled,
+you can set their value to 1 to enable or 0 to disable. Below we list the
+different supported default parameters which can be defined on configuration
+files, along with the current built-in setting.
+.PP
+.BI [data]
+.br
+.BI noalign=0
+.PP
+.BI [inode]
+.br
+.BI align=1
+.br
+.BI projid32bit=1
+.br
+.BI sparse=0
+.PP
+.BI [log]
+.br
+.BI lazy-count=1
+.PP
+.BI [metadata]
+.br
+.BI crc=1
+.br
+.BI finobt=1
+.br
+.BI rmapbt=0
+.br
+.BI reflink=0
+.PP
+.BI [naming]
+.br
+.BI ftype=1
+.PP
+.BI [rtdev]
+.br
+.BI noalign=0
+.PP
+.SH SEE ALSO
+.BR mkfs.xfs (8),
+.BR xfsctl (3),
+.BR xfs_info (8),
+.BR xfs_admin (8),
+.BR xfsdump (8),
+.BR xfsrestore (8).
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 4b8c78c37806..fadc4c589a8c 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -83,6 +83,26 @@  and
 .B \-l internal \-l size=10m
 are equivalent.
 .PP
+An optional XFS configuration file directory
+.B mkfs.xfs.d (5)
+exists to help fine tune default parameters which can be used when calling
+.B mkfs.xfs (8).
+If present, the default file /etc/mkfs.xfs.d/default
+will be used to override system build-in defaults. Refer to mkfs.xfs.d (5)
+for a list of current defaults.
+Command line arguments directly passed to
+.B mkfs.xfs (8)
+will always override parameters set in the configuration file.
+You can override the configuration file used within the
+.B mkfs.xfs.d (5)
+directory by using the -c parameter. Alternatively
+you can set and use the MKFS_XFS_CONFIG environment variable to override
+the default full path of the configuration file which
+.B mkfs.xfs (8)
+looks for.
+If you use -c the configuration file must be present under
+.B mkfs.xfs.d (8).
+.PP
 In the descriptions below, sizes are given in sectors, bytes, blocks,
 kilobytes, megabytes, gigabytes, etc.
 Sizes are treated as hexadecimal if prefixed by 0x or 0X,
@@ -123,6 +143,11 @@  Many feature options allow an optional argument of 0 or 1, to explicitly
 disable or enable the functionality.
 .SH OPTIONS
 .TP
+.BI \-c " configuration-file"
+Override the configuration file used under the
+.B mkfs.xfs.d
+directory.
+.TP
 .BI \-b " block_size_options"
 This option specifies the fundamental block size of the filesystem.
 The valid
@@ -923,6 +948,7 @@  Prints the version number and exits.
 .SH SEE ALSO
 .BR xfs (5),
 .BR mkfs (8),
+.BR mkfs.xfs.d (5),
 .BR mount (8),
 .BR xfs_info (8),
 .BR xfs_admin (8).
diff --git a/mkfs/Makefile b/mkfs/Makefile
index c84f9b6ae63b..7906b1d4e67d 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -8,7 +8,7 @@  include $(TOPDIR)/include/builddefs
 LTCOMMAND = mkfs.xfs
 
 HFILES =
-CFILES = proto.c xfs_mkfs.c
+CFILES = proto.c xfs_mkfs.c xfs_mkfs_config.c
 
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index cb549be89835..aaf3f71a93cf 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -21,6 +21,7 @@ 
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
 #include "xfs_mkfs_common.h"
+#include "xfs_mkfs_config.h"
 #include "xfs_mkfs_cli.h"
 
 
@@ -3077,11 +3078,13 @@  print_mkfs_cfg(
 	struct mkfs_params	*cfg,
 	char			*dfile,
 	char			*logfile,
-	char			*rtfile)
+	char			*rtfile,
+	struct mkfs_default_params *dft)
 {
 	struct sb_feat_args	*fp = &cfg->sb_feat;
 
 	printf(_(
+"config-file=%-22s\n"
 "meta-data=%-22s isize=%-6d agcount=%lld, agsize=%lld blks\n"
 "         =%-22s sectsz=%-5u attr=%u, projid32bit=%u\n"
 "         =%-22s crc=%-8u finobt=%u, sparse=%u, rmapbt=%u, reflink=%u\n"
@@ -3091,6 +3094,7 @@  print_mkfs_cfg(
 "log      =%-22s bsize=%-6d blocks=%lld, version=%d\n"
 "         =%-22s sectsz=%-5u sunit=%d blks, lazy-count=%d\n"
 "realtime =%-22s extsz=%-6d blocks=%lld, rtextents=%lld\n"),
+		dft->type != DEFAULTS_BUILTIN ? dft->config_file : "empty",
 		dfile, cfg->inodesize, (long long)cfg->agcount,
 			(long long)cfg->agsize,
 		"", cfg->sectorsize, fp->attr_version, fp->projid32bit,
@@ -3665,6 +3669,7 @@  rewrite_secondary_superblocks(
 /* build time defaults */
 struct mkfs_default_params	built_in_dft = {
 	.type = DEFAULTS_BUILTIN,
+	.config_file = MKFS_XFS_CONF_DIR "default",
 	.sectorsize = XFS_MIN_SECTORSIZE,
 	.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
 	.sb_feat = {
@@ -3714,6 +3719,13 @@  static void process_defaults(
 	struct mkfs_default_params	*dft,
 	struct cli_params		*cli)
 {
+	int ret;
+
+	ret = parse_config_init(dft);
+
+	if (ret && dft->type != DEFAULTS_BUILTIN)
+		usage();
+
 	install_defaults(dft, cli);
 }
 
@@ -3750,6 +3762,8 @@  main(
 	};
 	struct mkfs_params	cfg = {};
 	struct mkfs_default_params	dft;
+	char *tmp_config;
+	char custom_config[PATH_MAX];
 
 	reset_defaults_and_cli(&dft, &cli);
 
@@ -3760,21 +3774,36 @@  main(
 	textdomain(PACKAGE);
 
 	/*
-	 * TODO: Sourcing defaults from a config file
-	 *
 	 * Before anything else, see if there's a config file with different
-	 * defaults. If a file exists in <package location>, read in the new
+	 * defaults. If a file exists in MKFS_XFS_CONF_DIR/default, read the new
 	 * default values and overwrite them in the &dft structure. This way the
 	 * new defaults will apply before we parse the CLI, and the CLI will
 	 * still be able to override them. When more than one source is
 	 * implemented, emit a message to indicate where the defaults being
 	 * used came from.
 	 */
+	tmp_config = getenv("MKFS_XFS_CONFIG");
+	if (tmp_config != NULL) {
+		dft.config_file = tmp_config;
+		dft.type = DEFAULTS_ENVIRONMENT_CONFIG;
+	}
 
 	process_defaults(&dft, &cli);
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
 		switch (c) {
+		case 'c':
+			reset_defaults_and_cli(&dft, &cli);
+
+			memset(custom_config, 0, sizeof(custom_config));
+			snprintf(custom_config, sizeof(custom_config), "%s%s",
+				 MKFS_XFS_CONF_DIR, optarg);
+			dft.config_file = custom_config;
+			dft.type = DEFAULTS_TYPE_CONFIG;
+
+			process_defaults(&dft, &cli);
+
+			break;
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
@@ -3823,10 +3852,8 @@  main(
 	} else
 		dfile = xi.dname;
 
-	/*
-	 * printf(_("Default configuration sourced from %s\n"),
-	 *	  default_type_str(dft.type));
-	 */
+	printf(_("Default configuration sourced from %s\n"),
+	       default_type_str(dft.type));
 
 	protostring = setup_proto(protofile);
 
@@ -3902,7 +3929,7 @@  main(
 	calculate_log_size(&cfg, &cli, mp);
 
 	if (!quiet || dry_run) {
-		print_mkfs_cfg(&cfg, dfile, logfile, rtfile);
+		print_mkfs_cfg(&cfg, dfile, logfile, rtfile, &dft);
 		if (dry_run)
 			exit(0);
 	}
diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
index d867ab377185..028bbf96017f 100644
--- a/mkfs/xfs_mkfs_common.h
+++ b/mkfs/xfs_mkfs_common.h
@@ -46,9 +46,20 @@  struct sb_feat_args {
  * These are the different possibilities by which you can end up parsing
  * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
  * file parsed and we are using the built-in defaults on this code.
+ * DEFAULTS_CONFIG means the default configuration file was found and used.
+ * DEFAULTS_TYPE_CONFIG means the user asked for a custom configuration type
+ * and it was used, this means the default directory for mkfs.xfs
+ * configuration files was used to look for the type specified. If you need
+ * to override the default mkfs.xfs directory for configuration file you can
+ * use the MKFS_XFS_CONFIG environment variable to set the absolute path to
+ * your custom configuration file, when this is set the type is set to
+ * DEFAULTS_ENVIRONMENT_CONFIG.
  */
 enum default_params_type {
 	DEFAULTS_BUILTIN = 0,
+	DEFAULTS_CONFIG,
+	DEFAULTS_ENVIRONMENT_CONFIG,
+	DEFAULTS_TYPE_CONFIG,
 };
 
 /*
@@ -61,6 +72,7 @@  enum default_params_type {
  */
 struct mkfs_default_params {
 	enum default_params_type type; /* where the defaults came from */
+	const char *config_file;
 
 	int	sectorsize;
 	int	blocksize;
diff --git a/mkfs/xfs_mkfs_config.c b/mkfs/xfs_mkfs_config.c
new file mode 100644
index 000000000000..d554638982ff
--- /dev/null
+++ b/mkfs/xfs_mkfs_config.c
@@ -0,0 +1,591 @@ 
+/*
+ * Copyright (c) 2018 Luis R. Rodriguez <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "xfs_mkfs_common.h"
+#include "xfs_mkfs_config.h"
+
+#define CONFIG_MAX_KEY		1024
+#define CONFIG_MAX_VALUE	PATH_MAX
+#define CONFIG_MAX_BUFFER	CONFIG_MAX_KEY + CONFIG_MAX_VALUE + 3
+
+/*
+ * Enums for each configuration option. All these currently match the CLI
+ * parameters for now but this may change later, so we keep all this code
+ * and definitions separate. The rules for configuration parameters may also
+ * differ.
+ */
+
+enum {
+	B_SIZE = 0,
+	B_MAX_OPTS,
+};
+
+enum {
+	D_AGCOUNT = 0,
+	D_FILE,
+	D_NAME,
+	D_SIZE,
+	D_SUNIT,
+	D_SWIDTH,
+	D_AGSIZE,
+	D_SU,
+	D_SW,
+	D_SECTSIZE,
+	D_NOALIGN,
+	D_RTINHERIT,
+	D_PROJINHERIT,
+	D_EXTSZINHERIT,
+	D_COWEXTSIZE,
+	D_MAX_OPTS,
+};
+
+enum {
+	I_ALIGN = 0,
+	I_MAXPCT,
+	I_PERBLOCK,
+	I_SIZE,
+	I_ATTR,
+	I_PROJID32BIT,
+	I_SPINODES,
+	I_MAX_OPTS,
+};
+
+enum {
+	L_AGNUM = 0,
+	L_INTERNAL,
+	L_SIZE,
+	L_VERSION,
+	L_SUNIT,
+	L_SU,
+	L_DEV,
+	L_SECTSIZE,
+	L_FILE,
+	L_NAME,
+	L_LAZYSBCNTR,
+	L_MAX_OPTS,
+};
+
+enum {
+	N_SIZE = 0,
+	N_VERSION,
+	N_FTYPE,
+	N_MAX_OPTS,
+};
+
+enum {
+	R_EXTSIZE = 0,
+	R_SIZE,
+	R_DEV,
+	R_FILE,
+	R_NAME,
+	R_NOALIGN,
+	R_MAX_OPTS,
+};
+
+enum {
+	S_SIZE = 0,
+	S_SECTSIZE,
+	S_MAX_OPTS,
+};
+
+enum {
+	M_CRC = 0,
+	M_FINOBT,
+	M_UUID,
+	M_RMAPBT,
+	M_REFLINK,
+	M_MAX_OPTS,
+};
+
+/* Just define the max options array size manually right now */
+#define MAX_SUBOPTS	D_MAX_OPTS
+
+enum mkfs_config_section {
+	MKFS_CONFIG_SECTION_BLOCK = 0,
+	MKFS_CONFIG_SECTION_DATA,
+	MKFS_CONFIG_SECTION_INODE,
+	MKFS_CONFIG_SECTION_LOG,
+	MKFS_CONFIG_SECTION_METADATA,
+	MKFS_CONFIG_SECTION_NAMING,
+	MKFS_CONFIG_SECTION_RTDEV,
+	MKFS_CONFIG_SECTION_SECTOR,
+
+	MKFS_CONFIG_SECTION_INVALID,
+};
+
+struct opt_params {
+	const enum mkfs_config_section section;
+	const char	*subopts[MAX_SUBOPTS];
+};
+
+extern struct opt_params config_sopts;
+
+struct opt_params config_bopts = {
+	.section = 'b',
+	.subopts = {
+		[B_SIZE] = "size",
+	},
+};
+
+struct opt_params config_dopts = {
+	.section = 'd',
+	.subopts = {
+		[D_AGCOUNT] = "agcount",
+		[D_FILE] = "file",
+		[D_NAME] = "name",
+		[D_SIZE] = "size",
+		[D_SUNIT] = "sunit",
+		[D_SWIDTH] = "swidth",
+		[D_AGSIZE] = "agsize",
+		[D_SU] = "su",
+		[D_SW] = "sw",
+		[D_SECTSIZE] = "sectsize",
+		[D_NOALIGN] = "noalign",
+		[D_RTINHERIT] = "rtinherit",
+		[D_PROJINHERIT] = "projinherit",
+		[D_EXTSZINHERIT] = "extszinherit",
+		[D_COWEXTSIZE] = "cowextsize",
+	},
+};
+
+struct opt_params config_iopts = {
+	.section = 'i',
+	.subopts = {
+		[I_ALIGN] = "align",
+		[I_MAXPCT] = "maxpct",
+		[I_PERBLOCK] = "perblock",
+		[I_SIZE] = "size",
+		[I_ATTR] = "attr",
+		[I_PROJID32BIT] = "projid32bit",
+		[I_SPINODES] = "sparse",
+	},
+};
+
+struct opt_params config_lopts = {
+	.section = 'l',
+	.subopts = {
+		[L_AGNUM] = "agnum",
+		[L_INTERNAL] = "internal",
+		[L_SIZE] = "size",
+		[L_VERSION] = "version",
+		[L_SUNIT] = "sunit",
+		[L_SU] = "su",
+		[L_DEV] = "logdev",
+		[L_SECTSIZE] = "sectsize",
+		[L_FILE] = "file",
+		[L_NAME] = "name",
+		[L_LAZYSBCNTR] = "lazy-count",
+	},
+};
+
+struct opt_params config_nopts = {
+	.section = 'n',
+	.subopts = {
+		[N_SIZE] = "size",
+		[N_VERSION] = "version",
+		[N_FTYPE] = "ftype",
+	},
+};
+
+struct opt_params config_ropts = {
+	.section = 'r',
+	.subopts = {
+		[R_EXTSIZE] = "extsize",
+		[R_SIZE] = "size",
+		[R_DEV] = "rtdev",
+		[R_FILE] = "file",
+		[R_NAME] = "name",
+		[R_NOALIGN] = "noalign",
+	},
+};
+
+struct opt_params config_sopts = {
+	.section = 's',
+	.subopts = {
+		[S_SIZE] = "size",
+		[S_SECTSIZE] = "sectsize",
+	},
+};
+
+struct opt_params config_mopts = {
+	.section = 'm',
+	.subopts = {
+		[M_CRC] = "crc",
+		[M_FINOBT] = "finobt",
+		[M_UUID] = "uuid",
+		[M_RMAPBT] = "rmapbt",
+		[M_REFLINK] = "reflink",
+	},
+};
+
+const char *default_type_str(enum default_params_type type)
+{
+	switch (type) {
+	case DEFAULTS_BUILTIN:
+		return _("package built-in definitions");
+	case DEFAULTS_CONFIG:
+		return _("default configuration system file");
+	case DEFAULTS_ENVIRONMENT_CONFIG:
+		return _("custom configuration file set by environment");
+	case DEFAULTS_TYPE_CONFIG:
+		return _("custom configuration type file");
+	}
+	return _("Unkown\n");
+}
+
+static int block_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	return -EINVAL;
+}
+
+static int data_config_parser(struct mkfs_default_params *dft, int subopt,
+			      bool value)
+{
+	switch (subopt) {
+	case D_NOALIGN:
+		dft->sb_feat.nodalign = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int inode_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	switch (subopt) {
+	case I_ALIGN:
+		dft->sb_feat.inode_align = value;
+		break;
+	case I_PROJID32BIT:
+		dft->sb_feat.projid32bit = value;
+		break;
+	case I_SPINODES:
+		dft->sb_feat.spinodes = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int log_config_parser(struct mkfs_default_params *dft, int subopt,
+			     bool value)
+{
+	switch (subopt) {
+	case L_LAZYSBCNTR:
+		dft->sb_feat.lazy_sb_counters = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int meta_config_parser(struct mkfs_default_params *dft, int subopt,
+			      bool value)
+{
+	switch (subopt) {
+	case M_CRC:
+		dft->sb_feat.crcs_enabled = value;
+		if (dft->sb_feat.crcs_enabled)
+			dft->sb_feat.dirftype = true;
+		break;
+	case M_FINOBT:
+		dft->sb_feat.finobt = value;
+		break;
+	case M_RMAPBT:
+		dft->sb_feat.rmapbt = value;
+		break;
+	case M_REFLINK:
+		dft->sb_feat.reflink = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int naming_config_parser(struct mkfs_default_params *dft, int subopt,
+				bool value)
+{
+	switch (subopt) {
+	case N_FTYPE:
+		dft->sb_feat.dirftype = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int rtdev_config_parser(struct mkfs_default_params *dft, int subopt,
+			       bool value)
+{
+	switch (subopt) {
+	case R_NOALIGN:
+		dft->sb_feat.nortalign = value;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int sector_config_parser(struct mkfs_default_params *dft, int subopt,
+				bool value)
+{
+	return -EINVAL;
+}
+
+struct config_subopts {
+	enum mkfs_config_section	section;
+	struct opt_params		*opts;
+	int (*config_parser)(struct mkfs_default_params *dft,
+			     int subop, bool value);
+} config_subopt_tab[] = {
+	{ MKFS_CONFIG_SECTION_BLOCK, &config_bopts, block_config_parser },
+	{ MKFS_CONFIG_SECTION_DATA, &config_dopts, data_config_parser },
+	{ MKFS_CONFIG_SECTION_INODE, &config_iopts, inode_config_parser },
+	{ MKFS_CONFIG_SECTION_LOG, &config_lopts, log_config_parser },
+	{ MKFS_CONFIG_SECTION_METADATA, &config_mopts, meta_config_parser },
+	{ MKFS_CONFIG_SECTION_NAMING, &config_nopts, naming_config_parser },
+	{ MKFS_CONFIG_SECTION_RTDEV, &config_ropts, rtdev_config_parser },
+	{ MKFS_CONFIG_SECTION_SECTOR, &config_sopts, sector_config_parser },
+	{ '\0', NULL },
+};
+
+static int
+parse_config_subopts(
+	enum mkfs_config_section	section,
+	bool				value,
+	char				*line,
+	struct mkfs_default_params	*dft)
+{
+	struct config_subopts *sop = &config_subopt_tab[0];
+	char	**subopts = (char **)sop->opts->subopts;
+	int	subopt;
+	char *val;
+
+	while (sop->opts) {
+		if (sop->section == section)
+			break;
+		sop++;
+	}
+
+	/* should never happen */
+	if (!sop->opts)
+		return -EINVAL;
+
+	subopts = (char **)sop->opts->subopts;
+	subopt = getsubopt(&line, subopts, &val);
+	return (sop->config_parser)(dft, subopt, value);
+}
+
+static enum mkfs_config_section get_config_section(const char *buffer)
+{
+	if (strncmp("block", buffer, 5) == 0)
+		return MKFS_CONFIG_SECTION_BLOCK;
+	if (strncmp("data", buffer, 4) == 0)
+		return MKFS_CONFIG_SECTION_DATA;
+	if (strncmp("inode", buffer, 5) == 0)
+		return MKFS_CONFIG_SECTION_INODE;
+	if (strncmp("log", buffer, 3) == 0)
+		return MKFS_CONFIG_SECTION_LOG;
+	if (strncmp("metadata", buffer, 8) == 0)
+		return MKFS_CONFIG_SECTION_METADATA;
+	if (strncmp("naming", buffer, 6) == 0)
+		return MKFS_CONFIG_SECTION_NAMING;
+	if (strncmp("rtdev", buffer, 5) == 0)
+		return MKFS_CONFIG_SECTION_RTDEV;
+	if (strncmp("sector", buffer, 6) == 0)
+		return MKFS_CONFIG_SECTION_SECTOR;
+
+	return MKFS_CONFIG_SECTION_INVALID;
+}
+
+static int mkfs_check_config_file_limits(const char *filename,
+					 const struct stat *sb,
+					 unsigned int max_entries)
+{
+	unsigned long long size_limit;
+
+	size_limit = CONFIG_MAX_BUFFER * max_entries;
+
+	/*
+	 * Detect wrap around -- if max_entries * size_limit goves over
+	 * unsigned long long we detect that there. Note some libraries,
+	 * like libiniconfig, only groks max INT_MAX entries anyway, so
+	 * if we ever use a library for parsing we'd be restricted to that
+	 * limit.
+	 */
+	if (size_limit < max_entries || max_entries > INT_MAX)
+		return -E2BIG;
+
+	if (sb->st_size > size_limit) {
+		fprintf(stderr,
+			"File %s is too big! File size limit: %llu\n",
+			filename, size_limit);
+		return -E2BIG;
+	}
+
+	return 0;
+}
+
+enum parse_line_type {
+	PARSE_COMMENT = 0,
+	PARSE_SECTION,
+	PARSE_TAG_VALUE,
+	PARSE_INVALID,
+	PARSE_EOF,
+};
+
+static int parse_line_section(const char *line, char *tag)
+{
+	return sscanf(line, " [%[^]]s]", tag);
+}
+
+static int parse_line_tag_value(const char *line, char *tag,
+				unsigned int *value)
+{
+	return sscanf(line, " %[^ \t=]"
+		      " = "
+		      "%u ",
+		      tag, value);
+}
+
+static enum parse_line_type parse_get_line_type(const char *line, char *tag,
+						bool *value)
+{
+	int ret;
+	unsigned int uint_value;
+
+	memset(tag, 0, 80);
+
+	if (strlen(line) < 2)
+		return PARSE_INVALID;
+
+	if (line[0] == '#')
+		return PARSE_COMMENT;
+
+	ret = parse_line_section(line, tag);
+	if (ret == 1)
+		return  PARSE_SECTION;
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	ret = parse_line_tag_value(line, tag, &uint_value);
+	if (ret == 2) {
+		if (uint_value != 1 && uint_value != 0)
+			return -EINVAL;
+
+		*value = !!uint_value;
+
+		return PARSE_TAG_VALUE;
+	}
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	return PARSE_INVALID;
+}
+
+int parse_config_init(struct mkfs_default_params *dft)
+{
+	int ret;
+	FILE *fp;
+	struct stat sp;
+	unsigned int num_subopts_sections = sizeof(config_subopt_tab) /
+					    sizeof(struct config_subopts) - 1;
+	char *p;
+	char line[80], tag[80];
+	bool value;
+	enum parse_line_type parse_type;
+	enum mkfs_config_section section = MKFS_CONFIG_SECTION_INVALID;
+
+	fp = fopen(dft->config_file, "r");
+	if (!fp) {
+		if (dft->type != DEFAULTS_BUILTIN) {
+			fprintf(stderr, _("could not open config file: %s\n"),
+				dft->config_file);
+			ret = -ENOENT;
+		} else
+			ret = 0;
+		return ret;
+	}
+
+	/*
+	 * If we found a file without it being specified on the command line,
+	 * it would be the default configuration file.
+	 */
+	if (dft->type == DEFAULTS_BUILTIN)
+		dft->type = DEFAULTS_CONFIG;
+
+	ret = stat(dft->config_file, &sp);
+	if (ret) {
+		if (dft->type != DEFAULTS_BUILTIN)
+			fprintf(stderr, _("could not stat() config file: %s: %s\n"),
+				dft->config_file, strerror(ret));
+		return ret;
+	}
+
+	if (!sp.st_size)
+		return 0;
+
+	ret = mkfs_check_config_file_limits(dft->config_file, &sp,
+					    num_subopts_sections);
+	if (ret)
+		return ret;
+
+	while (!feof(fp)) {
+		p = fgets(line, sizeof(line), fp);
+		if (!p)
+			continue;
+
+		parse_type = parse_get_line_type(line, tag, &value);
+
+		switch (parse_type) {
+		case PARSE_COMMENT:
+		case PARSE_INVALID:
+		case PARSE_EOF:
+			break;
+		case PARSE_SECTION:
+			section = get_config_section(tag);
+			if (!section) {
+				fprintf(stderr, _("Invalid section: %s\n"),
+						tag);
+				return -EINVAL;
+			}
+			break;
+		case PARSE_TAG_VALUE:
+			/* Trims white spaces */
+			snprintf(line, sizeof(line), "%s=%u", tag, value);
+			ret = parse_config_subopts(section, value, line, dft);
+			if (ret) {
+				fprintf(stderr, _("Error parsine line: %s\n"),
+						line);
+				return ret;
+			}
+			break;
+		}
+	}
+
+	return 0;
+}
diff --git a/mkfs/xfs_mkfs_config.h b/mkfs/xfs_mkfs_config.h
new file mode 100644
index 000000000000..407d37fffb1a
--- /dev/null
+++ b/mkfs/xfs_mkfs_config.h
@@ -0,0 +1,11 @@ 
+#ifndef _XFS_MKFS_CONFIG_H
+#define _XFS_MKFS_CONFIG_H
+
+#include "xfs_mkfs_common.h"
+
+#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d/"
+
+const char *default_type_str(enum default_params_type type);
+int parse_config_init(struct mkfs_default_params *dft);
+
+#endif /* _XFS_MKFS_CONFIG_H */