[PATCH v3 0/4] xfsprogs: add mkfs.xfs configuration file parsing support

[PATCH v3 0/4] xfsprogs: add mkfs.xfs configuration file parsing support

[Luis R. Rodriguez]

This is v3 series should address all prior feedback from the last
iteration. If I forgot something please let me know. The changes that
went into this v3 and prior are listed below. You can fetch the goods on
my kernel.org xfsprogs-dev 20180524-own-parser branch [0].

Lemme know what ya'll think.

  Luis

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/xfsprogs-dev.git/log/?h=20180524-own-parser

Changes on v3:

- Prefix errors on the configuration parsing with config file used and
  exact line.

- Use uint64_t throughout all parsers as requested.

- Parse the CLI early for -c as I originally had implemented this on v1
  but now platform_getoptreset() to reset the opts.

- Drop the enum for the source type for the configuration file type,
  note that since we rely on the stack to set the variables we should
  then always set the dft.src on main().

- Use getline() for fetching lines as requested

- Add a special iscomment() and isempty() to handle all space and
  comment parsing. Now when PARSE_INVALID issued we really mean it.

- Embrace latest bikeshed preferences:
	config.c, config.h, random function names

- Add man pages to be parsed via configure so that @sysconfigdir@ gets
  properly parsed

- get_confopts() now iterates over the known sections tab and gives you
  the right struct right away.

- Add respecification checks for a section.

- Add respecification checks for -c, -c is only allowed once.

- Add --sysconfigdir to debian/rules, note that if you *don't* set
  --sysconfigdir your man pages will end up with ${prefix}. We could
  add a secondary target parsing just in case, or we can do some hacks
  with autoconf for this, but I don't think its worth it. Other packages
  deal with this by having ./configure always run with --sysconfigdir
  set (see systemd for instance).

- Reduced the number of declared enums, only enums for the config
  subparams which are currently supported for parsing are declared.

- mkfs.xfs -c foo now will search for $PWD/foo and if that fails the
  sysconfigdir/mkfs.xfs.d/foo.

- mkfs.xfs -c ../foo works and so should ./foo, etc.

- The MKFS_XFS_CONFIG variable support was dropped in favor or just
  allowing the user to specify the full path now.

- Embraces xfsprogs coding style, c'est la vie.

Changes on v2:

- Stayed with our own parser as its the smallest and we're willing to
  maintain it as its simple and clear.

- Use -c for the configuration file, and drop the "type" nomenclature to
  avoid confusion on the interwebs.

- Start to split files off

- Duplicate a bit of items as suggested at LSFMM for the configuration
  parser structures. We can later consolidate if we think its really
  needed, however we want the freedom to change these as we see fit and
  most importantly keep the code apart.
                                                                                                                                                                                              
- Conflict resolution and validation is managed now by piggy backing off
  of the idea of using the defaults to instantiate CLI parameters. CLI
  always overrides.

Luis R. Rodriguez (4):
  mkfs: distinguish between struct sb_feat_args and struct cli_params
  mkfs: move shared config structs and into their own headers
  mkfs.xfs: add configuration file parsing support using our own parser
  debian/rules: use the new sysconfdir configuration setting

 configure.ac                           |   6 +-
 debian/rules                           |   4 +-
 include/builddefs.in                   |   2 +
 man/man5/Makefile                      |   2 +
 man/man5/mkfs.xfs.d.5.in               | 153 ++++++++
 man/man8/Makefile                      |   2 +
 man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} |  27 ++
 mkfs/Makefile                          |   2 +-
 mkfs/config.c                          | 644 +++++++++++++++++++++++++++++++++
 mkfs/config.h                          |  86 +++++
 mkfs/xfs_mkfs.c                        | 130 ++++---
 11 files changed, 995 insertions(+), 63 deletions(-)
 create mode 100644 man/man5/mkfs.xfs.d.5.in
 rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%)
 create mode 100644 mkfs/config.c
 create mode 100644 mkfs/config.h

-- 
2.16.3

[PATCH v3 1/4] mkfs: distinguish between struct sb_feat_args and struct cli_params

[Luis R. Rodriguez]

