[PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Darrick J. Wong]

Hi all,

This series refactors the build system so that we can generate a mkfs
configuration file template from the defaults hardwired into the source
code.  We can ship the template as /etc/xfs/mkfs/default.template and we
can also put it in the mkfs.xfs manpage.  The series is based off of
Eric's mkfs-config branch as of 12-June-2018.

--D

[PATCH 1/5] mkfs: move build-time defaults to a separate file

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the built-in defaults to a separate file so that we can generate
config files and manpages from them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/Makefile   |    2 +-
 mkfs/config.h   |    2 ++
 mkfs/defaults.c |   27 +++++++++++++++++++++++++++
 mkfs/xfs_mkfs.c |   19 +------------------
 4 files changed, 31 insertions(+), 19 deletions(-)
 create mode 100644 mkfs/defaults.c


diff --git a/mkfs/Makefile b/mkfs/Makefile
index c7815b3e..5af8a6cc 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 config.c
+CFILES = proto.c xfs_mkfs.c config.c defaults.c
 
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
diff --git a/mkfs/config.h b/mkfs/config.h
index db22adec..544f8a6d 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -119,4 +119,6 @@ parse_defaults_file(
 	struct mkfs_default_params	*dft,
 	const char			*config_file);
 
+extern const struct sb_feat_args default_features;
+
 #endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/defaults.c b/mkfs/defaults.c
new file mode 100644
index 00000000..6e35a4f9
--- /dev/null
+++ b/mkfs/defaults.c
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ */
+#include "libxfs.h"
+#include "config.h"
+
+/* build time feature defaults */
+const struct sb_feat_args default_features = {
+	.log_version = 2,
+	.attr_version = 2,
+	.dir_version = 2,
+	.inode_align = true,
+	.nci = false,
+	.lazy_sb_counters = true,
+	.projid32bit = true,
+	.crcs_enabled = true,
+	.dirftype = true,
+	.finobt = true,
+	.spinodes = true,
+	.rmapbt = false,
+	.reflink = false,
+	.parent_pointers = false,
+	.nodalign = false,
+	.nortalign = false,
+};
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d32c2c8e..40a5da12 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3725,24 +3725,7 @@ main(
 		.type = DEFAULTS_BUILTIN,
 		.sectorsize = XFS_MIN_SECTORSIZE,
 		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
-		.sb_feat = {
-			.log_version = 2,
-			.attr_version = 2,
-			.dir_version = 2,
-			.inode_align = true,
-			.nci = false,
-			.lazy_sb_counters = true,
-			.projid32bit = true,
-			.crcs_enabled = true,
-			.dirftype = true,
-			.finobt = true,
-			.spinodes = true,
-			.rmapbt = false,
-			.reflink = false,
-			.parent_pointers = false,
-			.nodalign = false,
-			.nortalign = false,
-		},
+		.sb_feat = default_features,
 	};
 	char			*cli_config_file = NULL;
 	char			*config_file = NULL;

Re: [PATCH 1/5] mkfs: move build-time defaults to a separate file

[Eric Sandeen]

On 6/13/18 2:31 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the built-in defaults to a separate file so that we can generate
> config files and manpages from them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  mkfs/Makefile   |    2 +-
>  mkfs/config.h   |    2 ++
>  mkfs/defaults.c |   27 +++++++++++++++++++++++++++
>  mkfs/xfs_mkfs.c |   19 +------------------
>  4 files changed, 31 insertions(+), 19 deletions(-)
>  create mode 100644 mkfs/defaults.c
> 
> 
> diff --git a/mkfs/Makefile b/mkfs/Makefile
> index c7815b3e..5af8a6cc 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 config.c
> +CFILES = proto.c xfs_mkfs.c config.c defaults.c
>  
>  LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
>  	$(LIBUUID)
> diff --git a/mkfs/config.h b/mkfs/config.h
> index db22adec..544f8a6d 100644
> --- a/mkfs/config.h
> +++ b/mkfs/config.h
> @@ -119,4 +119,6 @@ parse_defaults_file(
>  	struct mkfs_default_params	*dft,
>  	const char			*config_file);
>  
> +extern const struct sb_feat_args default_features;
> +
>  #endif /* _XFS_MKFS_CONFIG_H */
> diff --git a/mkfs/defaults.c b/mkfs/defaults.c
> new file mode 100644
> index 00000000..6e35a4f9
> --- /dev/null
> +++ b/mkfs/defaults.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + */
> +#include "libxfs.h"
> +#include "config.h"
> +
> +/* build time feature defaults */
> +const struct sb_feat_args default_features = {
> +	.log_version = 2,
> +	.attr_version = 2,
> +	.dir_version = 2,
> +	.inode_align = true,
> +	.nci = false,
> +	.lazy_sb_counters = true,
> +	.projid32bit = true,
> +	.crcs_enabled = true,
> +	.dirftype = true,
> +	.finobt = true,
> +	.spinodes = true,
> +	.rmapbt = false,
> +	.reflink = false,
> +	.parent_pointers = false,
> +	.nodalign = false,
> +	.nortalign = false,
> +};
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d32c2c8e..40a5da12 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3725,24 +3725,7 @@ main(
>  		.type = DEFAULTS_BUILTIN,
>  		.sectorsize = XFS_MIN_SECTORSIZE,
>  		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
> -		.sb_feat = {
> -			.log_version = 2,
> -			.attr_version = 2,
> -			.dir_version = 2,
> -			.inode_align = true,
> -			.nci = false,
> -			.lazy_sb_counters = true,
> -			.projid32bit = true,
> -			.crcs_enabled = true,
> -			.dirftype = true,
> -			.finobt = true,
> -			.spinodes = true,
> -			.rmapbt = false,
> -			.reflink = false,
> -			.parent_pointers = false,
> -			.nodalign = false,
> -			.nortalign = false,
> -		},
> +		.sb_feat = default_features,

I'm not a huge fan of making a new "defaults.c" which only contains /some/ of
the defaults (the boolean features, more or less, except also too some versions,
but also too not sizes ...) it's a weird split that's not intuitive at all.

If having the separate file is helpful wouldn't it make more sense to move all of
mkfs_default_params into it?

-Eric

[PATCH 2/5] mkfs: move config file enums to config.h

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the config file enums to config.h and make the names distinct from
the cli ones.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/config.c |   90 ++++++++++++++++++---------------------------------------
 mkfs/config.h |   32 ++++++++++++++++++++
 2 files changed, 61 insertions(+), 61 deletions(-)


diff --git a/mkfs/config.c b/mkfs/config.c
index 835adc45..23447aa4 100644
--- a/mkfs/config.c
+++ b/mkfs/config.c
@@ -32,38 +32,6 @@
  * 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	5
-
 static int
 config_check_bool(
 	uint64_t	value)
@@ -84,13 +52,13 @@ data_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum data_subopts		subopt = psubopt;
+	enum cfg_data_subopts		subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case D_NOALIGN:
+	case CFG_D_NOALIGN:
 		dft->sb_feat.nodalign = value;
 		return 0;
 	}
@@ -103,19 +71,19 @@ inode_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum inode_subopts		subopt = psubopt;
+	enum cfg_inode_subopts		subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case I_ALIGN:
+	case CFG_I_ALIGN:
 		dft->sb_feat.inode_align = value;
 		return 0;
-	case I_PROJID32BIT:
+	case CFG_I_PROJID32BIT:
 		dft->sb_feat.projid32bit = value;
 		return 0;
-	case I_SPINODES:
+	case CFG_I_SPINODES:
 		dft->sb_feat.spinodes = value;
 		return 0;
 	}
@@ -128,13 +96,13 @@ log_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum log_subopts		subopt = psubopt;
+	enum cfg_log_subopts		subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case L_LAZYSBCNTR:
+	case CFG_L_LAZYSBCNTR:
 		dft->sb_feat.lazy_sb_counters = value;
 		return 0;
 	}
@@ -147,24 +115,24 @@ metadata_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum metadata_subopts		subopt = psubopt;
+	enum cfg_metadata_subopts	subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case M_CRC:
+	case CFG_M_CRC:
 		dft->sb_feat.crcs_enabled = value;
 		if (dft->sb_feat.crcs_enabled)
 			dft->sb_feat.dirftype = true;
 		return 0;
-	case M_FINOBT:
+	case CFG_M_FINOBT:
 		dft->sb_feat.finobt = value;
 		return 0;
-	case M_RMAPBT:
+	case CFG_M_RMAPBT:
 		dft->sb_feat.rmapbt = value;
 		return 0;
-	case M_REFLINK:
+	case CFG_M_REFLINK:
 		dft->sb_feat.reflink = value;
 		return 0;
 	}
@@ -177,13 +145,13 @@ naming_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum naming_subopts		subopt = psubopt;
+	enum cfg_naming_subopts		subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case N_FTYPE:
+	case CFG_N_FTYPE:
 		dft->sb_feat.dirftype = value;
 		return 0;
 	}