The struct sb_feat_args will actually be shared between the code which
processes command line options and the configuration file, as such we
need to clarify and reflect this clearly in documentation.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 mkfs/xfs_mkfs.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 3804814b3b8a..95cd6ced13f0 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -707,26 +707,18 @@ cli_opt_set(
 }
 
 /*
- * Options configured on the command line.
- *
- * This stores all the specific config parameters the user sets on the command
- * line. We do not use these values directly - they are inputs to the mkfs
- * geometry validation and override any default configuration value we have.
+ * Shared superblock configuration options
  *
- * We don't keep flags to indicate what parameters are set - if we need to check
- * if an option was set on the command line, we check the relevant entry in the
- * option table which records whether it was specified in the .seen and
- * .str_seen variables in the table.
+ * These options provide shared configuration tunables for the filesystem
+ * superblock. There are three possible sources for these options set, each
+ * source can overriding the later source:
  *
- * Some parameters are stored as strings for post-parsing after their dependent
- * options have been resolved (e.g. block size and sector size have been parsed
- * and validated).
+ * 	o built-in defaults
+ * 	o configuration file (XXX)
+ * 	o command line
  *
- * This allows us to check that values have been set without needing separate
- * flags for each value, and hence avoids needing to record and check for each
- * specific option that can set the value later on in the code. In the cases
- * where we don't have a cli_params structure around, the above cli_opt_set()
- * function can be used.
+ * These values are not used directly - they are inputs into the mkfs geometry
+ * validation.
  */
 struct sb_feat_args {
 	int	log_version;
@@ -747,6 +739,25 @@ struct sb_feat_args {
 	bool	nortalign;
 };
 
+/*
+ * Options configured on the command line.
+ *
+ * This stores all the specific config parameters the user sets on the command
+ * line.  We don't keep flags to indicate what parameters are set - if we need
+ * to check if an option was set on the command line, we check the relevant
+ * entry in the option table which records whether it was specified in the
+ * .seen and .str_seen variables in the table.
+ *
+ * Some parameters are stored as strings for post-parsing after their dependent
+ * options have been resolved (e.g. block size and sector size have been parsed
+ * and validated).
+ *
+ * This allows us to check that values have been set without needing separate
+ * flags for each value, and hence avoids needing to record and check for each
+ * specific option that can set the value later on in the code. In the cases
+ * where we don't have a cli_params structure around, the function cli_opt_set()
+ * function can be used.
+ */
 struct cli_params {
 	int	sectorsize;
 	int	blocksize;
-- 
2.16.3

Re: [PATCH v3 1/4] mkfs: distinguish between struct sb_feat_args and struct cli_params

[Darrick J. Wong]

On Thu, May 24, 2018 at 08:19:40PM -0700, Luis R. Rodriguez wrote:
> The struct sb_feat_args will actually be shared between the code which
> processes command line options and the configuration file, as such we
> need to clarify and reflect this clearly in documentation.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  mkfs/xfs_mkfs.c | 45 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 3804814b3b8a..95cd6ced13f0 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -707,26 +707,18 @@ cli_opt_set(
>  }
>  
>  /*
> - * Options configured on the command line.
> - *
> - * This stores all the specific config parameters the user sets on the command
> - * line. We do not use these values directly - they are inputs to the mkfs
> - * geometry validation and override any default configuration value we have.
> + * Shared superblock configuration options
>   *
> - * We don't keep flags to indicate what parameters are set - if we need to check
> - * if an option was set on the command line, we check the relevant entry in the
> - * option table which records whether it was specified in the .seen and
> - * .str_seen variables in the table.
> + * These options provide shared configuration tunables for the filesystem
> + * superblock. There are three possible sources for these options set, each
> + * source can overriding the later source:
>   *
> - * Some parameters are stored as strings for post-parsing after their dependent
> - * options have been resolved (e.g. block size and sector size have been parsed
> - * and validated).
> + * 	o built-in defaults
> + * 	o configuration file (XXX)
> + * 	o command line
>   *
> - * This allows us to check that values have been set without needing separate
> - * flags for each value, and hence avoids needing to record and check for each
> - * specific option that can set the value later on in the code. In the cases
> - * where we don't have a cli_params structure around, the above cli_opt_set()
> - * function can be used.
> + * These values are not used directly - they are inputs into the mkfs geometry
> + * validation.
>   */
>  struct sb_feat_args {
>  	int	log_version;
> @@ -747,6 +739,25 @@ struct sb_feat_args {
>  	bool	nortalign;
>  };
>  
> +/*
> + * Options configured on the command line.
> + *
> + * This stores all the specific config parameters the user sets on the command
> + * line.  We don't keep flags to indicate what parameters are set - if we need
> + * to check if an option was set on the command line, we check the relevant
> + * entry in the option table which records whether it was specified in the
> + * .seen and .str_seen variables in the table.
> + *
> + * Some parameters are stored as strings for post-parsing after their dependent
> + * options have been resolved (e.g. block size and sector size have been parsed
> + * and validated).
> + *
> + * This allows us to check that values have been set without needing separate
> + * flags for each value, and hence avoids needing to record and check for each
> + * specific option that can set the value later on in the code. In the cases
> + * where we don't have a cli_params structure around, the function cli_opt_set()
> + * function can be used.
> + */
>  struct cli_params {
>  	int	sectorsize;
>  	int	blocksize;
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/4] mkfs: move shared config structs and into their own headers

[Luis R. Rodriguez]

Both struct sb_feat_args and struct mkfs_default_params will be shared
between CLI processing and the configuration file processing added later,
so move these to their own header.

This will help ensure we split things neatly later and also will help
ensure the configuration file processing code from the CLI code are kept
separate and cannot touch each other's data structures. This also makes
it clear what is actually shared between both.

There are no introduced functional changes in this commit and no
documentation changes, this is just code shuffling.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 mkfs/config.h   | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/xfs_mkfs.c | 55 +---------------------------------------
 2 files changed, 79 insertions(+), 54 deletions(-)
 create mode 100644 mkfs/config.h

diff --git a/mkfs/config.h b/mkfs/config.h
new file mode 100644
index 000000000000..e5ea968e2d65
--- /dev/null
+++ b/mkfs/config.h
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * Copyright (c) 2016-2017 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * 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
+ */
+#ifndef _XFS_MKFS_CONFIG_H
+#define _XFS_MKFS_CONFIG_H
+
+struct fsxattr;
+
+/*
+ * Shared superblock configuration options
+ *
+ * These options provide shared configuration tunables for the filesystem
+ * superblock. There are three possible sources for these options set, each
+ * source can overriding the later source:
+ *
+ * 	o built-in defaults
+ * 	o configuration file (XXX)
+ * 	o command line
+ *
+ * These values are not used directly - they are inputs into the mkfs geometry
+ * validation.
+ */
+struct sb_feat_args {
+	int	log_version;
+	int	attr_version;
+	int	dir_version;
+	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
+	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
+	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
+	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
+	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
+	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
+	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
+	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
+	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
+	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
+	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
+	bool	nodalign;
+	bool	nortalign;
+};
+
+/*
+ * Default filesystem features and configuration values
+ *
+ * This structure contains the default mkfs values that are to be used when
+ * a user does not specify the option on the command line. We do not use these
+ * values directly - they are inputs to the mkfs geometry validation and
+ * calculations.
+ */
+struct mkfs_default_params {
+	char	*source;	/* where the defaults came from */
+
+	int	sectorsize;
+	int	blocksize;
+
+	/* feature flags that are set */
+	struct sb_feat_args	sb_feat;
+
+	/* root inode characteristics */
+	struct fsxattr		fsx;
+};
+
+#endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 95cd6ced13f0..217bb972538d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -20,6 +20,7 @@
 #include <ctype.h>
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
+#include "config.h"
 
 
 
@@ -706,39 +707,6 @@ cli_opt_set(
 	       opts->subopt_params[subopt].str_seen;
 }
 
-/*
- * Shared superblock configuration options
- *
- * These options provide shared configuration tunables for the filesystem
- * superblock. There are three possible sources for these options set, each
- * source can overriding the later source:
- *
- * 	o built-in defaults
- * 	o configuration file (XXX)
- * 	o command line
- *
- * These values are not used directly - they are inputs into the mkfs geometry
- * validation.
- */
-struct sb_feat_args {
-	int	log_version;
-	int	attr_version;
-	int	dir_version;
-	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
-	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
-	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
-	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
-	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
-	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
-	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
-	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
-	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
-	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
-	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
-	bool	nodalign;
-	bool	nortalign;
-};
-
 /*
  * Options configured on the command line.
  *
@@ -850,27 +818,6 @@ struct mkfs_params {
 	struct sb_feat_args	sb_feat;
 };
 
-/*
- * Default filesystem features and configuration values
- *
- * This structure contains the default mkfs values that are to be used when
- * a user does not specify the option on the command line. We do not use these
- * values directly - they are inputs to the mkfs geometry validation and
- * calculations.
- */
-struct mkfs_default_params {
-	char	*source;	/* where the defaults came from */
-
-	int	sectorsize;
-	int	blocksize;
-
-	/* feature flags that are set */
-	struct sb_feat_args	sb_feat;
-
-	/* root inode characteristics */
-	struct fsxattr		fsx;
-};
-
 static void __attribute__((noreturn))
 usage( void )
 {
-- 
2.16.3

Re: [PATCH v3 2/4] mkfs: move shared config structs and into their own headers

[Darrick J. Wong]

On Thu, May 24, 2018 at 08:19:41PM -0700, Luis R. Rodriguez wrote:
> Both struct sb_feat_args and struct mkfs_default_params will be shared
> between CLI processing and the configuration file processing added later,
> so move these to their own header.
> 
> This will help ensure we split things neatly later and also will help
> ensure the configuration file processing code from the CLI code are kept
> separate and cannot touch each other's data structures. This also makes
> it clear what is actually shared between both.
> 
> There are no introduced functional changes in this commit and no
> documentation changes, this is just code shuffling.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
>  mkfs/config.h   | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  mkfs/xfs_mkfs.c | 55 +---------------------------------------
>  2 files changed, 79 insertions(+), 54 deletions(-)
>  create mode 100644 mkfs/config.h
> 
> diff --git a/mkfs/config.h b/mkfs/config.h
> new file mode 100644
> index 000000000000..e5ea968e2d65
> --- /dev/null
> +++ b/mkfs/config.h
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (c) 2000-2005 Silicon Graphics, Inc.
> + * Copyright (c) 2016-2017 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * 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
> + */
> +#ifndef _XFS_MKFS_CONFIG_H
> +#define _XFS_MKFS_CONFIG_H
> +
> +struct fsxattr;
> +
> +/*
> + * Shared superblock configuration options
> + *
> + * These options provide shared configuration tunables for the filesystem
> + * superblock. There are three possible sources for these options set, each
> + * source can overriding the later source:
> + *
> + * 	o built-in defaults
> + * 	o configuration file (XXX)
> + * 	o command line
> + *
> + * These values are not used directly - they are inputs into the mkfs geometry
> + * validation.
> + */
> +struct sb_feat_args {
> +	int	log_version;
> +	int	attr_version;
> +	int	dir_version;
> +	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
> +	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
> +	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
> +	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
> +	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
> +	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
> +	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
> +	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
> +	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
> +	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
> +	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
> +	bool	nodalign;
> +	bool	nortalign;
> +};
> +
> +/*
> + * Default filesystem features and configuration values
> + *
> + * This structure contains the default mkfs values that are to be used when
> + * a user does not specify the option on the command line. We do not use these
> + * values directly - they are inputs to the mkfs geometry validation and
> + * calculations.
> + */
> +struct mkfs_default_params {
> +	char	*source;	/* where the defaults came from */
> +
> +	int	sectorsize;
> +	int	blocksize;

Indentation ^^^ is uneven with the rest of the struct, but otherwise:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +
> +	/* feature flags that are set */
> +	struct sb_feat_args	sb_feat;
> +
> +	/* root inode characteristics */
> +	struct fsxattr		fsx;
> +};
> +
> +#endif /* _XFS_MKFS_CONFIG_H */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 95cd6ced13f0..217bb972538d 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -20,6 +20,7 @@
>  #include <ctype.h>
>  #include "xfs_multidisk.h"
>  #include "libxcmd.h"
> +#include "config.h"
>  
>  
>  
> @@ -706,39 +707,6 @@ cli_opt_set(
>  	       opts->subopt_params[subopt].str_seen;
>  }
>  
> -/*
> - * Shared superblock configuration options
> - *
> - * These options provide shared configuration tunables for the filesystem
> - * superblock. There are three possible sources for these options set, each
> - * source can overriding the later source:
> - *
> - * 	o built-in defaults
> - * 	o configuration file (XXX)
> - * 	o command line
> - *
> - * These values are not used directly - they are inputs into the mkfs geometry
> - * validation.
> - */
> -struct sb_feat_args {
> -	int	log_version;
> -	int	attr_version;
> -	int	dir_version;
> -	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
> -	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
> -	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
> -	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
> -	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
> -	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
> -	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
> -	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
> -	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
> -	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
> -	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
> -	bool	nodalign;
> -	bool	nortalign;
> -};
> -
>  /*
>   * Options configured on the command line.
>   *
> @@ -850,27 +818,6 @@ struct mkfs_params {
>  	struct sb_feat_args	sb_feat;
>  };
>  
> -/*
> - * Default filesystem features and configuration values
> - *
> - * This structure contains the default mkfs values that are to be used when
> - * a user does not specify the option on the command line. We do not use these
> - * values directly - they are inputs to the mkfs geometry validation and
> - * calculations.
> - */
> -struct mkfs_default_params {
> -	char	*source;	/* where the defaults came from */
> -
> -	int	sectorsize;
> -	int	blocksize;
> -
> -	/* feature flags that are set */
> -	struct sb_feat_args	sb_feat;
> -
> -	/* root inode characteristics */
> -	struct fsxattr		fsx;
> -};
> -
>  static void __attribute__((noreturn))
>  usage( void )
>  {
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] mkfs: move shared config structs and into their own headers

[Luis R. Rodriguez]

On Thu, May 24, 2018 at 08:35:48PM -0700, Darrick J. Wong wrote:
> On Thu, May 24, 2018 at 08:19:41PM -0700, Luis R. Rodriguez wrote:
> > diff --git a/mkfs/config.h b/mkfs/config.h
> > new file mode 100644
> > index 000000000000..e5ea968e2d65
> > --- /dev/null
> > +++ b/mkfs/config.h
> > +struct mkfs_default_params {
> > +	char	*source;	/* where the defaults came from */
> > +
> > +	int	sectorsize;
> > +	int	blocksize;
> 
> Indentation ^^^ is uneven with the rest of the struct, but otherwise:

Bug-compatible :P -- ie it was in the old code IIRC, as all I did was
copy and paste on my editor between the two.

> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

  Luis

[PATCH v3 3/4] mkfs.xfs: add configuration file parsing support using our own parser

[Luis R. Rodriguez]

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 search path for the configuration file will be:

	1) $PWD/experimental
	2) /etc/mkfs.xfs.d/experimental

Absolute paths are supported, in which case they will be used directly
and the mkfs.xfs.d directory is ignored.

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. 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 <mcgrof@kernel.org>
---
 configure.ac                           |   6 +-
 include/builddefs.in                   |   2 +
 man/man5/Makefile                      |   2 +
 man/man5/mkfs.xfs.d.5.in               | 153 ++++++++
 man/man8/Makefile                      |   2 +
 man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} |  27 ++
 mkfs/Makefile                          |   2 +-
 mkfs/config.c                          | 644 +++++++++++++++++++++++++++++++++
 mkfs/config.h                          |  10 +-
 mkfs/xfs_mkfs.c                        |  76 +++-
 10 files changed, 910 insertions(+), 14 deletions(-)
 create mode 100644 man/man5/mkfs.xfs.d.5.in
 rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%)
 create mode 100644 mkfs/config.c

diff --git a/configure.ac b/configure.ac
index 508eefede073..94c5bda725f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -233,5 +233,9 @@ AC_CHECK_SIZEOF([char *])
 AC_TYPE_UMODE_T
 AC_MANUAL_FORMAT
 
-AC_CONFIG_FILES([include/builddefs])
+AC_CONFIG_FILES([
+	include/builddefs
+	man/man5/mkfs.xfs.d.5
+	man/man8/mkfs.xfs.8
+])
 AC_OUTPUT
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/Makefile b/man/man5/Makefile
index fe0aef6f016b..0b33122b064e 100644
--- a/man/man5/Makefile
+++ b/man/man5/Makefile
@@ -19,3 +19,5 @@ install : default
 	$(INSTALL) -m 755 -d $(MAN_DEST)
 	$(INSTALL_MAN)
 install-dev :
+
+LDIRT += mkfs.xfs.d.5
diff --git a/man/man5/mkfs.xfs.d.5.in b/man/man5/mkfs.xfs.d.5.in
new file mode 100644
index 000000000000..c8dac58f5c9d
--- /dev/null
+++ b/man/man5/mkfs.xfs.d.5.in
@@ -0,0 +1,153 @@
+.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.
+
+The built-in mkfs defaults are decided by the XFS community. New features are
+only enabled by default when the community consider it stable.  One can
+override these defaults on the
+.B mkfs.xfs (8)
+command line, but there are cases where it is desirable for the distro or the
+system administrator to establish their own supported defaults in a uniform
+manner, regardless of the version of
+.B mkfs.xfs (8)
+used. This may desirable for example on systems with old kernels
+where the built-in default
+.B mkfs.xfs (8)
+parameters create a filesystem that is not supported by the old kernel.
+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. 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 as a home to define different configuration files which can be used
+to override the built-in default parameters by
+.B mkfs.xfs (8).
+If the
+.B -c
+parameter is not used, the default configuration file:
+.IP
+.I @sysconfdir@/mkfs.xfs.d/default
+.PP
+will be looked for first and if present will be used to override
+.B mkfs.xfs (8)
+built-in default parameters.
+.PP
+You can override the configuration file by specifying its name when using
+.B mkfs.xfs (8)
+by using the
+.B -c
+parameter.
+.PP
+If a relative path without the special character '.' is passed,
+the configuration file is first looked for in you current working directory;
+if the configuration file is not present in your current working directory
+the configuration file with the name given is looked in the
+.B mkfs.xfs.d (5)
+directory.
+.PP
+If
+.B -c
+is used with relative path with which has a leading '.' character, the given
+path is used directly, so the configuration file will be relative to your
+current directory.
+.PP
+If
+.B -c
+is used with an absolute path -- a path with a leading '/' character --
+then the absolute path given is used for the configuration file.
+.PP
+For example:
+.IP
+.B mkfs.xfs -c experimental -f /dev/sda1
+.PP
+Will make
+.B mkfs.xfs (8)
+look for the following configuration files and use the first one it finds:
+.IP
+.B $PWD/experimental
+.br
+.B @sysconfdir@/mkfs.xfs.d/experimental
+.PP
+If you used an absolute path, for example:
+.IP
+.B mkfs.xfs -c /tmp/experimental -f /dev/sda1
+.PP
+Then only the configuration file /tmp/experimental will be looked for and
+used if present.
+.PP
+If you use the
+.B -c
+parameter the configuration file must be present and must parse correctly.
+.PP
+Parameters 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/Makefile b/man/man8/Makefile
index 36620da172ae..2a6548079997 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -19,3 +19,5 @@ install : default
 	$(INSTALL) -m 755 -d $(MAN_DEST)
 	$(INSTALL_MAN)
 install-dev :
+
+LDIRT += mkfs.xfs.8
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8.in
similarity index 96%
rename from man/man8/mkfs.xfs.8
rename to man/man8/mkfs.xfs.8.in
index 4b8c78c37806..81e2753bd2b5 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8.in
@@ -83,6 +83,24 @@ 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 different default parameters which can be used when
+calling
+.B mkfs.xfs (8).
+If present, and if
+.B -c
+is not used, the default configuration file @sysconfigdir@/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 and further details.
+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 by using the
+.B -c
+parameter, further explained below and in
+.B mkfs.xfs.d (5)
+.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 +141,14 @@ 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. If a relative path is given the search
+path for the configuration file is your current directory and then the
+.B mkfs.xfs.d (5)
+directory. If an absolute path is given it is used directly. For more details
+refer to
+.B mkfs.xfs.d (5)
+.TP
 .BI \-b " block_size_options"
 This option specifies the fundamental block size of the filesystem.
 The valid
@@ -923,6 +949,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..c7815b3e106b 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 config.c
 
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
diff --git a/mkfs/config.c b/mkfs/config.c
new file mode 100644
index 000000000000..2d21728b7b62
--- /dev/null
+++ b/mkfs/config.c
@@ -0,0 +1,644 @@
+/*
+ * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * 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; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * 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 <ctype.h>
+#include <dirent.h>
+#include <fcntl.h>
+
+#include "libxfs.h"
+#include "config.h"
+
+/*
+ * 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.
+ *
+ * We only provide definitions for what we currently support parsing.
+ */
+
+enum data_subopts {
+	D_NOALIGN = 0,
+};
+
+enum inode_subopts {
+	I_ALIGN = 0,
+	I_PROJID32BIT,
+	I_SPINODES,
+};
+
+enum log_subopts {
+	L_LAZYSBCNTR = 0,
+};
+
+enum metadata_subopts {
+	M_CRC = 0,
+	M_FINOBT,
+	M_RMAPBT,
+	M_REFLINK,
+};
+
+enum naming_subopts {
+	N_FTYPE = 0,
+};
+
+enum rtdev_subopts {
+	R_NOALIGN = 0,
+};
+
+/* Just define the max options array size manually right now */
+#define MAX_SUBOPTS	4
+
+static int
+data_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum data_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case D_NOALIGN:
+		dft->sb_feat.nodalign = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+static int
+inode_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum inode_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case I_ALIGN:
+		dft->sb_feat.inode_align = value;
+		return 0;
+	case I_PROJID32BIT:
+		dft->sb_feat.projid32bit = value;
+		return 0;
+	case I_SPINODES:
+		dft->sb_feat.spinodes = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+static int
+log_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum log_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case L_LAZYSBCNTR:
+		dft->sb_feat.lazy_sb_counters = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+static int
+metadata_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum metadata_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case M_CRC:
+		dft->sb_feat.crcs_enabled = value;
+		if (dft->sb_feat.crcs_enabled)
+			dft->sb_feat.dirftype = true;
+		return 0;
+	case M_FINOBT:
+		dft->sb_feat.finobt = value;
+		return 0;
+	case M_RMAPBT:
+		dft->sb_feat.rmapbt = value;
+		return 0;
+	case M_REFLINK:
+		dft->sb_feat.reflink = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+static int
+naming_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum naming_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case N_FTYPE:
+		dft->sb_feat.dirftype = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+static int
+rtdev_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum rtdev_subopts	subopt = psubopt;
+
+	switch (subopt) {
+	case R_NOALIGN:
+		dft->sb_feat.nortalign = value;
+		return 0;
+	}
+	return EINVAL;
+}
+
+struct confopts {
+	const char		*name;
+	const char		*subopts[MAX_SUBOPTS];
+	int			(*parser)(struct mkfs_default_params *dft,
+					  int psubopt, uint64_t value);
+	bool			seen;
+} confopts_tab[] = {
+	{
+		.name = "data",
+		.subopts = {
+			[D_NOALIGN] = "noalign",
+		},
+		.parser = data_config_parser,
+	},
+	{
+		.name = "inode",
+		.subopts = {
+			[I_ALIGN] = "align",
+			[I_PROJID32BIT] = "projid32bit",
+			[I_SPINODES] = "sparse",
+		},
+		.parser = inode_config_parser,
+	},
+	{
+		.name = "log",
+		.subopts = {
+			[L_LAZYSBCNTR] = "lazy-count",
+		},
+		.parser = log_config_parser,
+	},
+	{
+		.name = "naming",
+		.subopts = {
+			[N_FTYPE] = "ftype",
+		},
+		.parser = naming_config_parser,
+	},
+	{
+		.name = "rtdev",
+		.subopts = {
+			[R_NOALIGN] = "noalign",
+		},
+		.parser = rtdev_config_parser,
+	},
+	{
+		.name = "metadata",
+		.subopts = {
+			[M_CRC] = "crc",
+			[M_FINOBT] = "finobt",
+			[M_RMAPBT] = "rmapbt",
+			[M_REFLINK] = "reflink",
+		},
+		.parser = metadata_config_parser,
+	},
+};
+
+static struct confopts *
+get_confopts(
+	const char	*section)
+{
+	unsigned int	i;
+	struct confopts	*opts;
+
+	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
+		opts = &confopts_tab[i];
+		if (!opts)
+			return NULL;
+		if (strcmp(opts->name, section) == 0) {
+			if (opts->seen) {
+				fprintf(stderr, _("Section '%s' respecified\n"),
+						section);
+				return NULL;
+			}
+			opts->seen = true;
+			return opts;
+		}
+	}
+	return NULL;
+}
+
+enum parse_line_type {
+	PARSE_COMMENT = 0,
+	PARSE_EMPTY,
+	PARSE_SECTION,
+	PARSE_TAG_VALUE,
+	PARSE_INVALID,
+	PARSE_EOF,
+};
+
+static bool
+isempty(
+	const char	*line,
+	ssize_t		linelen)
+{
+	ssize_t		i = 0;
+	char		p;
+
+	while (i != linelen) {
+		p = line[i];
+		i++;
+
+		/* tab or space */
+		if (isblank(p))
+			continue;
+		else
+			return false;
+	}
+
+	return true;
+}
+
+static bool
+iscomment(
+	const char	*line,
+	ssize_t		linelen)
+{
+	ssize_t		i = 0;
+	char		p;
+
+	while (i != linelen) {
+		p = line[i];
+		i++;
+
+		/* tab or space */
+		if (isblank(p))
+			continue;
+
+		if (p == '#')
+			return true;
+
+		return false;
+	}
+
+	return false;
+}
+
+static int
+parse_line_section(
+	const char	*line,
+	char		**tag)
+{
+	return sscanf(line, " [%m[^]]]", tag);
+}
+
+static int
+parse_line_tag_value(
+	const char	*line,
+	char		**tag,
+	uint64_t	*value)
+{
+	return sscanf(line, " %m[^ \t=]"
+		      " = "
+		      "%lu ",
+		      tag, value);
+}
+
+static enum parse_line_type
+parse_get_line_type(
+	const char	*line,
+	ssize_t		linelen,
+	char		**tag,
+	uint64_t	*value)
+{
+	int		ret;
+	uint64_t	u64_value;
+
+	if (isempty(line, linelen))
+		return PARSE_EMPTY;
+
+	if (iscomment(line, linelen))
+		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, &u64_value);
+	if (ret == 2) {
+		*value = u64_value;
+
+		return PARSE_TAG_VALUE;
+	}
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	return PARSE_INVALID;
+}
+
+static int
+parse_config_stream(
+	struct mkfs_default_params	*dft,
+	const char 			*config_file,
+	FILE				*fp)
+{
+	int				ret;
+	char				*line = NULL;
+	ssize_t				linelen;
+	size_t				len = 0, lineno = 0;
+	uint64_t			value;
+	enum parse_line_type		parse_type;
+	struct confopts			*confopt = NULL;
+	int				subopt;
+
+	while ((linelen = getline(&line, &len, fp)) != -1) {
+		char *ignore_value;
+		char *p, *tag = NULL;
+
+		lineno++;
+
+		/*
+		 * tag is allocated for us by scanf(), it must freed only on any
+		 * successful parse of a section or tag-value pair.
+		 */
+		parse_type = parse_get_line_type(line, linelen, &tag, &value);
+
+		switch (parse_type) {
+		case PARSE_EMPTY:
+		case PARSE_COMMENT:
+			/* Nothing tag to free for these */
+			continue;
+		case PARSE_EOF:
+			break;
+		case PARSE_INVALID:
+			ret = EINVAL;
+			fprintf(stderr, _("Invalid line %s:%zu : %s\n"),
+					  config_file, lineno, line);
+			goto out;
+		case PARSE_SECTION:
+			confopt = get_confopts(tag);
+			if (!confopt) {
+				ret = EINVAL;
+				fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"),
+						config_file, lineno, tag);
+				free(tag);
+				goto out;
+			}
+			if (!confopt->subopts) {
+				ret = EINVAL;
+				fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"),
+						config_file, lineno, tag);
+				free(tag);
+				goto out;
+			}
+			free(tag);
+			break;
+		case PARSE_TAG_VALUE:
+			if (!confopt) {
+				ret = EINVAL;
+				fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"),
+						config_file, lineno, line);
+				free(tag);
+				goto out;
+			}
+
+			/*
+			 * We re-use the line buffer allocated by getline(),
+			 * however line must be kept pointing to its original
+			 * value to free it later. A separate pointer is needed
+			 * as getsubopt() will otherwise muck with the value
+			 * passed.
+			 */
+			p = line;
+
+			/*
+			 * Trims white spaces. getsubopt() does not grok
+			 * white space, it would fail otherwise.
+			 */
+			snprintf(p, len, "%s=%lu", tag, value);
+
+			/* Not needed anymore */
+			free(tag);
+
+			/*
+			 * We only use getsubopt() to validate the possible
+			 * subopt, we already parsed the value and its already
+			 * in a more preferred data type.
+			 */
+			subopt = getsubopt(&p, (char **) confopt->subopts,
+					   &ignore_value);
+
+			ret = confopt->parser(dft, subopt, value);
+			if (ret) {
+				fprintf(stderr, _("Error parsine line %s:%zu : %s\n"),
+						config_file, lineno, line);
+				goto out;
+			}
+
+			break;
+		}
+	}
+out:
+	/* We must free even if getline() failed */
+	free(line);
+	return ret;
+}
+
+static const char *conf_paths[] = {
+	".",
+	MKFS_XFS_CONF_DIR,
+};
+
+/*
+ * If the file is not found -1 is returned and errno set. Otherwise
+ * the file descriptor is returned.
+ */
+int
+open_cli_config(
+	char			*cli_config_file,
+	char			**fpath)
+{
+	int			fd, len;
+	char			*final_path = NULL;
+	char			*relative_path= NULL;
+	unsigned int		i;
+
+	if (strlen(cli_config_file) > 2) {
+		if (cli_config_file[0] == '.' && cli_config_file[1] == '/')
+			final_path = cli_config_file;
+		else if (cli_config_file[0] == '.' && cli_config_file[1] == '.')
+			final_path = cli_config_file;
+		else if (cli_config_file[0] == '/')
+			final_path = cli_config_file;
+		else
+			relative_path = cli_config_file;
+	} else if (strlen(cli_config_file) == 1) {
+		if (cli_config_file[0] == '.' || cli_config_file[0] == '/') {
+			errno = EINVAL;
+			return -1;
+		} else
+			relative_path = cli_config_file;
+	}
+
+	if (final_path) {
+		fd = open(final_path, O_RDONLY);
+		if (fd >= 0)
+			memcpy(*fpath, final_path, strlen(final_path));
+		return fd;
+	}
+
+	/* We finally know the path is relative but just to be sure */
+	if (!relative_path) {
+		errno = ENXIO;
+		return -1;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(conf_paths); i++) {
+		memset(*fpath, 0, PATH_MAX);
+		/*
+		 * For current directory evaluation, skip concatenating the
+		 * ./ from the file passed. We only concatenate for the other
+		 * paths we look up on.
+		 */
+		if (i == 0)
+			memcpy(*fpath, relative_path, strlen(relative_path));
+		else {
+			len = snprintf(*fpath, PATH_MAX, "%s/%s", conf_paths[i],
+				       relative_path);
+			/* Indicates truncation */
+			if (len >= PATH_MAX) {
+				errno = ENAMETOOLONG;
+				return -1;
+			}
+		}
+		fd = open(*fpath, O_RDONLY);
+		if (fd < 0)
+			continue;
+		return fd;
+	}
+
+	errno = ENOENT;
+	return -1;
+}
+
+/*
+ * This is only called *iff* there is a configuration file which we know we
+ * *must* parse.
+ *
+ * If default_fd is set and is a valid file descriptor then the configuration
+ * file passed is the system default configuraiton file, and we already know
+ * it exists. If default_fd is not set we assume we've been passed a
+ * configuration file from the command line and must it must exist, otherwise
+ * we have to error out.
+ */
+int
+parse_defaults_file(
+	struct mkfs_default_params		*dft,
+	int					default_fd,
+	char					*config_file)
+{
+	char			*fpath;
+	int			fd;
+	FILE			*fp;
+	int			ret;
+	struct stat		sp;
+
+	if (strlen(config_file) > PATH_MAX)
+		return ENAMETOOLONG;
+
+	fpath = malloc(PATH_MAX);
+	if (!fpath)
+		return ENOMEM;
+	memset(fpath, 0, PATH_MAX);
+
+	if (default_fd < 0) {
+		fd = open_cli_config(config_file, &fpath);
+		if (fd < 0) {
+			free(fpath);
+			return errno;
+		}
+	} else {
+		fd = default_fd;
+		memcpy(fpath, config_file, strlen(config_file));
+	}
+
+	/*
+	 * At this point we know we have a valid file descriptor and have
+	 * figured out the path to the file used on fpath. Get the file stream
+	 * and do a bit of sanity checks before parsing the file.
+	 */
+
+	fp = fdopen(fd, "r");
+	if (!fp) {
+		ret = errno;
+		fprintf(stderr, _("Unable to open stream for config file: %s : %s\n"),
+				fpath, strerror(errno));
+		goto out_close_fd;
+	}
+
+	ret = fstat(fd, &sp);
+	if (ret) {
+		ret = errno;
+		fprintf(stderr, _("Could not fstat() config file: %s: %s\n"),
+			fpath, strerror(errno));
+		goto out;
+	}
+
+	if (S_ISDIR(sp.st_mode)) {
+		ret = EBADF;
+		fprintf(stderr, _("Config file is a directory: %s\n"), fpath);
+		goto out;
+	}
+
+	/* Anything beyond 1 MiB is kind of silly right now */
+	if (sp.st_size > 1 * 1024 * 1024) {
+		ret = E2BIG;
+		goto out;
+	}
+
+	ret = parse_config_stream(dft, config_file, fp);
+	if (ret)
+		goto out;
+
+	printf(_("config-file=%s\n"), fpath);
+
+out:
+	fclose(fp);
+out_close_fd:
+	close(fd);
+	free(fpath);
+	return ret;
+}
diff --git a/mkfs/config.h b/mkfs/config.h
index e5ea968e2d65..0f429d9b7fd7 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -19,6 +19,8 @@
 #ifndef _XFS_MKFS_CONFIG_H
 #define _XFS_MKFS_CONFIG_H
 
+#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d"
+
 struct fsxattr;
 
 /*
@@ -29,7 +31,7 @@ struct fsxattr;
  * source can overriding the later source:
  *
  * 	o built-in defaults
- * 	o configuration file (XXX)
+ * 	o configuration file
  * 	o command line
  *
  * These values are not used directly - they are inputs into the mkfs geometry
@@ -75,4 +77,10 @@ struct mkfs_default_params {
 	struct fsxattr		fsx;
 };
 
+int
+parse_defaults_file(
+	struct mkfs_default_params	*dft,
+	int				default_fd,
+	char				*config_file);
+
 #endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 217bb972538d..f759e6078ca1 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3777,6 +3777,11 @@ main(
 			.nortalign = false,
 		},
 	};
+	char                    *default_config = MKFS_XFS_CONF_DIR "/default";
+	char			*cli_config_file = NULL;
+	char			*config_file = NULL;
+	int			default_fd = -1;
+	int			ret;
 
 	platform_uuid_generate(&cli.uuid);
 	progname = basename(argv[0]);
@@ -3785,25 +3790,74 @@ 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
-	 * 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.
+	 * defaults. If the CLI specified a full path we use and require that.
+	 * If a relative path was provided on the CLI we search the allowed
+	 * search paths for the file. If no config file was specified on the
+	 * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if
+	 * present, however this file is optional.
 	 *
-	 * printf(_("Default configuration sourced from %s\n"), dft.source);
+	 * If a configuration file is found we use it to help overwrite default
+	 * values in the &dft structure. This way the new defaults will apply
+	 * before we parse the CLI, and the user will still be able to override
+	 * them through the CLI.
+	 */
+
+	/*
+	 * Pull config line options from command line
 	 */