@@ -196,13 +164,13 @@ rtdev_config_parser(
 	int				psubopt,
 	uint64_t			value)
 {
-	enum rtdev_subopts		subopt = psubopt;
+	enum cfg_rtdev_subopts		subopt = psubopt;
 
 	if (config_check_bool(value) != 0)
 		return -1;
 
 	switch (subopt) {
-	case R_NOALIGN:
+	case CFG_R_NOALIGN:
 		dft->sb_feat.nortalign = value;
 		return 0;
 	}
@@ -211,7 +179,7 @@ rtdev_config_parser(
 
 struct confopts {
 	const char	*name;
-	const char	*subopts[MAX_SUBOPTS];
+	const char	*subopts[CFG_MAX_SUBOPTS];
 	int		(*parser)(struct mkfs_default_params *dft,
 				  int psubopt, uint64_t value);
 	bool		seen;
@@ -219,7 +187,7 @@ struct confopts {
 	{
 		.name = "data",
 		.subopts = {
-			[D_NOALIGN] = "noalign",
+			[CFG_D_NOALIGN] = "noalign",
 			NULL
 		},
 		.parser = data_config_parser,
@@ -227,9 +195,9 @@ struct confopts {
 	{
 		.name = "inode",
 		.subopts = {
-			[I_ALIGN] = "align",
-			[I_PROJID32BIT] = "projid32bit",
-			[I_SPINODES] = "sparse",
+			[CFG_I_ALIGN] = "align",
+			[CFG_I_PROJID32BIT] = "projid32bit",
+			[CFG_I_SPINODES] = "sparse",
 			NULL
 		},
 		.parser = inode_config_parser,
@@ -237,7 +205,7 @@ struct confopts {
 	{
 		.name = "log",
 		.subopts = {
-			[L_LAZYSBCNTR] = "lazy-count",
+			[CFG_L_LAZYSBCNTR] = "lazy-count",
 			NULL
 		},
 		.parser = log_config_parser,
@@ -245,7 +213,7 @@ struct confopts {
 	{
 		.name = "naming",
 		.subopts = {
-			[N_FTYPE] = "ftype",
+			[CFG_N_FTYPE] = "ftype",
 			NULL
 		},
 		.parser = naming_config_parser,
@@ -253,7 +221,7 @@ struct confopts {
 	{
 		.name = "rtdev",
 		.subopts = {
-			[R_NOALIGN] = "noalign",
+			[CFG_R_NOALIGN] = "noalign",
 			NULL
 		},
 		.parser = rtdev_config_parser,
@@ -261,10 +229,10 @@ struct confopts {
 	{
 		.name = "metadata",
 		.subopts = {
-			[M_CRC] = "crc",
-			[M_FINOBT] = "finobt",
-			[M_RMAPBT] = "rmapbt",
-			[M_REFLINK] = "reflink",
+			[CFG_M_CRC] = "crc",
+			[CFG_M_FINOBT] = "finobt",
+			[CFG_M_RMAPBT] = "rmapbt",
+			[CFG_M_REFLINK] = "reflink",
 			NULL
 		},
 		.parser = metadata_config_parser,
diff --git a/mkfs/config.h b/mkfs/config.h
index 544f8a6d..f00849e9 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -119,6 +119,38 @@ parse_defaults_file(
 	struct mkfs_default_params	*dft,
 	const char			*config_file);
 
+enum cfg_data_subopts {
+	CFG_D_NOALIGN = 0,
+};
+
+enum cfg_inode_subopts {
+	CFG_I_ALIGN = 0,
+	CFG_I_PROJID32BIT,
+	CFG_I_SPINODES,
+};
+
+enum cfg_log_subopts {
+	CFG_L_LAZYSBCNTR = 0,
+};
+
+enum cfg_metadata_subopts {
+	CFG_M_CRC = 0,
+	CFG_M_FINOBT,
+	CFG_M_RMAPBT,
+	CFG_M_REFLINK,
+};
+
+enum cfg_naming_subopts {
+	CFG_N_FTYPE = 0,
+};
+
+enum cfg_rtdev_subopts {
+	CFG_R_NOALIGN = 0,
+};
+
+/* Just define the max options array size manually right now */
+#define CFG_MAX_SUBOPTS	5
+
 extern const struct sb_feat_args default_features;
 
 #endif /* _XFS_MKFS_CONFIG_H */

Re: [PATCH 2/5] mkfs: move config file enums to config.h

[Eric Sandeen]

On 6/13/18 2:31 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the config file enums to config.h and make the names distinct from
> the cli ones.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Ok, happy with fixing up the namespace issues / removing the duplicated
names... still feeling uncomfortable with what is feeling like a degree of code
duplication overall ... keeping this all in sync is a bit of a pain and ...
well, I don't have a better plan yet, so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  mkfs/config.c |   90 ++++++++++++++++++---------------------------------------
>  mkfs/config.h |   32 ++++++++++++++++++++
>  2 files changed, 61 insertions(+), 61 deletions(-)
> 
> 
> diff --git a/mkfs/config.c b/mkfs/config.c
> index 835adc45..23447aa4 100644
> --- a/mkfs/config.c
> +++ b/mkfs/config.c
> @@ -32,38 +32,6 @@
>   * 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	5
> -
>  static int
>  config_check_bool(
>  	uint64_t	value)
> @@ -84,13 +52,13 @@ data_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum data_subopts		subopt = psubopt;
> +	enum cfg_data_subopts		subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case D_NOALIGN:
> +	case CFG_D_NOALIGN:
>  		dft->sb_feat.nodalign = value;
>  		return 0;
>  	}
> @@ -103,19 +71,19 @@ inode_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum inode_subopts		subopt = psubopt;
> +	enum cfg_inode_subopts		subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case I_ALIGN:
> +	case CFG_I_ALIGN:
>  		dft->sb_feat.inode_align = value;
>  		return 0;
> -	case I_PROJID32BIT:
> +	case CFG_I_PROJID32BIT:
>  		dft->sb_feat.projid32bit = value;
>  		return 0;
> -	case I_SPINODES:
> +	case CFG_I_SPINODES:
>  		dft->sb_feat.spinodes = value;
>  		return 0;
>  	}
> @@ -128,13 +96,13 @@ log_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum log_subopts		subopt = psubopt;
> +	enum cfg_log_subopts		subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case L_LAZYSBCNTR:
> +	case CFG_L_LAZYSBCNTR:
>  		dft->sb_feat.lazy_sb_counters = value;
>  		return 0;
>  	}
> @@ -147,24 +115,24 @@ metadata_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum metadata_subopts		subopt = psubopt;
> +	enum cfg_metadata_subopts	subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case M_CRC:
> +	case CFG_M_CRC:
>  		dft->sb_feat.crcs_enabled = value;
>  		if (dft->sb_feat.crcs_enabled)
>  			dft->sb_feat.dirftype = true;
>  		return 0;
> -	case M_FINOBT:
> +	case CFG_M_FINOBT:
>  		dft->sb_feat.finobt = value;
>  		return 0;
> -	case M_RMAPBT:
> +	case CFG_M_RMAPBT:
>  		dft->sb_feat.rmapbt = value;
>  		return 0;
> -	case M_REFLINK:
> +	case CFG_M_REFLINK:
>  		dft->sb_feat.reflink = value;
>  		return 0;
>  	}
> @@ -177,13 +145,13 @@ naming_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum naming_subopts		subopt = psubopt;
> +	enum cfg_naming_subopts		subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case N_FTYPE:
> +	case CFG_N_FTYPE:
>  		dft->sb_feat.dirftype = value;
>  		return 0;
>  	}
> @@ -196,13 +164,13 @@ rtdev_config_parser(
>  	int				psubopt,
>  	uint64_t			value)
>  {
> -	enum rtdev_subopts		subopt = psubopt;
> +	enum cfg_rtdev_subopts		subopt = psubopt;
>  
>  	if (config_check_bool(value) != 0)
>  		return -1;
>  
>  	switch (subopt) {
> -	case R_NOALIGN:
> +	case CFG_R_NOALIGN:
>  		dft->sb_feat.nortalign = value;
>  		return 0;
>  	}
> @@ -211,7 +179,7 @@ rtdev_config_parser(
>  
>  struct confopts {
>  	const char	*name;
> -	const char	*subopts[MAX_SUBOPTS];
> +	const char	*subopts[CFG_MAX_SUBOPTS];
>  	int		(*parser)(struct mkfs_default_params *dft,
>  				  int psubopt, uint64_t value);
>  	bool		seen;
> @@ -219,7 +187,7 @@ struct confopts {
>  	{
>  		.name = "data",
>  		.subopts = {
> -			[D_NOALIGN] = "noalign",
> +			[CFG_D_NOALIGN] = "noalign",
>  			NULL
>  		},
>  		.parser = data_config_parser,
> @@ -227,9 +195,9 @@ struct confopts {
>  	{
>  		.name = "inode",
>  		.subopts = {
> -			[I_ALIGN] = "align",
> -			[I_PROJID32BIT] = "projid32bit",
> -			[I_SPINODES] = "sparse",
> +			[CFG_I_ALIGN] = "align",
> +			[CFG_I_PROJID32BIT] = "projid32bit",
> +			[CFG_I_SPINODES] = "sparse",
>  			NULL
>  		},
>  		.parser = inode_config_parser,
> @@ -237,7 +205,7 @@ struct confopts {
>  	{
>  		.name = "log",
>  		.subopts = {
> -			[L_LAZYSBCNTR] = "lazy-count",
> +			[CFG_L_LAZYSBCNTR] = "lazy-count",
>  			NULL
>  		},
>  		.parser = log_config_parser,
> @@ -245,7 +213,7 @@ struct confopts {
>  	{
>  		.name = "naming",
>  		.subopts = {
> -			[N_FTYPE] = "ftype",
> +			[CFG_N_FTYPE] = "ftype",
>  			NULL
>  		},
>  		.parser = naming_config_parser,
> @@ -253,7 +221,7 @@ struct confopts {
>  	{
>  		.name = "rtdev",
>  		.subopts = {
> -			[R_NOALIGN] = "noalign",
> +			[CFG_R_NOALIGN] = "noalign",
>  			NULL
>  		},
>  		.parser = rtdev_config_parser,
> @@ -261,10 +229,10 @@ struct confopts {
>  	{
>  		.name = "metadata",
>  		.subopts = {
> -			[M_CRC] = "crc",
> -			[M_FINOBT] = "finobt",
> -			[M_RMAPBT] = "rmapbt",
> -			[M_REFLINK] = "reflink",
> +			[CFG_M_CRC] = "crc",
> +			[CFG_M_FINOBT] = "finobt",
> +			[CFG_M_RMAPBT] = "rmapbt",
> +			[CFG_M_REFLINK] = "reflink",
>  			NULL
>  		},
>  		.parser = metadata_config_parser,
> diff --git a/mkfs/config.h b/mkfs/config.h
> index 544f8a6d..f00849e9 100644
> --- a/mkfs/config.h
> +++ b/mkfs/config.h
> @@ -119,6 +119,38 @@ parse_defaults_file(
>  	struct mkfs_default_params	*dft,
>  	const char			*config_file);
>  
> +enum cfg_data_subopts {
> +	CFG_D_NOALIGN = 0,
> +};
> +
> +enum cfg_inode_subopts {
> +	CFG_I_ALIGN = 0,
> +	CFG_I_PROJID32BIT,
> +	CFG_I_SPINODES,
> +};
> +
> +enum cfg_log_subopts {
> +	CFG_L_LAZYSBCNTR = 0,
> +};
> +
> +enum cfg_metadata_subopts {
> +	CFG_M_CRC = 0,
> +	CFG_M_FINOBT,
> +	CFG_M_RMAPBT,
> +	CFG_M_REFLINK,
> +};
> +
> +enum cfg_naming_subopts {
> +	CFG_N_FTYPE = 0,
> +};
> +
> +enum cfg_rtdev_subopts {
> +	CFG_R_NOALIGN = 0,
> +};
> +
> +/* Just define the max options array size manually right now */
> +#define CFG_MAX_SUBOPTS	5
> +
>  extern const struct sb_feat_args default_features;
>  
>  #endif /* _XFS_MKFS_CONFIG_H */
> 
> --
> 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 2/5] mkfs: move config file enums to config.h

[Luis R. Rodriguez]

On Wed, Jun 13, 2018 at 09:51:34PM -0500, Eric Sandeen wrote:
> 
> 
> On 6/13/18 2:31 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Move the config file enums to config.h and make the names distinct from
> > the cli ones.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Ok, happy with fixing up the namespace issues / removing the duplicated
> names... 

Chinner had wanted to keep things separate purposely as he indicated we may
diverge eventually. So inclusion of the enums on even mkfs/xfs_mkfs.c seems
not needed but that will depend on your later patches and what the goal is.

  Luis

[PATCH 3/5] mkfs: hoist mkfs configfile dir string generation to build system

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Move the generation of MKFS_XFS_CONF_DIR to the build system; in the
next few patches we're going to use that to install a (disabled) default
config file.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/builddefs.in |    4 +++-
 mkfs/Makefile        |    5 +++++
 mkfs/config.h        |    3 ---
 3 files changed, 8 insertions(+), 4 deletions(-)


diff --git a/include/builddefs.in b/include/builddefs.in
index e1ee9f7b..06cb71a6 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -65,6 +65,9 @@ PKG_MAN_DIR	= @mandir@
 PKG_ETC_DIR	= @sysconfdir@
 PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
 PKG_LOCALE_DIR	= @datadir@/locale
+PKG_CFG_DIR	= $(PKG_ETC_DIR)/xfs
+PKG_MKFS_CFG_DIR = $(PKG_CFG_DIR)/mkfs
+PKG_MKFS_DEFAULT_CFGFILE = default
 
 CC		= @cc@
 BUILD_CC	= @BUILD_CC@
@@ -197,7 +200,6 @@ 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/mkfs/Makefile b/mkfs/Makefile
index 5af8a6cc..102f5214 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -10,6 +10,9 @@ LTCOMMAND = mkfs.xfs
 HFILES =
 CFILES = proto.c xfs_mkfs.c config.c defaults.c
 
+CFGFILE_CFLAGS = -DMKFS_XFS_CONF_DIR=\"$(PKG_MKFS_CFG_DIR)\" \
+		 -DMKFS_XFS_DEFAULT_CONFIG=\"$(PKG_MKFS_DEFAULT_CFGFILE)\"
+LCFLAGS += $(CFGFILE_CFLAGS)
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
 LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
@@ -22,6 +25,8 @@ include $(BUILDRULES)
 install: default
 	$(INSTALL) -m 755 -d $(PKG_ROOT_SBIN_DIR)
 	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_ROOT_SBIN_DIR)
+	$(INSTALL) -m 755 -d $(PKG_CFG_DIR)
+	$(INSTALL) -m 755 -d $(PKG_MKFS_CFG_DIR)
 install-dev:
 
 -include .dep
diff --git a/mkfs/config.h b/mkfs/config.h
index f00849e9..69f44405 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -19,9 +19,6 @@
 #ifndef _XFS_MKFS_CONFIG_H
 #define _XFS_MKFS_CONFIG_H
 
-#define MKFS_XFS_CONF_DIR      	ROOT_SYSCONFDIR "/xfs/mkfs"
-#define MKFS_XFS_DEFAULT_CONFIG	"default"
-
 struct fsxattr;
 
 /*

Re: [PATCH 3/5] mkfs: hoist mkfs configfile dir string generation to build system

[Eric Sandeen]

On 6/13/18 2:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Move the generation of MKFS_XFS_CONF_DIR to the build system; in the
> next few patches we're going to use that to install a (disabled) default
> config file.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  include/builddefs.in |    4 +++-
>  mkfs/Makefile        |    5 +++++
>  mkfs/config.h        |    3 ---
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/include/builddefs.in b/include/builddefs.in
> index e1ee9f7b..06cb71a6 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -65,6 +65,9 @@ PKG_MAN_DIR	= @mandir@
>  PKG_ETC_DIR	= @sysconfdir@
>  PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
>  PKG_LOCALE_DIR	= @datadir@/locale
> +PKG_CFG_DIR	= $(PKG_ETC_DIR)/xfs
> +PKG_MKFS_CFG_DIR = $(PKG_CFG_DIR)/mkfs
> +PKG_MKFS_DEFAULT_CFGFILE = default
>  
>  CC		= @cc@
>  BUILD_CC	= @BUILD_CC@
> @@ -197,7 +200,6 @@ 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/mkfs/Makefile b/mkfs/Makefile
> index 5af8a6cc..102f5214 100644
> --- a/mkfs/Makefile
> +++ b/mkfs/Makefile
> @@ -10,6 +10,9 @@ LTCOMMAND = mkfs.xfs
>  HFILES =
>  CFILES = proto.c xfs_mkfs.c config.c defaults.c
>  
> +CFGFILE_CFLAGS = -DMKFS_XFS_CONF_DIR=\"$(PKG_MKFS_CFG_DIR)\" \
> +		 -DMKFS_XFS_DEFAULT_CONFIG=\"$(PKG_MKFS_DEFAULT_CFGFILE)\"
> +LCFLAGS += $(CFGFILE_CFLAGS)
>  LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
>  	$(LIBUUID)
>  LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
> @@ -22,6 +25,8 @@ include $(BUILDRULES)
>  install: default
>  	$(INSTALL) -m 755 -d $(PKG_ROOT_SBIN_DIR)
>  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_ROOT_SBIN_DIR)
> +	$(INSTALL) -m 755 -d $(PKG_CFG_DIR)
> +	$(INSTALL) -m 755 -d $(PKG_MKFS_CFG_DIR)
>  install-dev:
>  
>  -include .dep
> diff --git a/mkfs/config.h b/mkfs/config.h
> index f00849e9..69f44405 100644
> --- a/mkfs/config.h
> +++ b/mkfs/config.h
> @@ -19,9 +19,6 @@
>  #ifndef _XFS_MKFS_CONFIG_H
>  #define _XFS_MKFS_CONFIG_H
>  
> -#define MKFS_XFS_CONF_DIR      	ROOT_SYSCONFDIR "/xfs/mkfs"
> -#define MKFS_XFS_DEFAULT_CONFIG	"default"
> -
>  struct fsxattr;
>  
>  /*
> 
> --
> 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 4/5] mkfs: emit config file from builtin defaults

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Create a build-time helper program to generate a config file from the
built-in defaults.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/Makefile   |   14 ++++-
 mkfs/mkconfig.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 mkfs/mkconfig.c


diff --git a/mkfs/Makefile b/mkfs/Makefile
index 102f5214..44ddbd30 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -12,13 +12,24 @@ CFILES = proto.c xfs_mkfs.c config.c defaults.c
 
 CFGFILE_CFLAGS = -DMKFS_XFS_CONF_DIR=\"$(PKG_MKFS_CFG_DIR)\" \
 		 -DMKFS_XFS_DEFAULT_CONFIG=\"$(PKG_MKFS_DEFAULT_CFGFILE)\"
+CFGFILE_TEMPLATE = $(PKG_MKFS_DEFAULT_CFGFILE).template
+
 LCFLAGS += $(CFGFILE_CFLAGS)
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
 LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
 LLDFLAGS = -static-libtool-libs
+LDIRT = $(CFGFILE_TEMPLATE)
+
+default: depend $(CFGFILE_TEMPLATE) $(LTCOMMAND)
+
+mkconfig: mkconfig.c defaults.c
+	@echo "    [CC]     mkconfig"
+	$(Q) $(BUILD_CC) $(BUILD_CFLAGS) $(CFGFILE_CFLAGS) -o $@ $^
 
-default: depend $(LTCOMMAND)
+$(CFGFILE_TEMPLATE): mkconfig
+	@echo "    [GENERATE] $@"
+	$(Q) ./mkconfig > $@
 
 include $(BUILDRULES)
 
@@ -27,6 +38,7 @@ install: default
 	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_ROOT_SBIN_DIR)
 	$(INSTALL) -m 755 -d $(PKG_CFG_DIR)
 	$(INSTALL) -m 755 -d $(PKG_MKFS_CFG_DIR)
+	$(INSTALL) -m 644 $(CFGFILE_TEMPLATE) $(PKG_MKFS_CFG_DIR)
 install-dev:
 
 -include .dep
diff --git a/mkfs/mkconfig.c b/mkfs/mkconfig.c
new file mode 100644
index 00000000..3826b0bf
--- /dev/null
+++ b/mkfs/mkconfig.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ */
+#include "libxfs.h"
+#include "config.h"
+
+enum cfgfile_var_type {
+	FV_BOOL = 0
+};
+
+struct subopt_map {
+	const char		*suboptname;
+	const void		*ptr;
+	enum cfgfile_var_type	type;
+};
+
+/* Map all the config file options to their default_features equivalents. */
+struct confopts {
+	const char		*name;
+	struct subopt_map	subopts[CFG_MAX_SUBOPTS];
+} confopts_tab[] = {
+	{
+		.name = "data",
+		.subopts = {
+			[CFG_D_NOALIGN] = {
+				.suboptname	= "noalign",
+				.ptr		= &default_features.nodalign,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+	{
+		.name = "inode",
+		.subopts = {
+			[CFG_I_ALIGN] = {
+				.suboptname	= "align",
+				.ptr		= &default_features.inode_align,
+				.type		= FV_BOOL,
+			},
+			[CFG_I_PROJID32BIT] = {
+				.suboptname	= "projid32bit",
+				.ptr		= &default_features.projid32bit,
+				.type		= FV_BOOL,
+			},
+			[CFG_I_SPINODES] = {
+				.suboptname	= "sparse",
+				.ptr		= &default_features.spinodes,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+	{
+		.name = "log",
+		.subopts = {
+			[CFG_L_LAZYSBCNTR] = {
+				.suboptname	= "lazy-count",
+				.ptr		= &default_features.lazy_sb_counters,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+	{
+		.name = "metadata",
+		.subopts = {
+			[CFG_M_CRC] = {
+				.suboptname	= "crc",
+				.ptr		= &default_features.crcs_enabled,
+				.type		= FV_BOOL,
+			},
+			[CFG_M_FINOBT] = {
+				.suboptname	= "finobt",
+				.ptr		= &default_features.finobt,
+				.type		= FV_BOOL,
+			},
+			[CFG_M_RMAPBT] = {
+				.suboptname	= "rmapbt",
+				.ptr		= &default_features.rmapbt,
+				.type		= FV_BOOL,
+			},
+			[CFG_M_REFLINK] = {
+				.suboptname	= "reflink",
+				.ptr		= &default_features.reflink,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+	{
+		.name = "naming",
+		.subopts = {
+			[CFG_N_FTYPE] = {
+				.suboptname	= "ftype",
+				.ptr		= &default_features.dirftype,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+	{
+		.name = "rtdev",
+		.subopts = {
+			[CFG_R_NOALIGN] = {
+				.suboptname	= "noalign",
+				.ptr		= &default_features.nortalign,
+				.type		= FV_BOOL,
+			},
+			{NULL}
+		},
+	},
+};
+
+/* Spit out a config file template. */
+int
+main(
+	int			argc,
+	char			*argv[])
+{
+	struct confopts		*opts = confopts_tab;
+	struct subopt_map	*submap;
+	int			c;
+	unsigned int		i, j;
+
+	while ((c = getopt(argc, argv, "")) != EOF) {
+		switch (c) {
+		case '?':
+			fprintf(stderr, "Unknown option %c.\n", optopt);
+			return 1;
+		}
+	}
+	if (argc - optind != 0) {
+		fprintf(stderr, "extra arguments\n");
+		return 1;
+	}
+
+	printf("# mkfs.xfs configuration file to collect settings.\n");
+	printf("# See the mkfs.xfs(8) manpage for details.\n");
+	printf("# Copy this file to %s/%s to override the built-in defaults.\n",
+			MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
+
+	for (i = 0; i < ARRAY_SIZE(confopts_tab); i++, opts++) {
+		if (i > 0)
+			printf("\n");
+		printf("[%s]\n", opts->name);
+		submap = opts->subopts;
+		for (j = 0;
+		     j < ARRAY_SIZE(opts->subopts) && submap->suboptname;
+		     j++, submap++) {
+			printf("%s = ", submap->suboptname);
+			switch (submap->type) {
+			case FV_BOOL:
+				printf("%d\n", *((bool *)submap->ptr));
+				break;
+			}
+		}
+	}
+
+	return 0;
+}

Re: [PATCH 4/5] mkfs: emit config file from builtin defaults

[Eric Sandeen]

On 6/13/18 2:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Create a build-time helper program to generate a config file from the
> built-in defaults.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Ok.  This is good, but I think we are going ot have to find a way to collapse
all these structures down, now we've replicated things 3 times (i.e. a new
boolean/feature will now hit xfs_mkfs.c, config.c, and mkconfig.c ...)

But this is a step forward for consistency w.r.t. manpage & template file,
so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  mkfs/Makefile   |   14 ++++-
>  mkfs/mkconfig.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 mkfs/mkconfig.c
> 
> 
> diff --git a/mkfs/Makefile b/mkfs/Makefile
> index 102f5214..44ddbd30 100644
> --- a/mkfs/Makefile
> +++ b/mkfs/Makefile
> @@ -12,13 +12,24 @@ CFILES = proto.c xfs_mkfs.c config.c defaults.c
>  
>  CFGFILE_CFLAGS = -DMKFS_XFS_CONF_DIR=\"$(PKG_MKFS_CFG_DIR)\" \
>  		 -DMKFS_XFS_DEFAULT_CONFIG=\"$(PKG_MKFS_DEFAULT_CFGFILE)\"
> +CFGFILE_TEMPLATE = $(PKG_MKFS_DEFAULT_CFGFILE).template
> +
>  LCFLAGS += $(CFGFILE_CFLAGS)
>  LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
>  	$(LIBUUID)
>  LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
>  LLDFLAGS = -static-libtool-libs
> +LDIRT = $(CFGFILE_TEMPLATE)
> +
> +default: depend $(CFGFILE_TEMPLATE) $(LTCOMMAND)
> +
> +mkconfig: mkconfig.c defaults.c
> +	@echo "    [CC]     mkconfig"
> +	$(Q) $(BUILD_CC) $(BUILD_CFLAGS) $(CFGFILE_CFLAGS) -o $@ $^
>  
> -default: depend $(LTCOMMAND)
> +$(CFGFILE_TEMPLATE): mkconfig
> +	@echo "    [GENERATE] $@"
> +	$(Q) ./mkconfig > $@
>  
>  include $(BUILDRULES)
>  
> @@ -27,6 +38,7 @@ install: default
>  	$(LTINSTALL) -m 755 $(LTCOMMAND) $(PKG_ROOT_SBIN_DIR)
>  	$(INSTALL) -m 755 -d $(PKG_CFG_DIR)
>  	$(INSTALL) -m 755 -d $(PKG_MKFS_CFG_DIR)
> +	$(INSTALL) -m 644 $(CFGFILE_TEMPLATE) $(PKG_MKFS_CFG_DIR)
>  install-dev:
>  
>  -include .dep
> diff --git a/mkfs/mkconfig.c b/mkfs/mkconfig.c
> new file mode 100644
> index 00000000..3826b0bf
> --- /dev/null
> +++ b/mkfs/mkconfig.c
> @@ -0,0 +1,163 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Oracle.  All Rights Reserved.
> + * Author: Darrick J. Wong <darrick.wong@oracle.com>
> + */
> +#include "libxfs.h"
> +#include "config.h"
> +
> +enum cfgfile_var_type {
> +	FV_BOOL = 0
> +};
> +
> +struct subopt_map {
> +	const char		*suboptname;
> +	const void		*ptr;
> +	enum cfgfile_var_type	type;
> +};
> +
> +/* Map all the config file options to their default_features equivalents. */
> +struct confopts {
> +	const char		*name;
> +	struct subopt_map	subopts[CFG_MAX_SUBOPTS];
> +} confopts_tab[] = {
> +	{
> +		.name = "data",
> +		.subopts = {
> +			[CFG_D_NOALIGN] = {
> +				.suboptname	= "noalign",
> +				.ptr		= &default_features.nodalign,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +	{
> +		.name = "inode",
> +		.subopts = {
> +			[CFG_I_ALIGN] = {
> +				.suboptname	= "align",
> +				.ptr		= &default_features.inode_align,
> +				.type		= FV_BOOL,
> +			},
> +			[CFG_I_PROJID32BIT] = {
> +				.suboptname	= "projid32bit",
> +				.ptr		= &default_features.projid32bit,
> +				.type		= FV_BOOL,
> +			},
> +			[CFG_I_SPINODES] = {
> +				.suboptname	= "sparse",
> +				.ptr		= &default_features.spinodes,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +	{
> +		.name = "log",
> +		.subopts = {
> +			[CFG_L_LAZYSBCNTR] = {
> +				.suboptname	= "lazy-count",
> +				.ptr		= &default_features.lazy_sb_counters,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +	{
> +		.name = "metadata",
> +		.subopts = {
> +			[CFG_M_CRC] = {
> +				.suboptname	= "crc",
> +				.ptr		= &default_features.crcs_enabled,
> +				.type		= FV_BOOL,
> +			},
> +			[CFG_M_FINOBT] = {
> +				.suboptname	= "finobt",
> +				.ptr		= &default_features.finobt,
> +				.type		= FV_BOOL,
> +			},
> +			[CFG_M_RMAPBT] = {
> +				.suboptname	= "rmapbt",
> +				.ptr		= &default_features.rmapbt,
> +				.type		= FV_BOOL,
> +			},
> +			[CFG_M_REFLINK] = {
> +				.suboptname	= "reflink",
> +				.ptr		= &default_features.reflink,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +	{
> +		.name = "naming",
> +		.subopts = {
> +			[CFG_N_FTYPE] = {
> +				.suboptname	= "ftype",
> +				.ptr		= &default_features.dirftype,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +	{
> +		.name = "rtdev",
> +		.subopts = {
> +			[CFG_R_NOALIGN] = {
> +				.suboptname	= "noalign",
> +				.ptr		= &default_features.nortalign,
> +				.type		= FV_BOOL,
> +			},
> +			{NULL}
> +		},
> +	},
> +};
> +
> +/* Spit out a config file template. */
> +int
> +main(
> +	int			argc,
> +	char			*argv[])
> +{
> +	struct confopts		*opts = confopts_tab;
> +	struct subopt_map	*submap;
> +	int			c;
> +	unsigned int		i, j;
> +
> +	while ((c = getopt(argc, argv, "")) != EOF) {
> +		switch (c) {
> +		case '?':
> +			fprintf(stderr, "Unknown option %c.\n", optopt);
> +			return 1;
> +		}
> +	}
> +	if (argc - optind != 0) {
> +		fprintf(stderr, "extra arguments\n");
> +		return 1;
> +	}
> +
> +	printf("# mkfs.xfs configuration file to collect settings.\n");
> +	printf("# See the mkfs.xfs(8) manpage for details.\n");
> +	printf("# Copy this file to %s/%s to override the built-in defaults.\n",
> +			MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
> +
> +	for (i = 0; i < ARRAY_SIZE(confopts_tab); i++, opts++) {
> +		if (i > 0)
> +			printf("\n");
> +		printf("[%s]\n", opts->name);
> +		submap = opts->subopts;
> +		for (j = 0;
> +		     j < ARRAY_SIZE(opts->subopts) && submap->suboptname;
> +		     j++, submap++) {
> +			printf("%s = ", submap->suboptname);
> +			switch (submap->type) {
> +			case FV_BOOL:
> +				printf("%d\n", *((bool *)submap->ptr));
> +				break;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> 
> --
> 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 5/5] mkfs: generate mkfs config file in man page

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Generate the default mkfs config file in the man page.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 Makefile               |    1 +
 man/man8/Makefile      |   13 ++++++++++---
 man/man8/mkfs.xfs.8.in |   35 +----------------------------------
 mkfs/mkconfig.c        |   22 ++++++++++++++++------
 4 files changed, 28 insertions(+), 43 deletions(-)


diff --git a/Makefile b/Makefile
index 7ddfa316..947eaaf5 100644
--- a/Makefile
+++ b/Makefile
@@ -92,6 +92,7 @@ copy: libxlog
 mkfs: libxcmd
 spaceman: libxcmd
 scrub: libhandle libxcmd
+man: mkfs
 
 ifeq ($(HAVE_BUILDDEFS), yes)
 include $(BUILDRULES)
diff --git a/man/man8/Makefile b/man/man8/Makefile
index 08e5e0d7..67a67cd2 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -13,13 +13,20 @@ LSRCFILES	= $(MAN_PAGES)
 
 default : $(MAN_PAGES)
 
-LDIRT		= mkfs.xfs.8
+LDIRT		= mkfs.xfs.8 cfg.8
 
 include $(BUILDRULES)
 
-mkfs.xfs.8: mkfs.xfs.8.in
+cfg.8: ../../mkfs/mkconfig
+	@echo "    [GENERATE] $@"
+	$(Q) ../../mkfs/mkconfig -m > $@
+
+mkfs.xfs.8: mkfs.xfs.8.in cfg.8
 	@echo "    [SED]    $@"
-	$(Q)$(SED) -e "s|@sysconfdir@|$(PKG_ETC_DIR)|g" < $< > $@
+	$(Q) $(SED) -e "s|@sysconfdir@|$(PKG_ETC_DIR)|g" \
+		   -e "/@default_mkfs_cfg@/r cfg.8" \
+		   -e "s/@default_mkfs_cfg@//g" \
+		   < $< > $@
 
 install : default
 	$(INSTALL) -m 755 -d $(MAN_DEST)
diff --git a/man/man8/mkfs.xfs.8.in b/man/man8/mkfs.xfs.8.in
index cf4bdf82..4a557910 100644
--- a/man/man8/mkfs.xfs.8.in
+++ b/man/man8/mkfs.xfs.8.in
@@ -970,40 +970,7 @@ Currently all default parameters can only be either enabled or disabled,
 with a value of 1 to enable or 0 to disable.
 See below for a list of all supported configuration parameters and their
 current built-in default settings.
-.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
+@default_mkfs_cfg@
 .PP
 .SH SEE ALSO
 .BR xfs (5),
diff --git a/mkfs/mkconfig.c b/mkfs/mkconfig.c
index 3826b0bf..ef00f6d9 100644
--- a/mkfs/mkconfig.c
+++ b/mkfs/mkconfig.c
@@ -124,9 +124,13 @@ main(
 	struct subopt_map	*submap;
 	int			c;
 	unsigned int		i, j;
+	bool			manpage = false;
 
-	while ((c = getopt(argc, argv, "")) != EOF) {
+	while ((c = getopt(argc, argv, "m")) != EOF) {
 		switch (c) {
+		case 'm':
+			manpage = true;
+			break;
 		case '?':
 			fprintf(stderr, "Unknown option %c.\n", optopt);
 			return 1;
@@ -137,19 +141,25 @@ main(
 		return 1;
 	}
 
-	printf("# mkfs.xfs configuration file to collect settings.\n");
-	printf("# See the mkfs.xfs(8) manpage for details.\n");
-	printf("# Copy this file to %s/%s to override the built-in defaults.\n",
-			MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
+	if (!manpage) {
+		printf("# mkfs.xfs configuration file to collect settings.\n");
+		printf("# See the mkfs.xfs(8) manpage for details.\n");
+		printf("# Copy this file to %s/%s to override the built-in defaults.\n",
+				MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
+	}
 
 	for (i = 0; i < ARRAY_SIZE(confopts_tab); i++, opts++) {
-		if (i > 0)
+		if (manpage)
+			printf(".PP\n.B ");
+		else if (i > 0)
 			printf("\n");
 		printf("[%s]\n", opts->name);
 		submap = opts->subopts;
 		for (j = 0;
 		     j < ARRAY_SIZE(opts->subopts) && submap->suboptname;
 		     j++, submap++) {
+			if (manpage)
+				printf(".br\n.B ");
 			printf("%s = ", submap->suboptname);
 			switch (submap->type) {
 			case FV_BOOL:

Re: [PATCH 5/5] mkfs: generate mkfs config file in man page

[Eric Sandeen]

On 6/13/18 2:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Generate the default mkfs config file in the man page.

Cool.  Minor things below.

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  Makefile               |    1 +
>  man/man8/Makefile      |   13 ++++++++++---
>  man/man8/mkfs.xfs.8.in |   35 +----------------------------------
>  mkfs/mkconfig.c        |   22 ++++++++++++++++------
>  4 files changed, 28 insertions(+), 43 deletions(-)
> 
> 
> diff --git a/Makefile b/Makefile
> index 7ddfa316..947eaaf5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -92,6 +92,7 @@ copy: libxlog
>  mkfs: libxcmd
>  spaceman: libxcmd
>  scrub: libhandle libxcmd
> +man: mkfs
>  
>  ifeq ($(HAVE_BUILDDEFS), yes)
>  include $(BUILDRULES)
> diff --git a/man/man8/Makefile b/man/man8/Makefile
> index 08e5e0d7..67a67cd2 100644
> --- a/man/man8/Makefile
> +++ b/man/man8/Makefile
> @@ -13,13 +13,20 @@ LSRCFILES	= $(MAN_PAGES)
>  
>  default : $(MAN_PAGES)
>  
> -LDIRT		= mkfs.xfs.8
> +LDIRT		= mkfs.xfs.8 cfg.8
>  
>  include $(BUILDRULES)
>  
> -mkfs.xfs.8: mkfs.xfs.8.in
> +cfg.8: ../../mkfs/mkconfig

+cfg.8: $(TOPDIR)/mkfs/mkconfig

> +	@echo "    [GENERATE] $@"
> +	$(Q) ../../mkfs/mkconfig -m > $@

+	$(Q) $(TOPDIR)/mkfs/mkconfig -m > $@

(when in Rome...)

> +
> +mkfs.xfs.8: mkfs.xfs.8.in cfg.8
>  	@echo "    [SED]    $@"
> -	$(Q)$(SED) -e "s|@sysconfdir@|$(PKG_ETC_DIR)|g" < $< > $@
> +	$(Q) $(SED) -e "s|@sysconfdir@|$(PKG_ETC_DIR)|g" \
> +		   -e "/@default_mkfs_cfg@/r cfg.8" \
> +		   -e "s/@default_mkfs_cfg@//g" \
> +		   < $< > $@

Just for kicks can we use a consistent sed delimiter?

Also ... meh, ok, that sed command works.

/@default_mkfs_cfg@/{
	s/@default_mkfs_cfg@//g
	r cfg.8
}
works too I think ;)  But ... eh, yours works, it's fine.

>  
>  install : default
>  	$(INSTALL) -m 755 -d $(MAN_DEST)
> diff --git a/man/man8/mkfs.xfs.8.in b/man/man8/mkfs.xfs.8.in
> index cf4bdf82..4a557910 100644
> --- a/man/man8/mkfs.xfs.8.in
> +++ b/man/man8/mkfs.xfs.8.in
> @@ -970,40 +970,7 @@ Currently all default parameters can only be either enabled or disabled,
>  with a value of 1 to enable or 0 to disable.
>  See below for a list of all supported configuration parameters and their
>  current built-in default settings.
> -.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
> +@default_mkfs_cfg@
>  .PP
>  .SH SEE ALSO
>  .BR xfs (5),
> diff --git a/mkfs/mkconfig.c b/mkfs/mkconfig.c
> index 3826b0bf..ef00f6d9 100644
> --- a/mkfs/mkconfig.c
> +++ b/mkfs/mkconfig.c
> @@ -124,9 +124,13 @@ main(
>  	struct subopt_map	*submap;
>  	int			c;
>  	unsigned int		i, j;
> +	bool			manpage = false;
>  
> -	while ((c = getopt(argc, argv, "")) != EOF) {
> +	while ((c = getopt(argc, argv, "m")) != EOF) {
>  		switch (c) {
> +		case 'm':
> +			manpage = true;
> +			break;
>  		case '?':
>  			fprintf(stderr, "Unknown option %c.\n", optopt);
>  			return 1;
> @@ -137,19 +141,25 @@ main(
>  		return 1;
>  	}
>  
> -	printf("# mkfs.xfs configuration file to collect settings.\n");
> -	printf("# See the mkfs.xfs(8) manpage for details.\n");
> -	printf("# Copy this file to %s/%s to override the built-in defaults.\n",
> -			MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
> +	if (!manpage) {
> +		printf("# mkfs.xfs configuration file to collect settings.\n");
> +		printf("# See the mkfs.xfs(8) manpage for details.\n");
> +		printf("# Copy this file to %s/%s to override the built-in defaults.\n",
> +				MKFS_XFS_CONF_DIR, MKFS_XFS_DEFAULT_CONFIG);
> +	}
>  
>  	for (i = 0; i < ARRAY_SIZE(confopts_tab); i++, opts++) {
> -		if (i > 0)
> +		if (manpage)
> +			printf(".PP\n.B ");
> +		else if (i > 0)
>  			printf("\n");
>  		printf("[%s]\n", opts->name);
>  		submap = opts->subopts;
>  		for (j = 0;
>  		     j < ARRAY_SIZE(opts->subopts) && submap->suboptname;
>  		     j++, submap++) {
> +			if (manpage)
> +				printf(".br\n.B ");
>  			printf("%s = ", submap->suboptname);
>  			switch (submap->type) {
>  			case FV_BOOL:
> 
> --
> 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 0/5] xfsprogs-4.17: mkfs config file enhancements

[Dave Chinner]

On Wed, Jun 13, 2018 at 12:31:39PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> This series refactors the build system so that we can generate a mkfs
> configuration file template from the defaults hardwired into the source
> code.  We can ship the template as /etc/xfs/mkfs/default.template and we
> can also put it in the mkfs.xfs manpage.  The series is based off of
> Eric's mkfs-config branch as of 12-June-2018.

Why?

I don't see any advantage in writing a bunch of code to "optimise"
the process of changing a default template file. We don't change
defaults very often, and it only takes a few minutes to do manually.

With this change, we'll have code to maintain it to ensure that the
file gets updated properly, and it will probably take more time and
effort to validate that the generated file is correct (and debug if
it's not!) compared to the 30s it will take to hand edit the
template file to change or add a new default...

Speaking of which, I didn't see any check that tells us the template
was built correctly. Am I expected to have to check the template
file after I've installed the built packages to determine that it
was built correctly by the distro package builder?

I'm also not convinced we should ship a "default.template" file,
either.  I'd much prefer we ship a real config file with all the
options defined but commented out as a "start here template".

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Eric Sandeen]

On 6/13/18 11:06 PM, Dave Chinner wrote:
> With this change, we'll have code to maintain it to ensure that the
> file gets updated properly, and it will probably take more time and
> effort to validate that the generated file is correct (and debug if
> it's not!) compared to the 30s it will take to hand edit the
> template file to change or add a new default...

Well, yeah.  /if/ we need a template, as well as text in a man page,
then this gets it down to editing 1 file instead of two, I guess.
But it really seems like we need to rethink these structures to get it
all tied together, not requiring 2 or 3 manual updates across several files.
It's bound to get out of sync.  I guess that can wait, but right now this
dispersal isn't good.

> I'm also not convinced we should ship a "default.template" file,
> either.  I'd much prefer we ship a real config file with all the
> options defined but commented out as a "start here template".

As the code stands today, if a 100% commented-out config file
exists, then mkfs will tell you that it's overriding built-in
defaults with the config file which contains ... nothing.
If we ship such a thing by default, that's what will happen by
default.

(Actually today it fails to parse and errors out but a patch
to fix that is on the list).

I guess we could behave as if "no config file" if a config file
contained no parseable tokens ... but that gets weird too.

Putting a blank config file in place to be parsed by default
is messy, so a .template approach seems reasonable to me, if we
need such a thing at all.

-Eric

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Dave Chinner]

On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> On 6/13/18 11:06 PM, Dave Chinner wrote:
> > With this change, we'll have code to maintain it to ensure that the
> > file gets updated properly, and it will probably take more time and
> > effort to validate that the generated file is correct (and debug if
> > it's not!) compared to the 30s it will take to hand edit the
> > template file to change or add a new default...
> 
> Well, yeah.  /if/ we need a template, as well as text in a man page,
> then this gets it down to editing 1 file instead of two, I guess.
> But it really seems like we need to rethink these structures to get it
> all tied together, not requiring 2 or 3 manual updates across several files.
> It's bound to get out of sync.  I guess that can wait, but right now this
> dispersal isn't good.

So let's get the basic conig file stuff in first, then cosolidate,
then add all the bells and whistles.

Too many cooks trying to add all their own bells and whistles before
the core behaviour, infrastructure and implementation was nailed
down was pretty much what lead all the tablised CLI option parsing
code. And we're doing it again with this config file stuff...

> > I'm also not convinced we should ship a "default.template" file,
> > either.  I'd much prefer we ship a real config file with all the
> > options defined but commented out as a "start here template".
> 
> As the code stands today, if a 100% commented-out config file
> exists, then mkfs will tell you that it's overriding built-in
> defaults with the config file which contains ... nothing.
> If we ship such a thing by default, that's what will happen by
> default.

Then that's a bug in the new parsing code and needs fixing. :)

> (Actually today it fails to parse and errors out but a patch
> to fix that is on the list).
> 
> I guess we could behave as if "no config file" if a config file
> contained no parseable tokens ... but that gets weird too.

Don't see why that is wierd, either - it's pretty common behaviour
for package shipped config files to have all options are present but
commented out.

> Putting a blank config file in place to be parsed by default
> is messy, so a .template approach seems reasonable to me, if we
> need such a thing at all.

If we really need a separate template, then perhaps it should be in
the mkfs config file man page where we document all the supported
options?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Darrick J. Wong]

On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> > On 6/13/18 11:06 PM, Dave Chinner wrote:
> > > With this change, we'll have code to maintain it to ensure that the
> > > file gets updated properly, and it will probably take more time and
> > > effort to validate that the generated file is correct (and debug if
> > > it's not!) compared to the 30s it will take to hand edit the
> > > template file to change or add a new default...

Counterpoint: the mkfs config file template in mkfs.xfs.8 is already
incorrect; sparse inodes was enabled by default in 4.16 but the manpage
cfg file says it isn't.  This is exactly what happens when we rely on
ourselves to hand edit the template file.

Put the defaults in *one* place in the source code tree and generate
everything else off of that, instead of keeping multiple copies that
will then get out of sync.  We clearly suck at this maintenance.

> > Well, yeah.  /if/ we need a template, as well as text in a man page,
> > then this gets it down to editing 1 file instead of two, I guess.
> > But it really seems like we need to rethink these structures to get it
> > all tied together, not requiring 2 or 3 manual updates across several files.
> > It's bound to get out of sync.  I guess that can wait, but right now this
> > dispersal isn't good.
> 
> So let's get the basic conig file stuff in first, then cosolidate,
> then add all the bells and whistles.

That's where I started from -- fix the existing mess with the manpage,
then fix the problems in the parsing code, then move all the CLI parsing and
validation code out of xfs_mkfs.c because it's just too long.

Granted, I haven't gotten all the way there yet, so perhaps it was a bad
idea to post just the first of those three parts.

> Too many cooks trying to add all their own bells and whistles before
> the core behaviour, infrastructure and implementation was nailed
> down was pretty much what lead all the tablised CLI option parsing
> code. And we're doing it again with this config file stuff...

Yeah.  TBH I feel at a loss for what exactly your vision is for how mkfs
option parsing is supposed to work in the end.  How close is this:

1. First we have the mkfs_default_params, which are the builtin
defaults.

2. Configuration file parameters are supposed to change the
mkfs_default_params, yes?

3. The mkfs_default_params are used to seed the cli_params.

4. The getopt loop pulls the CLI option data into the cli_params.

5. At this point we've melded together the builtin options
with whatever the config file told us about, then folded in the CLI
options, right?

6. Now we start calling the validate_* and calculate_* functions.  These
functions check the full set of configuration data and copy them into
the mkfs_params; and they calculate whatever other bits are needed to
finish the mkfs_params.

7. Finally the validated mkfs_params are fed to the initialisation
functions to set up the actual filesystem.

So AFAICT what we have now is a big collection of ops_params that
contain all the cli parsing options and a bunch of functions that take
the cli strings and fill out the appropriate parts of cli_params.

Luis built a confopts_tab(le) that mapped cfgfile options to parsing
functions that know how to take the cfgfile strings and fill out the
appropriate parts of mkfs_default_params.  I understand now that from
these first five patches it sure looks like I'm building yet a third
structure, but this isn't the endgame -- I want a data structure that
maps cfgfile options directly to the mkfs default params.  I'm doing
this by building the new direct-mapped structure on the side and
migrating the parser code to use that.

Ås for "how do I check that the distro provided default template even
works?", I'm working on building an xfstest to make sure that the
default templates we ship (and whatever might be in
/etc/xfs/mkfs/default) actually produce a functioning filesystem.

> > > I'm also not convinced we should ship a "default.template" file,
> > > either.  I'd much prefer we ship a real config file with all the
> > > options defined but commented out as a "start here template".
> > 
> > As the code stands today, if a 100% commented-out config file
> > exists, then mkfs will tell you that it's overriding built-in
> > defaults with the config file which contains ... nothing.
> > If we ship such a thing by default, that's what will happen by
> > default.
> 
> Then that's a bug in the new parsing code and needs fixing. :)
> 
> > (Actually today it fails to parse and errors out but a patch
> > to fix that is on the list).
> > 
> > I guess we could behave as if "no config file" if a config file
> > contained no parseable tokens ... but that gets weird too.
> 
> Don't see why that is wierd, either - it's pretty common behaviour
> for package shipped config files to have all options are present but
> commented out.

Agreed, but I think Eric wanted to be cautious and not have mkfs parsing
the default config file until this settles a little more, so I designed
it to spit out a default.template file.  For now users will continue to
use the same old code paths unless they explicitly light up the new
ones by providing /etc/xfs/mkfs/default or -c on their own.

--D

> > Putting a blank config file in place to be parsed by default
> > is messy, so a .template approach seems reasonable to me, if we
> > need such a thing at all.
> 
> If we really need a separate template, then perhaps it should be in
> the mkfs config file man page where we document all the supported
> options?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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 0/5] xfsprogs-4.17: mkfs config file enhancements

[Luis R. Rodriguez]

On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
> On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> > On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> > > On 6/13/18 11:06 PM, Dave Chinner wrote:
> > > > With this change, we'll have code to maintain it to ensure that the
> > > > file gets updated properly, and it will probably take more time and
> > > > effort to validate that the generated file is correct (and debug if
> > > > it's not!) compared to the 30s it will take to hand edit the
> > > > template file to change or add a new default...

If we had an xfstest to *test* that same generated file, this would not
be an issue and from what I gather we need quite a bit of work to get there.

I'm working on a test for config stuff but that will just test for now
(and this reveals some future work needed):

a) a set of config files we know should work and ensure they produce the
   same filesystem as if we had used CLI params. We can use xfs_db -c version
   against both filesystems and check that each differences. Since this would
   use the same xfsprogs for the results of a config based filesystem and
   the CLI based filesystem the diff would only generate if there really
   was a change between both runs, and you can use any xfsprogs version
   for it.

   It does however leave actual expected results on the filesystem up to
   a separate test, it assumes that xfs/191-input-validation is doing its
   job, but word is that needs some love.

b) a set of invalid config files and ensure they never work

c) test to ensure cli can override config params

To test that a self generated config file works would be a next step (d),
and I think we can validate it by also making sure it yields the same
filesystem as if we just ran mkfs.xfs with no options, so same strategy
as in a).

> Counterpoint: the mkfs config file template in mkfs.xfs.8 is already
> incorrect; sparse inodes was enabled by default in 4.16 but the manpage
> cfg file says it isn't.  This is exactly what happens when we rely on
> ourselves to hand edit the template file.
> 
> Put the defaults in *one* place in the source code tree and generate
> everything else off of that, instead of keeping multiple copies that
> will then get out of sync.  We clearly suck at this maintenance.

I personally prefer this automation, so with this is mind I think this
is a change in the right direction.

> > > Well, yeah.  /if/ we need a template, as well as text in a man page,
> > > then this gets it down to editing 1 file instead of two, I guess.
> > > But it really seems like we need to rethink these structures to get it
> > > all tied together, not requiring 2 or 3 manual updates across several files.
> > > It's bound to get out of sync.  I guess that can wait, but right now this
> > > dispersal isn't good.
> > 
> > So let's get the basic conig file stuff in first, then cosolidate,
> > then add all the bells and whistles.
> 
> That's where I started from -- fix the existing mess with the manpage,

That should be a separate patch first.

> then fix the problems in the parsing code, then move all the CLI parsing and
> validation code out of xfs_mkfs.c because it's just too long.

Yay!

> Granted, I haven't gotten all the way there yet, so perhaps it was a bad
> idea to post just the first of those three parts.
> 
> > Too many cooks trying to add all their own bells and whistles before
> > the core behaviour, infrastructure and implementation was nailed
> > down was pretty much what lead all the tablised CLI option parsing
> > code. And we're doing it again with this config file stuff...
> 
> Yeah.  TBH I feel at a loss for what exactly your vision is for how mkfs
> option parsing is supposed to work in the end.  How close is this:

BTW it seems we may need to document this vision as otherwise others may
run into issues later as well.

> 1. First we have the mkfs_default_params, which are the builtin
> defaults.
> 
> 2. Configuration file parameters are supposed to change the
> mkfs_default_params, yes?
> 
> 3. The mkfs_default_params are used to seed the cli_params.
> 
> 4. The getopt loop pulls the CLI option data into the cli_params.
> 
> 5. At this point we've melded together the builtin options
> with whatever the config file told us about, then folded in the CLI
> options, right?
> 
> 6. Now we start calling the validate_* and calculate_* functions.  These
> functions check the full set of configuration data and copy them into
> the mkfs_params; and they calculate whatever other bits are needed to
> finish the mkfs_params.
> 
> 7. Finally the validated mkfs_params are fed to the initialisation
> functions to set up the actual filesystem.

FWIW this at least matches my own expectations so far.

> So AFAICT what we have now is a big collection of ops_params that
> contain all the cli parsing options and a bunch of functions that take
> the cli strings and fill out the appropriate parts of cli_params.
> 
> Luis built a confopts_tab(le) that mapped cfgfile options to parsing
> functions that know how to take the cfgfile strings and fill out the
> appropriate parts of mkfs_default_params.  I understand now that from
> these first five patches it sure looks like I'm building yet a third
> structure, but this isn't the endgame -- I want a data structure that
> maps cfgfile options directly to the mkfs default params.  I'm doing
> this by building the new direct-mapped structure on the side and
> migrating the parser code to use that.
> 
> Ås for "how do I check that the distro provided default template even
> works?", I'm working on building an xfstest to make sure that the
> default templates we ship (and whatever might be in
> /etc/xfs/mkfs/default) actually produce a functioning filesystem.

With proper testing in place for regular configs stuff (the tests
I have to write) and this I see your changes as desirable but I'm
wondering if this should just wait until the next cycle so we at
least have some effort on the testing started already?

  Luis

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Darrick J. Wong]

On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> > > On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> > > > On 6/13/18 11:06 PM, Dave Chinner wrote:
> > > > > With this change, we'll have code to maintain it to ensure that the
> > > > > file gets updated properly, and it will probably take more time and
> > > > > effort to validate that the generated file is correct (and debug if
> > > > > it's not!) compared to the 30s it will take to hand edit the
> > > > > template file to change or add a new default...
> 
> If we had an xfstest to *test* that same generated file, this would not
> be an issue and from what I gather we need quite a bit of work to get there.
> 
> I'm working on a test for config stuff but that will just test for now
> (and this reveals some future work needed):
> 
> a) a set of config files we know should work and ensure they produce the
>    same filesystem as if we had used CLI params. We can use xfs_db -c version
>    against both filesystems and check that each differences. Since this would
>    use the same xfsprogs for the results of a config based filesystem and
>    the CLI based filesystem the diff would only generate if there really
>    was a change between both runs, and you can use any xfsprogs version
>    for it.

Hmm, just out of curiosity, are there any mkfs cli/config options that
do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?

>    It does however leave actual expected results on the filesystem up to
>    a separate test, it assumes that xfs/191-input-validation is doing its
>    job, but word is that needs some love.
> 
> b) a set of invalid config files and ensure they never work

Agreed, I have some twisty ones of my own, though fixing them requires
some amount of sscanf format string tweaking. :)

> c) test to ensure cli can override config params

<nod>

> To test that a self generated config file works would be a next step (d),
> and I think we can validate it by also making sure it yields the same
> filesystem as if we just ran mkfs.xfs with no options, so same strategy
> as in a).

Ok, good.  I'd also argue for a test that tries every file in
/etc/xfs/mkfs/* to see if mkfs will format the filesystem described in
the config file, that way we can pick up all the distro-packaged files.

> > Counterpoint: the mkfs config file template in mkfs.xfs.8 is already
> > incorrect; sparse inodes was enabled by default in 4.16 but the manpage
> > cfg file says it isn't.  This is exactly what happens when we rely on
> > ourselves to hand edit the template file.
> > 
> > Put the defaults in *one* place in the source code tree and generate
> > everything else off of that, instead of keeping multiple copies that
> > will then get out of sync.  We clearly suck at this maintenance.
> 
> I personally prefer this automation, so with this is mind I think this
> is a change in the right direction.

:)

> > > > Well, yeah.  /if/ we need a template, as well as text in a man page,
> > > > then this gets it down to editing 1 file instead of two, I guess.
> > > > But it really seems like we need to rethink these structures to get it
> > > > all tied together, not requiring 2 or 3 manual updates across several files.
> > > > It's bound to get out of sync.  I guess that can wait, but right now this
> > > > dispersal isn't good.
> > > 
> > > So let's get the basic conig file stuff in first, then cosolidate,
> > > then add all the bells and whistles.
> > 
> > That's where I started from -- fix the existing mess with the manpage,
> 
> That should be a separate patch first.

> > then fix the problems in the parsing code, then move all the CLI parsing and
> > validation code out of xfs_mkfs.c because it's just too long.
> 
> Yay!
> 
> > Granted, I haven't gotten all the way there yet, so perhaps it was a bad
> > idea to post just the first of those three parts.
> > 
> > > Too many cooks trying to add all their own bells and whistles before
> > > the core behaviour, infrastructure and implementation was nailed
> > > down was pretty much what lead all the tablised CLI option parsing
> > > code. And we're doing it again with this config file stuff...
> > 
> > Yeah.  TBH I feel at a loss for what exactly your vision is for how mkfs
> > option parsing is supposed to work in the end.  How close is this:
> 
> BTW it seems we may need to document this vision as otherwise others may
> run into issues later as well.

Yes.  This should go in xfs_mkfs.c after moving all the other stuff out.

> > 1. First we have the mkfs_default_params, which are the builtin
> > defaults.
> > 
> > 2. Configuration file parameters are supposed to change the
> > mkfs_default_params, yes?
> > 
> > 3. The mkfs_default_params are used to seed the cli_params.
> > 
> > 4. The getopt loop pulls the CLI option data into the cli_params.
> > 
> > 5. At this point we've melded together the builtin options
> > with whatever the config file told us about, then folded in the CLI
> > options, right?
> > 
> > 6. Now we start calling the validate_* and calculate_* functions.  These
> > functions check the full set of configuration data and copy them into
> > the mkfs_params; and they calculate whatever other bits are needed to
> > finish the mkfs_params.
> > 
> > 7. Finally the validated mkfs_params are fed to the initialisation
> > functions to set up the actual filesystem.
> 
> FWIW this at least matches my own expectations so far.
> 
> > So AFAICT what we have now is a big collection of ops_params that
> > contain all the cli parsing options and a bunch of functions that take
> > the cli strings and fill out the appropriate parts of cli_params.
> > 
> > Luis built a confopts_tab(le) that mapped cfgfile options to parsing
> > functions that know how to take the cfgfile strings and fill out the
> > appropriate parts of mkfs_default_params.  I understand now that from
> > these first five patches it sure looks like I'm building yet a third
> > structure, but this isn't the endgame -- I want a data structure that
> > maps cfgfile options directly to the mkfs default params.  I'm doing
> > this by building the new direct-mapped structure on the side and
> > migrating the parser code to use that.
> > 
> > Ås for "how do I check that the distro provided default template even
> > works?", I'm working on building an xfstest to make sure that the
> > default templates we ship (and whatever might be in
> > /etc/xfs/mkfs/default) actually produce a functioning filesystem.
> 
> With proper testing in place for regular configs stuff (the tests
> I have to write) and this I see your changes as desirable but I'm
> wondering if this should just wait until the next cycle so we at
> least have some effort on the testing started already?

Agreed.  I'm scurrying back under the rocks so that Eric can get
xfsprogs 4.17 out the door.

(I'm actually going to go debug some hardware and see if I can get
fscounter scrub working.)

--D

>   Luis
> --
> 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 0/5] xfsprogs-4.17: mkfs config file enhancements

[Luis R. Rodriguez]

On Thu, Jun 14, 2018 at 10:59:09AM -0700, Darrick J. Wong wrote:
> On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
> > a) a set of config files we know should work and ensure they produce the
> >    same filesystem as if we had used CLI params. We can use xfs_db -c version
> >    against both filesystems and check that each differences. Since this would
> >    use the same xfsprogs for the results of a config based filesystem and
> >    the CLI based filesystem the diff would only generate if there really
> >    was a change between both runs, and you can use any xfsprogs version
> >    for it.
> 
> Hmm, just out of curiosity, are there any mkfs cli/config options that
> do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?

A good question indeed. But more importantly, how could we verify that in
the future automatically?

> >    It does however leave actual expected results on the filesystem up to
> >    a separate test, it assumes that xfs/191-input-validation is doing its
> >    job, but word is that needs some love.
> > 
> > b) a set of invalid config files and ensure they never work
> 
> Agreed, I have some twisty ones of my own, 

Great!

> though fixing them requires some amount of sscanf format string tweaking. :)

Oh fun, so we can have the test fail for the get go. Note that at one point
of parser evolution we may get to the point of actually reaching a point of
it being more better to just embrace a library. But in terms of size and
maintenance due to the simplicity of what we need to parse we were just not
there yet.

Let's recall that sharing the profile parser from e2fsprogs was the smallest,
but libini_config the more generic one.

If you want to experiment with them:

https://gitlab.com/mcgrof/libini_config-demo.git
https://gitlab.com/mcgrof/libekprofile.git

> > c) test to ensure cli can override config params
> 
> <nod>
> 
> > To test that a self generated config file works would be a next step (d),
> > and I think we can validate it by also making sure it yields the same
> > filesystem as if we just ran mkfs.xfs with no options, so same strategy
> > as in a).
> 
> Ok, good.  I'd also argue for a test that tries every file in
> /etc/xfs/mkfs/* to see if mkfs will format the filesystem described in
> the config file, that way we can pick up all the distro-packaged files.

Works with me, so we have 3 tests in mind already.

> > With proper testing in place for regular configs stuff (the tests
> > I have to write) and this I see your changes as desirable but I'm
> > wondering if this should just wait until the next cycle so we at
> > least have some effort on the testing started already?
> 
> Agreed.  I'm scurrying back under the rocks so that Eric can get
> xfsprogs 4.17 out the door.

OK cool, I'll try to finish the discussed test soon.

> (I'm actually going to go debug some hardware and see if I can get
> fscounter scrub working.)

:)

  Luis

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Darrick J. Wong]

On Thu, Jun 14, 2018 at 08:16:02PM +0200, Luis R. Rodriguez wrote:
> On Thu, Jun 14, 2018 at 10:59:09AM -0700, Darrick J. Wong wrote:
> > On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
> > > a) a set of config files we know should work and ensure they produce the
> > >    same filesystem as if we had used CLI params. We can use xfs_db -c version
> > >    against both filesystems and check that each differences. Since this would
> > >    use the same xfsprogs for the results of a config based filesystem and
> > >    the CLI based filesystem the diff would only generate if there really
> > >    was a change between both runs, and you can use any xfsprogs version
> > >    for it.
> > 
> > Hmm, just out of curiosity, are there any mkfs cli/config options that
> > do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?
> 
> A good question indeed. But more importantly, how could we verify that in
> the future automatically?
> 
> > >    It does however leave actual expected results on the filesystem up to
> > >    a separate test, it assumes that xfs/191-input-validation is doing its
> > >    job, but word is that needs some love.
> > > 
> > > b) a set of invalid config files and ensure they never work
> > 
> > Agreed, I have some twisty ones of my own, 
> 
> Great!
> 
> > though fixing them requires some amount of sscanf format string tweaking. :)
> 
> Oh fun, so we can have the test fail for the get go. Note that at one point
> of parser evolution we may get to the point of actually reaching a point of
> it being more better to just embrace a library. But in terms of size and
> maintenance due to the simplicity of what we need to parse we were just not
> there yet.

[future babble]

It's not /that/ much tweaking.  First a function that trims leading and
trailing whitespace, then cuts off the string at the first '#' (so we
can have eol-comments anywhere).

Section headers:
n = sscanf(line, " [ %m[^] \f\n\r\t\v] %ms %m[^\n]", &tag, &cp, &junk);

We pick up the section name (in *tag) if cp == "]" and n == 2 (i.e.
there's no junk at the end of the line.

"# some comment"
"[data]"
"[data] # some comment"
" [data]"
"[ data]"
"[data ]"
"[data] "
<repeat but with tabs instead of spaces>

"[data] noalign = 1"
"[data cow]"
"[data"
"data]"
" [data cow]"
"[ data cow]"
"[data cow]"
"[data cow ]"
"[data cow] "
"[data.cow]"
"[data noalign = 1]"
"[nonexistentsection]"

Key/value:

n = sscanf(line, " %m[^][ \f\n\r\t\v=] %m[=] %m[^\n]", &key, &eq, &val);

We pick up the key/value pair if n == 3, eq == "]" and *key is found in
the current section header, and if *val can be stroull'd.

Assuming a [data] section,

"# some comment"
"noalign=1"
"noalign=1 # some comment"
" noalign=1"
"noalign =1"
"noalign= 1"
"noalign=1 "
" noalign =1"
" noalign= 1"
" noalign=1 "
"noalign = 1"
"noalign =1 "
"noalign= 1 "
" noalign = 1"
" noalign= 1 "
"noalign = 1 "
<repeat with tabs>

"noalign moo = 1"
"noalign is 1"
"noalign = 10"
"noalign = 109825091285091825091285018250"
"noalign = [metadata]"
"moocow"
"moocow = 5"

etc.  So long as we tag it EXPERIMENTAL I don't have a problem with
landing the current code as-is for 4.17.

> Let's recall that sharing the profile parser from e2fsprogs was the smallest,
> but libini_config the more generic one.
> 
> If you want to experiment with them:
> 
> https://gitlab.com/mcgrof/libini_config-demo.git
> https://gitlab.com/mcgrof/libekprofile.git
> 
> > > c) test to ensure cli can override config params
> > 
> > <nod>
> > 
> > > To test that a self generated config file works would be a next step (d),
> > > and I think we can validate it by also making sure it yields the same
> > > filesystem as if we just ran mkfs.xfs with no options, so same strategy
> > > as in a).
> > 
> > Ok, good.  I'd also argue for a test that tries every file in
> > /etc/xfs/mkfs/* to see if mkfs will format the filesystem described in
> > the config file, that way we can pick up all the distro-packaged files.
> 
> Works with me, so we have 3 tests in mind already.
> 
> > > With proper testing in place for regular configs stuff (the tests
> > > I have to write) and this I see your changes as desirable but I'm
> > > wondering if this should just wait until the next cycle so we at
> > > least have some effort on the testing started already?
> > 
> > Agreed.  I'm scurrying back under the rocks so that Eric can get
> > xfsprogs 4.17 out the door.
> 
> OK cool, I'll try to finish the discussed test soon.

Looking forward to it!

--D

> > (I'm actually going to go debug some hardware and see if I can get
> > fscounter scrub working.)
> 
> :)
> 
>   Luis
> --
> 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 0/5] xfsprogs-4.17: mkfs config file enhancements

[Luis R. Rodriguez]

On Thu, Jun 14, 2018 at 11:35:28AM -0700, Darrick J. Wong wrote:
> Section headers:
> n = sscanf(line, " [ %m[^] \f\n\r\t\v] %ms %m[^\n]", &tag, &cp, &junk);
> 
> We pick up the section name (in *tag) if cp == "]" and n == 2 (i.e.
> there's no junk at the end of the line.
> 
> "# some comment"
> "[data]"
> "[data] # some comment"
> " [data]"
> "[ data]"
> "[data ]"
> "[data] "
> <repeat but with tabs instead of spaces>
> 
> "[data] noalign = 1"
> "[data cow]"
> "[data"
> "data]"
> " [data cow]"
> "[ data cow]"
> "[data cow]"
> "[data cow ]"
> "[data cow] "
> "[data.cow]"
> "[data noalign = 1]"
> "[nonexistentsection]"
> 
> Key/value:
> 
> n = sscanf(line, " %m[^][ \f\n\r\t\v=] %m[=] %m[^\n]", &key, &eq, &val);
> 
> We pick up the key/value pair if n == 3, eq == "]" and *key is found in
> the current section header, and if *val can be stroull'd.
> 
> Assuming a [data] section,
> 
> "# some comment"
> "noalign=1"
> "noalign=1 # some comment"
> " noalign=1"
> "noalign =1"
> "noalign= 1"
> "noalign=1 "
> " noalign =1"
> " noalign= 1"
> " noalign=1 "
> "noalign = 1"
> "noalign =1 "
> "noalign= 1 "
> " noalign = 1"
> " noalign= 1 "
> "noalign = 1 "
> <repeat with tabs>
> 
> "noalign moo = 1"
> "noalign is 1"
> "noalign = 10"
> "noalign = 109825091285091825091285018250"
> "noalign = [metadata]"
> "moocow"
> "moocow = 5"

I tried some stuff but I could not get it to barf. Can you send me some
config file attachments which do make it puke? Or patches? :)

  Luis

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Eric Sandeen]

On 6/14/18 12:59 PM, Darrick J. Wong wrote:
> On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
>> On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
>>> On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
>>>> On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
>>>>> On 6/13/18 11:06 PM, Dave Chinner wrote:
>>>>>> With this change, we'll have code to maintain it to ensure that the
>>>>>> file gets updated properly, and it will probably take more time and
>>>>>> effort to validate that the generated file is correct (and debug if
>>>>>> it's not!) compared to the 30s it will take to hand edit the
>>>>>> template file to change or add a new default...
>>
>> If we had an xfstest to *test* that same generated file, this would not
>> be an issue and from what I gather we need quite a bit of work to get there.
>>
>> I'm working on a test for config stuff but that will just test for now
>> (and this reveals some future work needed):
>>
>> a) a set of config files we know should work and ensure they produce the
>>    same filesystem as if we had used CLI params. We can use xfs_db -c version
>>    against both filesystems and check that each differences. Since this would
>>    use the same xfsprogs for the results of a config based filesystem and
>>    the CLI based filesystem the diff would only generate if there really
>>    was a change between both runs, and you can use any xfsprogs version
>>    for it.
> 
> Hmm, just out of curiosity, are there any mkfs cli/config options that
> do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?

Of the options that actually write [meta]data to disk, these don't show
up in the info output:

-m uuid=
-l agnum=
-L <label>

The rest do in some form, though as you know several mkfs options all specify
the same metadata detail in different ways, so i.e. su/sunit distill down to
a single xfs_info field.

-Eric

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Dave Chinner]

On Thu, Jun 14, 2018 at 02:05:02PM -0500, Eric Sandeen wrote:
> 
> 
> On 6/14/18 12:59 PM, Darrick J. Wong wrote:
> > On Thu, Jun 14, 2018 at 07:46:40PM +0200, Luis R. Rodriguez wrote:
> >> On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
> >>> On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> >>>> On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> >>>>> On 6/13/18 11:06 PM, Dave Chinner wrote:
> >>>>>> With this change, we'll have code to maintain it to ensure that the
> >>>>>> file gets updated properly, and it will probably take more time and
> >>>>>> effort to validate that the generated file is correct (and debug if
> >>>>>> it's not!) compared to the 30s it will take to hand edit the
> >>>>>> template file to change or add a new default...
> >>
> >> If we had an xfstest to *test* that same generated file, this would not
> >> be an issue and from what I gather we need quite a bit of work to get there.
> >>
> >> I'm working on a test for config stuff but that will just test for now
> >> (and this reveals some future work needed):
> >>
> >> a) a set of config files we know should work and ensure they produce the
> >>    same filesystem as if we had used CLI params. We can use xfs_db -c version
> >>    against both filesystems and check that each differences. Since this would
> >>    use the same xfsprogs for the results of a config based filesystem and
> >>    the CLI based filesystem the diff would only generate if there really
> >>    was a change between both runs, and you can use any xfsprogs version
> >>    for it.
> > 
> > Hmm, just out of curiosity, are there any mkfs cli/config options that
> > do /not/ show up in the output of mkfs and/or 'xfs_db -c info'?
> 
> Of the options that actually write [meta]data to disk, these don't show
> up in the info output:
> 
> -m uuid=
> -l agnum=
> -L <label>

And the extent size hints and other attributes written to the root
inode. And anything in the protofile.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Dave Chinner]

On Wed, Jun 13, 2018 at 11:29:49PM -0700, Darrick J. Wong wrote:
> On Thu, Jun 14, 2018 at 03:08:44PM +1000, Dave Chinner wrote:
> > On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
> > > On 6/13/18 11:06 PM, Dave Chinner wrote:
> > > > With this change, we'll have code to maintain it to ensure that the
> > > > file gets updated properly, and it will probably take more time and
> > > > effort to validate that the generated file is correct (and debug if
> > > > it's not!) compared to the 30s it will take to hand edit the
> > > > template file to change or add a new default...
> 
> Counterpoint: the mkfs config file template in mkfs.xfs.8 is already
> incorrect; sparse inodes was enabled by default in 4.16 but the manpage
> cfg file says it isn't.  This is exactly what happens when we rely on
> ourselves to hand edit the template file.
> 
> Put the defaults in *one* place in the source code tree and generate
> everything else off of that, instead of keeping multiple copies that
> will then get out of sync.  We clearly suck at this maintenance.

Then you need to explain the grander picture in your patch
description. The only thing that stuck out at me was "autogenerate
a template file" - none of the maintenance issues that get solved,
etc were in the description.

> > > Well, yeah.  /if/ we need a template, as well as text in a man page,
> > > then this gets it down to editing 1 file instead of two, I guess.
> > > But it really seems like we need to rethink these structures to get it
> > > all tied together, not requiring 2 or 3 manual updates across several files.
> > > It's bound to get out of sync.  I guess that can wait, but right now this
> > > dispersal isn't good.
> > 
> > So let's get the basic conig file stuff in first, then cosolidate,
> > then add all the bells and whistles.
> 
> That's where I started from -- fix the existing mess with the manpage,
> then fix the problems in the parsing code, then move all the CLI parsing and
> validation code out of xfs_mkfs.c because it's just too long.
> 
> Granted, I haven't gotten all the way there yet, so perhaps it was a bad
> idea to post just the first of those three parts.

More that there was no indication that this was anything more than
"more code we'll have to wade through later". i.e. it was presented
without the necessary context to understand what direction you were
heading.

That comes back to my "too many cooks" comment. Everyone seems to
have a different "killer feature" they want, but there has been no
discussion or co-ordination of how to deliver it all in a sane,
maintainable fashion.

If nothing else, my comments have prompted people to talk about what
they are trying to do rather than assuming everyone else knows what
everyone else wants.

> > Too many cooks trying to add all their own bells and whistles before
> > the core behaviour, infrastructure and implementation was nailed
> > down was pretty much what lead all the tablised CLI option parsing
> > code. And we're doing it again with this config file stuff...
> 
> Yeah.  TBH I feel at a loss for what exactly your vision is for how mkfs
> option parsing is supposed to work in the end.  How close is this:
> 
> 1. First we have the mkfs_default_params, which are the builtin
> defaults.
> 
> 2. Configuration file parameters are supposed to change the
> mkfs_default_params, yes?
> 
> 3. The mkfs_default_params are used to seed the cli_params.
> 
> 4. The getopt loop pulls the CLI option data into the cli_params.
> 
> 5. At this point we've melded together the builtin options
> with whatever the config file told us about, then folded in the CLI
> options, right?
> 
> 6. Now we start calling the validate_* and calculate_* functions.  These
> functions check the full set of configuration data and copy them into
> the mkfs_params; and they calculate whatever other bits are needed to
> finish the mkfs_params.
> 
> 7. Finally the validated mkfs_params are fed to the initialisation
> functions to set up the actual filesystem.

Pretty much spot on. It's set up as a data flow through specific
structures and actors that manipulate the data in those structures.
It's about as simple and concise as I could think of, and the fact
you've nailed it in one go tells me it's about as obvious as I could
of hoped for.

> So AFAICT what we have now is a big collection of ops_params that
> contain all the cli parsing options and a bunch of functions that take
> the cli strings and fill out the appropriate parts of cli_params.
> 
> Luis built a confopts_tab(le) that mapped cfgfile options to parsing
> functions that know how to take the cfgfile strings and fill out the
> appropriate parts of mkfs_default_params.

Right - what I want is for all the functionality to land, the
config file formats to stabilise and how we package/test/ship it all
to be settled /before/ we tried to consolidate everything back into
a common set of infrastructure and then expand it to do more things
for us.

> I understand now that from
> these first five patches it sure looks like I'm building yet a third
> structure, but this isn't the endgame -- I want a data structure that
> maps cfgfile options directly to the mkfs default params.  I'm doing
> this by building the new direct-mapped structure on the side and
> migrating the parser code to use that.

Ok, so you needed to explain this bigger picture up front, but I
knew nothing of it until now.  I'm happy for us to build new stuff,
but I really want to get things that are difficult to change or have
significant user impact if we change them right first.  IOWs, I
really don't want to have config file support ship with an
"experimental" tag as an excuse for not thinking though all the
issues before shipping it.

> Ås for "how do I check that the distro provided default template even
> works?", I'm working on building an xfstest to make sure that the
> default templates we ship (and whatever might be in
> /etc/xfs/mkfs/default) actually produce a functioning filesystem.

Which also wasn't explained :P

> > > > I'm also not convinced we should ship a "default.template" file,
> > > > either.  I'd much prefer we ship a real config file with all the
> > > > options defined but commented out as a "start here template".
> > > 
> > > As the code stands today, if a 100% commented-out config file
> > > exists, then mkfs will tell you that it's overriding built-in
> > > defaults with the config file which contains ... nothing.
> > > If we ship such a thing by default, that's what will happen by
> > > default.
> > 
> > Then that's a bug in the new parsing code and needs fixing. :)
> > 
> > > (Actually today it fails to parse and errors out but a patch
> > > to fix that is on the list).
> > > 
> > > I guess we could behave as if "no config file" if a config file
> > > contained no parseable tokens ... but that gets weird too.
> > 
> > Don't see why that is wierd, either - it's pretty common behaviour
> > for package shipped config files to have all options are present but
> > commented out.
> 
> Agreed, but I think Eric wanted to be cautious and not have mkfs parsing
> the default config file until this settles a little more, so I designed
> it to spit out a default.template file.  For now users will continue to
> use the same old code paths unless they explicitly light up the new
> ones by providing /etc/xfs/mkfs/default or -c on their own.

Sure.

But my point remains that separate config templates in /etc/ are
strange and unusual.  e.g. on debian, example config files usually
end up in /usr/share/doc/<pkg>/examples/, and lots of packages have
them:

$ ls /usr/share/doc/*/ |grep example |wc -l
186
$

Picking one a random:

$ ls /usr/share/doc/wpasupplicant/examples
ieee8021x.conf
openCryptoki.conf
plaintext.conf
udhcpd-p2p.conf
wep.conf
wpa2-eap-ccmp.conf
wpa-psk-tkip.conf
wpa-roam.conf
wpa_supplicant.conf.gz
$

Now, let's compare that to template config files
in /etc on the same system:

# find /etc/ |grep template |wc -l
28

Most of those a for wireless encryption setup for WICD. What
remains:

# find /etc/ |grep template |grep -v wicd
/etc/gimp/2.0/templaterc
/etc/lvm/profile/command_profile_template.profile
/etc/lvm/profile/metadata_profile_template.profile
/etc/exim4/exim4.conf.template
#

Yup: four packages of the 2547 packages installed on this machine
ship template config files. And the exim template was a Debian
package Maintainer decision to allow either split or global config
files to be used.

That's kinda what I'm getting at here - shipping config templates
in /etc seems to be quite unusual, and the location for example
config files is largely a distro packaging choice. i.e. the distro
package mantainer decides on whether a template is shipped and where
it is located, and that usually follows distro packaging guidelines.

IOWs, for the debian package build we should probably be putting
mkfs config template file into
/usr/share/doc/xfsprogs/examples/mkfs.conf, not /etc/xfs/....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Eric Sandeen]

On 6/14/18 6:33 PM, Dave Chinner wrote:
> That's kinda what I'm getting at here - shipping config templates
> in /etc seems to be quite unusual, and the location for example
> config files is largely a distro packaging choice. i.e. the distro
> package mantainer decides on whether a template is shipped and where
> it is located, and that usually follows distro packaging guidelines.
> 
> IOWs, for the debian package build we should probably be putting
> mkfs config template file into
> /usr/share/doc/xfsprogs/examples/mkfs.conf, not /etc/xfs/....

So imho we should put an example in /doc, or a fully-commented-out
config file in /etc (apache/dovecot/postfix do that, IIRC)

It doesn't really matter to me which one we do, though because we
decided to print the "defaults config file" source at mkfs time, printing
that for a file which does nothing is a bit of a surprise.  From
that perspective, I guess /doc sounds a little better to me.

-Eric

Re: [PATCH 0/5] xfsprogs-4.17: mkfs config file enhancements

[Eric Sandeen]

On 6/14/18 12:08 AM, Dave Chinner wrote:
> On Wed, Jun 13, 2018 at 11:23:09PM -0500, Eric Sandeen wrote:
>> On 6/13/18 11:06 PM, Dave Chinner wrote:
>>> With this change, we'll have code to maintain it to ensure that the
>>> file gets updated properly, and it will probably take more time and
>>> effort to validate that the generated file is correct (and debug if
>>> it's not!) compared to the 30s it will take to hand edit the
>>> template file to change or add a new default...
>>
>> Well, yeah.  /if/ we need a template, as well as text in a man page,
>> then this gets it down to editing 1 file instead of two, I guess.
>> But it really seems like we need to rethink these structures to get it
>> all tied together, not requiring 2 or 3 manual updates across several files.
>> It's bound to get out of sync.  I guess that can wait, but right now this
>> dispersal isn't good.
> 
> So let's get the basic conig file stuff in first, then cosolidate,
> then add all the bells and whistles.
> 
> Too many cooks trying to add all their own bells and whistles before
> the core behaviour, infrastructure and implementation was nailed
> down was pretty much what lead all the tablised CLI option parsing
> code. And we're doing it again with this config file stuff...
> 
>>> I'm also not convinced we should ship a "default.template" file,
>>> either.  I'd much prefer we ship a real config file with all the
>>> options defined but commented out as a "start here template".
>>
>> As the code stands today, if a 100% commented-out config file
>> exists, then mkfs will tell you that it's overriding built-in
>> defaults with the config file which contains ... nothing.
>> If we ship such a thing by default, that's what will happen by
>> default.
> 
> Then that's a bug in the new parsing code and needs fixing. :)

*shrug*  No, the parsing code is fine (in this respect), but I guess
we can change verbiage to indicate that a config file was used
without indicating that it actually /did/ anything....
 
>> (Actually today it fails to parse and errors out but a patch
>> to fix that is on the list).
>>
>> I guess we could behave as if "no config file" if a config file
>> contained no parseable tokens ... but that gets weird too.
> 
> Don't see why that is wierd, either - it's pretty common behaviour
> for package shipped config files to have all options are present but
> commented out.

Because most packages don't have this kinda weird behavior of telling
you on stdout which config file they're operating from, if any...

>> Putting a blank config file in place to be parsed by default
>> is messy, so a .template approach seems reasonable to me, if we
>> need such a thing at all.
> 
> If we really need a separate template, then perhaps it should be in
> the mkfs config file man page where we document all the supported
> options?

There is currently a "template" in the mkfs.xfs.8 manpage, insofar
as a template can exist in a manpage, and that's exactly what Darrick's
patch was autogenerating.

(I'm realizing though that there's not great or explicit documentation
of the mapping between commandline options and the allowable config
options.)

-Eric