+	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+		switch (c) {
+		case 'c':
+			if (cli_config_file) {
+				fprintf(stderr, _("respecification of configuration not allowed\n"));
+				exit(1);
+			}
+			cli_config_file = optarg;
+			dft.source = _("command line");
+			break;
+		default:
+			continue;
+		}
+	}
+
+	if (cli_config_file)
+		config_file = cli_config_file;
+	else {
+		default_fd = open(default_config, O_RDONLY);
+		if (default_fd >= 0) {
+			dft.source = _("system default configuration file");
+			config_file = default_config;
+		}
+	}
+
+	if (config_file) {
+		/* If default_fd is set it will be closed for us */
+		ret = parse_defaults_file(&dft, default_fd, config_file);
+		if (ret) {
+			fprintf(stderr, _("Could not open %s config file: %s : %s\n"),
+					dft.source, config_file,
+					strerror(ret));
+			exit(1);
+		}
+	}
 
-	/* copy new defaults into CLI parsing structure */
+	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(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
 	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	platform_getoptreset();
+
+	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;
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
-- 
2.16.3

Re: [PATCH v3 3/4] mkfs.xfs: add configuration file parsing support using our own parser

[Darrick J. Wong]

On Thu, May 24, 2018 at 08:19:42PM -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 search path for the configuration file will be:
> 
> 	1) $PWD/experimental
> 	2) /etc/mkfs.xfs.d/experimental
> 
> Absolute paths are supported, in which case they will be used directly
> and the mkfs.xfs.d directory is ignored.
> 
> 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. 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 <mcgrof@kernel.org>
> ---
>  configure.ac                           |   6 +-
>  include/builddefs.in                   |   2 +
>  man/man5/Makefile                      |   2 +
>  man/man5/mkfs.xfs.d.5.in               | 153 ++++++++
>  man/man8/Makefile                      |   2 +
>  man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} |  27 ++
>  mkfs/Makefile                          |   2 +-
>  mkfs/config.c                          | 644 +++++++++++++++++++++++++++++++++
>  mkfs/config.h                          |  10 +-
>  mkfs/xfs_mkfs.c                        |  76 +++-
>  10 files changed, 910 insertions(+), 14 deletions(-)
>  create mode 100644 man/man5/mkfs.xfs.d.5.in
>  rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%)
>  create mode 100644 mkfs/config.c
> 
> diff --git a/configure.ac b/configure.ac
> index 508eefede073..94c5bda725f2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -233,5 +233,9 @@ AC_CHECK_SIZEOF([char *])
>  AC_TYPE_UMODE_T
>  AC_MANUAL_FORMAT
>  
> -AC_CONFIG_FILES([include/builddefs])
> +AC_CONFIG_FILES([
> +	include/builddefs
> +	man/man5/mkfs.xfs.d.5
> +	man/man8/mkfs.xfs.8

Building the manpages as part of configure?  That's clever. :)

> +])
>  AC_OUTPUT
> 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/Makefile b/man/man5/Makefile
> index fe0aef6f016b..0b33122b064e 100644
> --- a/man/man5/Makefile
> +++ b/man/man5/Makefile
> @@ -19,3 +19,5 @@ install : default
>  	$(INSTALL) -m 755 -d $(MAN_DEST)
>  	$(INSTALL_MAN)
>  install-dev :
> +
> +LDIRT += mkfs.xfs.d.5
> diff --git a/man/man5/mkfs.xfs.d.5.in b/man/man5/mkfs.xfs.d.5.in
> new file mode 100644
> index 000000000000..c8dac58f5c9d
> --- /dev/null
> +++ b/man/man5/mkfs.xfs.d.5.in
> @@ -0,0 +1,153 @@
> +.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.
> +
> +The built-in mkfs defaults are decided by the XFS community. New features are
> +only enabled by default when the community consider it stable.  One can

"...consider them stable."

> +override these defaults on the
> +.B mkfs.xfs (8)
> +command line, but there are cases where it is desirable for the distro or the
> +system administrator to establish their own supported defaults in a uniform
> +manner, regardless of the version of
> +.B mkfs.xfs (8)
> +used. This may desirable for example on systems with old kernels

"This may be desirable..."

> +where the built-in default
> +.B mkfs.xfs (8)
> +parameters create a filesystem that is not supported by the old kernel.
> +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. 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 as a home to define different configuration files which can be used
> +to override the built-in default parameters by
> +.B mkfs.xfs (8).
> +If the
> +.B -c
> +parameter is not used, the default configuration file:
> +.IP
> +.I @sysconfdir@/mkfs.xfs.d/default
> +.PP
> +will be looked for first and if present will be used to override
> +.B mkfs.xfs (8)
> +built-in default parameters.
> +.PP
> +You can override the configuration file by specifying its name when using

"...override the default configuration file...", right?

> +.B mkfs.xfs (8)
> +by using the
> +.B -c
> +parameter.
> +.PP
> +If a relative path without the special character '.' is passed,

"If the path does not start with a '.', the current working directory is
searched for the file.  If the file is not found there, the mkfs.xfs.d(5)
directory is searched for the file." ?

> +the configuration file is first looked for in you current working directory;
> +if the configuration file is not present in your current working directory
> +the configuration file with the name given is looked in the
> +.B mkfs.xfs.d (5)
> +directory.
> +.PP
> +If
> +.B -c
> +is used with relative path with which has a leading '.' character, the given
> +path is used directly, so the configuration file will be relative to your
> +current directory.

"...relative to the current working directory."

(Trying to be consistent with 'current working directory'.)

> +.PP
> +If
> +.B -c
> +is used with an absolute path -- a path with a leading '/' character --
> +then the absolute path given is used for the configuration file.

"If the -c argument starts with a '/', it is considered an absolute
path, and opened." ?

(dunno about that...)

> +.PP
> +For example:
> +.IP
> +.B mkfs.xfs -c experimental -f /dev/sda1
> +.PP
> +Will make
> +.B mkfs.xfs (8)
> +look for the following configuration files and use the first one it finds:
> +.IP
> +.B $PWD/experimental
> +.br
> +.B @sysconfdir@/mkfs.xfs.d/experimental
> +.PP
> +If you used an absolute path, for example:
> +.IP
> +.B mkfs.xfs -c /tmp/experimental -f /dev/sda1
> +.PP
> +Then only the configuration file /tmp/experimental will be looked for and
> +used if present.
> +.PP
> +If you use the
> +.B -c
> +parameter the configuration file must be present and must parse correctly.
> +.PP
> +Parameters passed to the
> +.B mkfs.xfs (8)
> +command line always override any defaults set on the configuration file used.

"...set by the configuration file."

> +.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

We ought to have a comment in the same place that we define the builtin
defaults warning would-be patch authors to ensure that they update this
manpage.

> +.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/Makefile b/man/man8/Makefile
> index 36620da172ae..2a6548079997 100644
> --- a/man/man8/Makefile
> +++ b/man/man8/Makefile
> @@ -19,3 +19,5 @@ install : default
>  	$(INSTALL) -m 755 -d $(MAN_DEST)
>  	$(INSTALL_MAN)
>  install-dev :
> +
> +LDIRT += mkfs.xfs.8
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8.in
> similarity index 96%
> rename from man/man8/mkfs.xfs.8
> rename to man/man8/mkfs.xfs.8.in
> index 4b8c78c37806..81e2753bd2b5 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8.in
> @@ -83,6 +83,24 @@ 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 different default parameters which can be used when
> +calling
> +.B mkfs.xfs (8).
> +If present, and if
> +.B -c
> +is not used, the default configuration file @sysconfigdir@/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 and further details.
> +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 by using the
> +.B -c
> +parameter, further explained below and in
> +.B mkfs.xfs.d (5)
> +.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 +141,14 @@ 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. If a relative path is given the search
> +path for the configuration file is your current directory and then the
> +.B mkfs.xfs.d (5)
> +directory. If an absolute path is given it is used directly. For more details
> +refer to
> +.B mkfs.xfs.d (5)
> +.TP
>  .BI \-b " block_size_options"
>  This option specifies the fundamental block size of the filesystem.
>  The valid
> @@ -923,6 +949,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..c7815b3e106b 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 config.c
>  
>  LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
>  	$(LIBUUID)
> diff --git a/mkfs/config.c b/mkfs/config.c
> new file mode 100644
> index 000000000000..2d21728b7b62
> --- /dev/null
> +++ b/mkfs/config.c
> @@ -0,0 +1,644 @@
> +/*
> + * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org>
> + *
> + * 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; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * 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 <ctype.h>
> +#include <dirent.h>
> +#include <fcntl.h>
> +
> +#include "libxfs.h"
> +#include "config.h"
> +
> +/*
> + * 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.
> + *
> + * We only provide definitions for what we currently support parsing.
> + */
> +
> +enum data_subopts {
> +	D_NOALIGN = 0,
> +};
> +
> +enum inode_subopts {
> +	I_ALIGN = 0,
> +	I_PROJID32BIT,
> +	I_SPINODES,
> +};
> +
> +enum log_subopts {
> +	L_LAZYSBCNTR = 0,
> +};
> +
> +enum metadata_subopts {
> +	M_CRC = 0,
> +	M_FINOBT,
> +	M_RMAPBT,
> +	M_REFLINK,
> +};
> +
> +enum naming_subopts {
> +	N_FTYPE = 0,
> +};
> +
> +enum rtdev_subopts {
> +	R_NOALIGN = 0,
> +};
> +
> +/* Just define the max options array size manually right now */
> +#define MAX_SUBOPTS	4
> +
> +static int
> +data_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum data_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case D_NOALIGN:
> +		dft->sb_feat.nodalign = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +static int
> +inode_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum inode_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case I_ALIGN:
> +		dft->sb_feat.inode_align = value;
> +		return 0;
> +	case I_PROJID32BIT:
> +		dft->sb_feat.projid32bit = value;
> +		return 0;
> +	case I_SPINODES:
> +		dft->sb_feat.spinodes = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +static int
> +log_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum log_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case L_LAZYSBCNTR:
> +		dft->sb_feat.lazy_sb_counters = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +static int
> +metadata_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum metadata_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case M_CRC:
> +		dft->sb_feat.crcs_enabled = value;
> +		if (dft->sb_feat.crcs_enabled)
> +			dft->sb_feat.dirftype = true;
> +		return 0;
> +	case M_FINOBT:
> +		dft->sb_feat.finobt = value;
> +		return 0;
> +	case M_RMAPBT:
> +		dft->sb_feat.rmapbt = value;
> +		return 0;
> +	case M_REFLINK:
> +		dft->sb_feat.reflink = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +static int
> +naming_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum naming_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case N_FTYPE:
> +		dft->sb_feat.dirftype = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +static int
> +rtdev_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum rtdev_subopts	subopt = psubopt;
> +
> +	switch (subopt) {
> +	case R_NOALIGN:
> +		dft->sb_feat.nortalign = value;
> +		return 0;
> +	}
> +	return EINVAL;
> +}
> +
> +struct confopts {
> +	const char		*name;
> +	const char		*subopts[MAX_SUBOPTS];
> +	int			(*parser)(struct mkfs_default_params *dft,
> +					  int psubopt, uint64_t value);
> +	bool			seen;
> +} confopts_tab[] = {
> +	{
> +		.name = "data",
> +		.subopts = {
> +			[D_NOALIGN] = "noalign",
> +		},
> +		.parser = data_config_parser,
> +	},
> +	{
> +		.name = "inode",
> +		.subopts = {
> +			[I_ALIGN] = "align",
> +			[I_PROJID32BIT] = "projid32bit",
> +			[I_SPINODES] = "sparse",
> +		},
> +		.parser = inode_config_parser,
> +	},
> +	{
> +		.name = "log",
> +		.subopts = {
> +			[L_LAZYSBCNTR] = "lazy-count",
> +		},
> +		.parser = log_config_parser,
> +	},
> +	{
> +		.name = "naming",
> +		.subopts = {
> +			[N_FTYPE] = "ftype",
> +		},
> +		.parser = naming_config_parser,
> +	},
> +	{
> +		.name = "rtdev",
> +		.subopts = {
> +			[R_NOALIGN] = "noalign",
> +		},
> +		.parser = rtdev_config_parser,
> +	},
> +	{
> +		.name = "metadata",
> +		.subopts = {
> +			[M_CRC] = "crc",
> +			[M_FINOBT] = "finobt",
> +			[M_RMAPBT] = "rmapbt",
> +			[M_REFLINK] = "reflink",
> +		},
> +		.parser = metadata_config_parser,
> +	},
> +};
> +
> +static struct confopts *
> +get_confopts(
> +	const char	*section)
> +{
> +	unsigned int	i;
> +	struct confopts	*opts;
> +
> +	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
> +		opts = &confopts_tab[i];
> +		if (!opts)
> +			return NULL;
> +		if (strcmp(opts->name, section) == 0) {
> +			if (opts->seen) {
> +				fprintf(stderr, _("Section '%s' respecified\n"),
> +						section);

If I have two [data] sections, will this resuilt in:

# mkfs.xfs -c foo /dev/sda1
Section 'data' respecified
Invalid section on line foo:1 [data]

?

The section isn't invalid, it's just double-specified, so...

> +				return NULL;
> +			}
> +			opts->seen = true;
> +			return opts;
> +		}
> +	}

...I'd print the 'Invalid section' error message here.

> +	return NULL;
> +}
> +
> +enum parse_line_type {
> +	PARSE_COMMENT = 0,
> +	PARSE_EMPTY,
> +	PARSE_SECTION,
> +	PARSE_TAG_VALUE,
> +	PARSE_INVALID,
> +	PARSE_EOF,
> +};
> +
> +static bool
> +isempty(
> +	const char	*line,
> +	ssize_t		linelen)
> +{
> +	ssize_t		i = 0;
> +	char		p;
> +
> +	while (i != linelen) {
> +		p = line[i];
> +		i++;
> +
> +		/* tab or space */
> +		if (isblank(p))
> +			continue;
> +		else
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool
> +iscomment(
> +	const char	*line,
> +	ssize_t		linelen)
> +{
> +	ssize_t		i = 0;
> +	char		p;
> +
> +	while (i != linelen) {
> +		p = line[i];
> +		i++;
> +
> +		/* tab or space */
> +		if (isblank(p))
> +			continue;
> +
> +		if (p == '#')
> +			return true;
> +
> +		return false;
> +	}
> +
> +	return false;
> +}
> +
> +static int
> +parse_line_section(
> +	const char	*line,
> +	char		**tag)
> +{
> +	return sscanf(line, " [%m[^]]]", tag);
> +}
> +
> +static int
> +parse_line_tag_value(
> +	const char	*line,
> +	char		**tag,
> +	uint64_t	*value)
> +{
> +	return sscanf(line, " %m[^ \t=]"
> +		      " = "
> +		      "%lu ",
> +		      tag, value);
> +}
> +
> +static enum parse_line_type
> +parse_get_line_type(
> +	const char	*line,
> +	ssize_t		linelen,
> +	char		**tag,
> +	uint64_t	*value)
> +{
> +	int		ret;
> +	uint64_t	u64_value;
> +
> +	if (isempty(line, linelen))
> +		return PARSE_EMPTY;
> +
> +	if (iscomment(line, linelen))
> +		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, &u64_value);
> +	if (ret == 2) {
> +		*value = u64_value;
> +
> +		return PARSE_TAG_VALUE;
> +	}
> +
> +	if (ret == EOF)
> +		return PARSE_EOF;
> +
> +	return PARSE_INVALID;
> +}
> +
> +static int
> +parse_config_stream(
> +	struct mkfs_default_params	*dft,
> +	const char 			*config_file,
> +	FILE				*fp)
> +{
> +	int				ret;
> +	char				*line = NULL;
> +	ssize_t				linelen;
> +	size_t				len = 0, lineno = 0;
> +	uint64_t			value;
> +	enum parse_line_type		parse_type;
> +	struct confopts			*confopt = NULL;
> +	int				subopt;
> +
> +	while ((linelen = getline(&line, &len, fp)) != -1) {
> +		char *ignore_value;
> +		char *p, *tag = NULL;
> +
> +		lineno++;
> +
> +		/*
> +		 * tag is allocated for us by scanf(), it must freed only on any
> +		 * successful parse of a section or tag-value pair.
> +		 */
> +		parse_type = parse_get_line_type(line, linelen, &tag, &value);
> +
> +		switch (parse_type) {
> +		case PARSE_EMPTY:
> +		case PARSE_COMMENT:
> +			/* Nothing tag to free for these */
> +			continue;
> +		case PARSE_EOF:
> +			break;
> +		case PARSE_INVALID:
> +			ret = EINVAL;
> +			fprintf(stderr, _("Invalid line %s:%zu : %s\n"),
> +					  config_file, lineno, line);
> +			goto out;
> +		case PARSE_SECTION:
> +			confopt = get_confopts(tag);
> +			if (!confopt) {
> +				ret = EINVAL;
> +				fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"),
> +						config_file, lineno, tag);
> +				free(tag);
> +				goto out;
> +			}
> +			if (!confopt->subopts) {
> +				ret = EINVAL;
> +				fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"),
> +						config_file, lineno, tag);
> +				free(tag);
> +				goto out;
> +			}
> +			free(tag);
> +			break;
> +		case PARSE_TAG_VALUE:
> +			if (!confopt) {
> +				ret = EINVAL;
> +				fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"),
> +						config_file, lineno, line);
> +				free(tag);
> +				goto out;
> +			}
> +
> +			/*
> +			 * We re-use the line buffer allocated by getline(),
> +			 * however line must be kept pointing to its original
> +			 * value to free it later. A separate pointer is needed
> +			 * as getsubopt() will otherwise muck with the value
> +			 * passed.
> +			 */
> +			p = line;
> +
> +			/*
> +			 * Trims white spaces. getsubopt() does not grok
> +			 * white space, it would fail otherwise.
> +			 */
> +			snprintf(p, len, "%s=%lu", tag, value);
> +
> +			/* Not needed anymore */
> +			free(tag);
> +
> +			/*
> +			 * We only use getsubopt() to validate the possible
> +			 * subopt, we already parsed the value and its already
> +			 * in a more preferred data type.
> +			 */
> +			subopt = getsubopt(&p, (char **) confopt->subopts,
> +					   &ignore_value);
> +
> +			ret = confopt->parser(dft, subopt, value);
> +			if (ret) {
> +				fprintf(stderr, _("Error parsine line %s:%zu : %s\n"),
> +						config_file, lineno, line);
> +				goto out;
> +			}
> +
> +			break;
> +		}
> +	}
> +out:
> +	/* We must free even if getline() failed */
> +	free(line);
> +	return ret;
> +}
> +
> +static const char *conf_paths[] = {
> +	".",
> +	MKFS_XFS_CONF_DIR,
> +};
> +
> +/*
> + * If the file is not found -1 is returned and errno set. Otherwise
> + * the file descriptor is returned.
> + */
> +int
> +open_cli_config(
> +	char			*cli_config_file,
> +	char			**fpath)
> +{
> +	int			fd, len;
> +	char			*final_path = NULL;
> +	char			*relative_path= NULL;
> +	unsigned int		i;
> +
> +	if (strlen(cli_config_file) > 2) {
> +		if (cli_config_file[0] == '.' && cli_config_file[1] == '/')
> +			final_path = cli_config_file;
> +		else if (cli_config_file[0] == '.' && cli_config_file[1] == '.')
> +			final_path = cli_config_file;
> +		else if (cli_config_file[0] == '/')
> +			final_path = cli_config_file;
> +		else
> +			relative_path = cli_config_file;
> +	} else if (strlen(cli_config_file) == 1) {
> +		if (cli_config_file[0] == '.' || cli_config_file[0] == '/') {
> +			errno = EINVAL;
> +			return -1;
> +		} else
> +			relative_path = cli_config_file;
> +	}
> +
> +	if (final_path) {
> +		fd = open(final_path, O_RDONLY);
> +		if (fd >= 0)
> +			memcpy(*fpath, final_path, strlen(final_path));
> +		return fd;
> +	}
> +
> +	/* We finally know the path is relative but just to be sure */
> +	if (!relative_path) {
> +		errno = ENXIO;
> +		return -1;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(conf_paths); i++) {
> +		memset(*fpath, 0, PATH_MAX);
> +		/*
> +		 * For current directory evaluation, skip concatenating the
> +		 * ./ from the file passed. We only concatenate for the other
> +		 * paths we look up on.

If conf_paths[0] was "./" then you wouldn't have to special case this,
I think.

> +		 */
> +		if (i == 0)
> +			memcpy(*fpath, relative_path, strlen(relative_path));
> +		else {
> +			len = snprintf(*fpath, PATH_MAX, "%s/%s", conf_paths[i],
> +				       relative_path);
> +			/* Indicates truncation */
> +			if (len >= PATH_MAX) {
> +				errno = ENAMETOOLONG;
> +				return -1;
> +			}
> +		}
> +		fd = open(*fpath, O_RDONLY);
> +		if (fd < 0)
> +			continue;
> +		return fd;
> +	}
> +
> +	errno = ENOENT;
> +	return -1;
> +}
> +
> +/*
> + * This is only called *iff* there is a configuration file which we know we
> + * *must* parse.
> + *
> + * If default_fd is set and is a valid file descriptor then the configuration
> + * file passed is the system default configuraiton file, and we already know
> + * it exists. If default_fd is not set we assume we've been passed a
> + * configuration file from the command line and must it must exist, otherwise
> + * we have to error out.
> + */
> +int
> +parse_defaults_file(
> +	struct mkfs_default_params		*dft,
> +	int					default_fd,
> +	char					*config_file)
> +{
> +	char			*fpath;
> +	int			fd;
> +	FILE			*fp;
> +	int			ret;
> +	struct stat		sp;
> +
> +	if (strlen(config_file) > PATH_MAX)
> +		return ENAMETOOLONG;
> +
> +	fpath = malloc(PATH_MAX);
> +	if (!fpath)
> +		return ENOMEM;
> +	memset(fpath, 0, PATH_MAX);
> +
> +	if (default_fd < 0) {
> +		fd = open_cli_config(config_file, &fpath);
> +		if (fd < 0) {
> +			free(fpath);
> +			return errno;
> +		}
> +	} else {
> +		fd = default_fd;
> +		memcpy(fpath, config_file, strlen(config_file));
> +	}
> +
> +	/*
> +	 * At this point we know we have a valid file descriptor and have
> +	 * figured out the path to the file used on fpath. Get the file stream
> +	 * and do a bit of sanity checks before parsing the file.
> +	 */
> +
> +	fp = fdopen(fd, "r");
> +	if (!fp) {
> +		ret = errno;
> +		fprintf(stderr, _("Unable to open stream for config file: %s : %s\n"),
> +				fpath, strerror(errno));

perror(fpath); ?

> +		goto out_close_fd;
> +	}
> +
> +	ret = fstat(fd, &sp);
> +	if (ret) {
> +		ret = errno;
> +		fprintf(stderr, _("Could not fstat() config file: %s: %s\n"),
> +			fpath, strerror(errno));
> +		goto out;
> +	}
> +
> +	if (S_ISDIR(sp.st_mode)) {
> +		ret = EBADF;
> +		fprintf(stderr, _("Config file is a directory: %s\n"), fpath);
> +		goto out;
> +	}
> +
> +	/* Anything beyond 1 MiB is kind of silly right now */
> +	if (sp.st_size > 1 * 1024 * 1024) {
> +		ret = E2BIG;
> +		goto out;
> +	}
> +
> +	ret = parse_config_stream(dft, config_file, fp);
> +	if (ret)
> +		goto out;
> +
> +	printf(_("config-file=%s\n"), fpath);
> +
> +out:
> +	fclose(fp);
> +out_close_fd:
> +	close(fd);
> +	free(fpath);
> +	return ret;
> +}
> diff --git a/mkfs/config.h b/mkfs/config.h
> index e5ea968e2d65..0f429d9b7fd7 100644
> --- a/mkfs/config.h
> +++ b/mkfs/config.h
> @@ -19,6 +19,8 @@
>  #ifndef _XFS_MKFS_CONFIG_H
>  #define _XFS_MKFS_CONFIG_H
>  
> +#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/mkfs.xfs.d"
> +
>  struct fsxattr;
>  
>  /*
> @@ -29,7 +31,7 @@ struct fsxattr;
>   * source can overriding the later source:
>   *
>   * 	o built-in defaults
> - * 	o configuration file (XXX)
> + * 	o configuration file
>   * 	o command line
>   *
>   * These values are not used directly - they are inputs into the mkfs geometry
> @@ -75,4 +77,10 @@ struct mkfs_default_params {
>  	struct fsxattr		fsx;
>  };
>  
> +int
> +parse_defaults_file(
> +	struct mkfs_default_params	*dft,
> +	int				default_fd,
> +	char				*config_file);
> +
>  #endif /* _XFS_MKFS_CONFIG_H */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 217bb972538d..f759e6078ca1 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3777,6 +3777,11 @@ main(
>  			.nortalign = false,
>  		},
>  	};
> +	char                    *default_config = MKFS_XFS_CONF_DIR "/default";
> +	char			*cli_config_file = NULL;
> +	char			*config_file = NULL;
> +	int			default_fd = -1;
> +	int			ret;
>  
>  	platform_uuid_generate(&cli.uuid);
>  	progname = basename(argv[0]);
> @@ -3785,25 +3790,74 @@ 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
> -	 * 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.
> +	 * defaults. If the CLI specified a full path we use and require that.
> +	 * If a relative path was provided on the CLI we search the allowed
> +	 * search paths for the file. If no config file was specified on the
> +	 * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if
> +	 * present, however this file is optional.
>  	 *
> -	 * printf(_("Default configuration sourced from %s\n"), dft.source);
> +	 * If a configuration file is found we use it to help overwrite default
> +	 * values in the &dft structure. This way the new defaults will apply
> +	 * before we parse the CLI, and the user will still be able to override
> +	 * them through the CLI.
> +	 */
> +
> +	/*
> +	 * Pull config line options from command line
>  	 */
> +	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> +		switch (c) {
> +		case 'c':
> +			if (cli_config_file) {
> +				fprintf(stderr, _("respecification of configuration not allowed\n"));
> +				exit(1);
> +			}
> +			cli_config_file = optarg;
> +			dft.source = _("command line");
> +			break;
> +		default:
> +			continue;
> +		}
> +	}
> +
> +	if (cli_config_file)
> +		config_file = cli_config_file;
> +	else {
> +		default_fd = open(default_config, O_RDONLY);
> +		if (default_fd >= 0) {
> +			dft.source = _("system default configuration file");
> +			config_file = default_config;
> +		}
> +	}
> +
> +	if (config_file) {
> +		/* If default_fd is set it will be closed for us */
> +		ret = parse_defaults_file(&dft, default_fd, config_file);
> +		if (ret) {
> +			fprintf(stderr, _("Could not open %s config file: %s : %s\n"),
> +					dft.source, config_file,
> +					strerror(ret));
> +			exit(1);
> +		}
> +	}
>  
> -	/* copy new defaults into CLI parsing structure */
> +	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(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
>  	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
>  
> -	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
> +	platform_getoptreset();
> +
> +	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;

Looks good otherwise.

--D

>  		case 'C':
>  		case 'f':
>  			force_overwrite = 1;
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] mkfs.xfs: add configuration file parsing support using our own parser

[Luis R. Rodriguez]

I've applied all the recommendations for the man page updates you
provided. More below on the code questions.

On Thu, May 24, 2018 at 09:01:43PM -0700, Darrick J. Wong wrote:
> On Thu, May 24, 2018 at 08:19:42PM -0700, Luis R. Rodriguez wrote:
> > +static struct confopts *
> > +get_confopts(
> > +	const char	*section)
> > +{
> > +	unsigned int	i;
> > +	struct confopts	*opts;
> > +
> > +	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
> > +		opts = &confopts_tab[i];
> > +		if (!opts)
> > +			return NULL;
> > +		if (strcmp(opts->name, section) == 0) {
> > +			if (opts->seen) {
> > +				fprintf(stderr, _("Section '%s' respecified\n"),
> > +						section);
> 
> If I have two [data] sections, will this resuilt in:
> 
> # mkfs.xfs -c foo /dev/sda1
> Section 'data' respecified
> Invalid section on line foo:1 [data]
> 
> ?

Yeah.

> The section isn't invalid, it's just double-specified, so...

I've fixed this, now we get:

ergon:~ # mkfs.xfs -f /dev/loop5 -c bar 
Section 'metadata' respecified
Error parsing command line config file: bar : Invalid argument

> > +				return NULL;
> > +			}
> > +			opts->seen = true;
> > +			return opts;
> > +		}
> > +	}
> 
> ...I'd print the 'Invalid section' error message here.

Or move the respecification error message check to the switch
statement handlig the section. I went with the later.

> > +static const char *conf_paths[] = {
> > +	".",
> > +	MKFS_XFS_CONF_DIR,
> > +};
> > +
> > +/*
> > + * If the file is not found -1 is returned and errno set. Otherwise
> > + * the file descriptor is returned.
> > + */
> > +int
> > +open_cli_config(
> > +	char			*cli_config_file,
> > +	char			**fpath)
> > +{
> > +	int			fd, len;
> > +	char			*final_path = NULL;
> > +	char			*relative_path= NULL;
> > +	unsigned int		i;
> > +
> > +	if (strlen(cli_config_file) > 2) {
> > +		if (cli_config_file[0] == '.' && cli_config_file[1] == '/')
> > +			final_path = cli_config_file;
> > +		else if (cli_config_file[0] == '.' && cli_config_file[1] == '.')
> > +			final_path = cli_config_file;
> > +		else if (cli_config_file[0] == '/')
> > +			final_path = cli_config_file;
> > +		else
> > +			relative_path = cli_config_file;
> > +	} else if (strlen(cli_config_file) == 1) {
> > +		if (cli_config_file[0] == '.' || cli_config_file[0] == '/') {
> > +			errno = EINVAL;
> > +			return -1;
> > +		} else
> > +			relative_path = cli_config_file;
> > +	}
> > +
> > +	if (final_path) {
> > +		fd = open(final_path, O_RDONLY);
> > +		if (fd >= 0)
> > +			memcpy(*fpath, final_path, strlen(final_path));
> > +		return fd;
> > +	}
> > +
> > +	/* We finally know the path is relative but just to be sure */
> > +	if (!relative_path) {
> > +		errno = ENXIO;
> > +		return -1;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(conf_paths); i++) {
> > +		memset(*fpath, 0, PATH_MAX);
> > +		/*
> > +		 * For current directory evaluation, skip concatenating the
> > +		 * ./ from the file passed. We only concatenate for the other
> > +		 * paths we look up on.
> 
> If conf_paths[0] was "./" then you wouldn't have to special case this,
> I think.

No, I had tried it, it looks odd still, if the user specified -c foo,
the output should show ./foo, not just foo.

> > +	fp = fdopen(fd, "r");
> > +	if (!fp) {
> > +		ret = errno;
> > +		fprintf(stderr, _("Unable to open stream for config file: %s : %s\n"),
> > +				fpath, strerror(errno));
> 
> perror(fpath); ?

Sure.

> Looks good otherwise.

Groovy. I'll wait for others to comment otherwise I can spin up 4th
iteration after the weekend.

  Luis

Re: [PATCH v3 3/4] mkfs.xfs: add configuration file parsing support using our own parser

[Darrick J. Wong]

On Fri, May 25, 2018 at 04:33:07PM -0700, Luis R. Rodriguez wrote:
> I've applied all the recommendations for the man page updates you
> provided. More below on the code questions.
> 
> On Thu, May 24, 2018 at 09:01:43PM -0700, Darrick J. Wong wrote:
> > On Thu, May 24, 2018 at 08:19:42PM -0700, Luis R. Rodriguez wrote:
> > > +static struct confopts *
> > > +get_confopts(
> > > +	const char	*section)
> > > +{
> > > +	unsigned int	i;
> > > +	struct confopts	*opts;
> > > +
> > > +	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
> > > +		opts = &confopts_tab[i];
> > > +		if (!opts)
> > > +			return NULL;
> > > +		if (strcmp(opts->name, section) == 0) {
> > > +			if (opts->seen) {
> > > +				fprintf(stderr, _("Section '%s' respecified\n"),
> > > +						section);
> > 
> > If I have two [data] sections, will this resuilt in:
> > 
> > # mkfs.xfs -c foo /dev/sda1
> > Section 'data' respecified
> > Invalid section on line foo:1 [data]
> > 
> > ?
> 
> Yeah.
> 
> > The section isn't invalid, it's just double-specified, so...
> 
> I've fixed this, now we get:
> 
> ergon:~ # mkfs.xfs -f /dev/loop5 -c bar 
> Section 'metadata' respecified
> Error parsing command line config file: bar : Invalid argument

Ok, thanks!

> > > +				return NULL;
> > > +			}
> > > +			opts->seen = true;
> > > +			return opts;
> > > +		}
> > > +	}
> > 
> > ...I'd print the 'Invalid section' error message here.
> 
> Or move the respecification error message check to the switch
> statement handlig the section. I went with the later.
> 
> > > +static const char *conf_paths[] = {
> > > +	".",
> > > +	MKFS_XFS_CONF_DIR,
> > > +};
> > > +
> > > +/*
> > > + * If the file is not found -1 is returned and errno set. Otherwise
> > > + * the file descriptor is returned.
> > > + */
> > > +int
> > > +open_cli_config(
> > > +	char			*cli_config_file,
> > > +	char			**fpath)
> > > +{
> > > +	int			fd, len;
> > > +	char			*final_path = NULL;
> > > +	char			*relative_path= NULL;
> > > +	unsigned int		i;
> > > +
> > > +	if (strlen(cli_config_file) > 2) {
> > > +		if (cli_config_file[0] == '.' && cli_config_file[1] == '/')
> > > +			final_path = cli_config_file;
> > > +		else if (cli_config_file[0] == '.' && cli_config_file[1] == '.')
> > > +			final_path = cli_config_file;
> > > +		else if (cli_config_file[0] == '/')
> > > +			final_path = cli_config_file;
> > > +		else
> > > +			relative_path = cli_config_file;
> > > +	} else if (strlen(cli_config_file) == 1) {
> > > +		if (cli_config_file[0] == '.' || cli_config_file[0] == '/') {
> > > +			errno = EINVAL;
> > > +			return -1;
> > > +		} else
> > > +			relative_path = cli_config_file;
> > > +	}
> > > +
> > > +	if (final_path) {
> > > +		fd = open(final_path, O_RDONLY);
> > > +		if (fd >= 0)
> > > +			memcpy(*fpath, final_path, strlen(final_path));
> > > +		return fd;
> > > +	}
> > > +
> > > +	/* We finally know the path is relative but just to be sure */
> > > +	if (!relative_path) {
> > > +		errno = ENXIO;
> > > +		return -1;
> > > +	}
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(conf_paths); i++) {
> > > +		memset(*fpath, 0, PATH_MAX);
> > > +		/*
> > > +		 * For current directory evaluation, skip concatenating the
> > > +		 * ./ from the file passed. We only concatenate for the other
> > > +		 * paths we look up on.
> > 
> > If conf_paths[0] was "./" then you wouldn't have to special case this,
> > I think.
> 
> No, I had tried it, it looks odd still, if the user specified -c foo,
> the output should show ./foo, not just foo.

Ah.  Good point.

> > > +	fp = fdopen(fd, "r");
> > > +	if (!fp) {
> > > +		ret = errno;
> > > +		fprintf(stderr, _("Unable to open stream for config file: %s : %s\n"),
> > > +				fpath, strerror(errno));
> > 
> > perror(fpath); ?
> 
> Sure.
> 
> > Looks good otherwise.
> 
> Groovy. I'll wait for others to comment otherwise I can spin up 4th
> iteration after the weekend.

<nod>

--D

>   Luis

[PATCH v3 4/4] debian/rules: use the new sysconfdir configuration setting

[Luis R. Rodriguez]

Configuration file support requires setting --sysconfdir=/etc/ if your
distribution expects mkfs.xfs.d directory to be in /etc.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 debian/rules | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/debian/rules b/debian/rules
index cb4fa22c1584..599d7998052e 100755
--- a/debian/rules
+++ b/debian/rules
@@ -20,9 +20,9 @@ stdenv = @GZIP=-q; export GZIP;
 
 options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \
 	  INSTALL_USER=root INSTALL_GROUP=root \
-	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
+	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto --sysconfdir=/etc/" ;
 diopts  = $(options) \
-	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
+	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto --sysconfdir=/etc/" ;
 checkdir = test -f debian/rules
 
 build: build-arch build-indep
-- 
2.16.3

Re: [PATCH v3 4/4] debian/rules: use the new sysconfdir configuration setting

[Darrick J. Wong]

On Thu, May 24, 2018 at 08:19:43PM -0700, Luis R. Rodriguez wrote:
> Configuration file support requires setting --sysconfdir=/etc/ if your
> distribution expects mkfs.xfs.d directory to be in /etc.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Looks good, thanks for putting this series together!
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  debian/rules | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/rules b/debian/rules
> index cb4fa22c1584..599d7998052e 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -20,9 +20,9 @@ stdenv = @GZIP=-q; export GZIP;
>  
>  options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \
>  	  INSTALL_USER=root INSTALL_GROUP=root \
> -	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
> +	  LOCAL_CONFIGURE_OPTIONS="--enable-readline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto --sysconfdir=/etc/" ;
>  diopts  = $(options) \
> -	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
> +	  export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto --sysconfdir=/etc/" ;
>  checkdir = test -f debian/rules
>  
>  build: build-arch build-indep
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html