[PATCH 0/3] mkfs: Configuration file defined options

[PATCH 0/3] mkfs: Configuration file defined options

[Dave Chinner]

Hi Folks,

Because needing config files for mkfs came up yet again in
discussion, here is a simple implementation of INI format config
files. These config files behave identically to options specified on
the command line - the do not change defaults, they do not override
CLI options, they are not overridden by cli options.

Example:

$ echo -e "[metadata]\ncrc = 0" > foo
$ mkfs/mkfs.xfs -N -c file=foo -d file=1,size=100m blah
Ini debug: file foo, section metadata, name crc, value 0
Parameters parsed from config file foo successfully
meta-data=blah                   isize=256    agcount=4, agsize=6400 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=0        finobt=0, sparse=0, rmapbt=0
         =                       reflink=0
data     =                       bsize=4096   blocks=25600, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=853, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
$

And there's a V4 filesystem as specified by the option defined
in the config file. If we do:

$ mkfs/mkfs.xfs -N -c file=foo -m crc=1 -d file=1,size=100m blah
Ini debug: file foo, section metadata, name crc, value 0
-m crc option respecified
Usage: mkfs.xfs
.....
$

You can see it errors out because the CRC option was specified in
both the config file and on the CLI.

There's lots of stuff we can do to make the conflict and respec
error messages better, but that doesn't change the basic
functionality of config file based mkfs options. To allow for future
changes to the way we want to apply the config file, I created a
full option subtype for config files. That means we can add another
option to say "apply config file as default values rather than as
options" if we decide that is functionality that we want to support.

But policy decisions like that are completely separate to the
mechanism, so these patches don't try to address desires to ship
"tuned" configs, system wide option files, shipping distro specific
defaults in config files, etc. This is purely a mechanism to allow
users to specify options via files instead of on the CLI.  No more,
no less.

So to preempt the endless bikeshedding from derailing getting this
functionality merged, all I'm going to say is "no". No feature
creep, no "but I want to ...", "can you make it pink...", nothing.
All I care about is providing a mechanism that people are still
asking for, and that's all I'm going to provide here.

I'm happy to fix bugs and code stuff that people want fixed to get
it merged, but in terms of functionality being provided I'm
essentially saying "take it or leave it" because I'm not going to
waste time bikeshedding this whole thing again.

This has only been given a basic smoke test right now (see above!
:).  I need to get Darrick's tests from the previous round of config
file bikeshedding working in my test environment to do more
substantial testing of this....

Cheers,

Dave.


Dave Chinner (3):
  build: add support for libinih for mkfs
  mkfs: add initial ini format config file parsing support
  mkfs: hook up suboption parsing to ini files

 configure.ac         |   3 +
 include/builddefs.in |   1 +
 include/linux.h      |   2 +-
 m4/package_inih.m4   |  20 ++++
 mkfs/Makefile        |   2 +-
 mkfs/xfs_mkfs.c      | 218 +++++++++++++++++++++++++++++++++++++------
 6 files changed, 218 insertions(+), 28 deletions(-)
 create mode 100644 m4/package_inih.m4

-- 
2.28.0

[PATCH 1/3] build: add support for libinih for mkfs

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Need to make sure the library is present so we can build mkfs with
config file support.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 configure.ac         |  3 +++
 include/builddefs.in |  1 +
 m4/package_inih.m4   | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+)
 create mode 100644 m4/package_inih.m4

diff --git a/configure.ac b/configure.ac
index fe1584e7704b..c0c7badccbdf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -145,6 +145,9 @@ AC_PACKAGE_UTILITIES(xfsprogs)
 AC_MULTILIB($enable_lib64)
 AC_RT($enable_librt)
 
+AC_PACKAGE_NEED_INI_H
+AC_PACKAGE_NEED_LIBINIH
+
 AC_PACKAGE_NEED_UUID_H
 AC_PACKAGE_NEED_UUIDCOMPARE
 
diff --git a/include/builddefs.in b/include/builddefs.in
index 30b2727a8db4..e8f447f92baf 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -27,6 +27,7 @@ LIBTERMCAP = @libtermcap@
 LIBEDITLINE = @libeditline@
 LIBBLKID = @libblkid@
 LIBDEVMAPPER = @libdevmapper@
+LIBINIH = @libinih@
 LIBXFS = $(TOPDIR)/libxfs/libxfs.la
 LIBFROG = $(TOPDIR)/libfrog/libfrog.la
 LIBXCMD = $(TOPDIR)/libxcmd/libxcmd.la
diff --git a/m4/package_inih.m4 b/m4/package_inih.m4
new file mode 100644
index 000000000000..a2775592e09d
--- /dev/null
+++ b/m4/package_inih.m4
@@ -0,0 +1,20 @@
+AC_DEFUN([AC_PACKAGE_NEED_INI_H],
+  [ AC_CHECK_HEADERS([ini.h])
+    if test $ac_cv_header_ini_h = no; then
+	echo
+	echo 'FATAL ERROR: could not find a valid ini.h header.'
+	echo 'Install the libinih development package.'
+	exit 1
+    fi
+  ])
+
+AC_DEFUN([AC_PACKAGE_NEED_LIBINIH],
+  [ AC_CHECK_LIB(inih, ini_parse,, [
+	echo
+	echo 'FATAL ERROR: could not find a valid inih library.'
+	echo 'Install the libinih library package.'
+	exit 1
+    ])
+    libinih=-linih
+    AC_SUBST(libinih)
+  ])
-- 
2.28.0

[PATCH 2/3] mkfs: add initial ini format config file parsing support

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Add the framework that will allow the config file to be supplied on
the CLI and passed to the library that will parse it. This does not
yet do any option parsing from the config file.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 mkfs/Makefile   |   2 +-
 mkfs/xfs_mkfs.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/mkfs/Makefile b/mkfs/Makefile
index 31482b08d559..b8805f7e1ea1 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -11,7 +11,7 @@ HFILES =
 CFILES = proto.c xfs_mkfs.c
 
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
-	$(LIBUUID)
+	$(LIBUUID) $(LIBINIH)
 LTDEPENDENCIES += $(LIBXFS) $(LIBXCMD) $(LIBFROG)
 LLDFLAGS = -static-libtool-libs
 
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280e388..6a373d614a56 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -11,6 +11,7 @@
 #include "libfrog/fsgeom.h"
 #include "libfrog/topology.h"
 #include "libfrog/convert.h"
+#include <ini.h>
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
 #define GIGABYTES(count, blog)	((uint64_t)(count) << (30 - (blog)))
@@ -44,6 +45,11 @@ enum {
 	B_MAX_OPTS,
 };
 
+enum {
+	C_FILE = 0,
+	C_MAX_OPTS,
+};
+
 enum {
 	D_AGCOUNT = 0,
 	D_FILE,
@@ -236,6 +242,28 @@ static struct opt_params bopts = {
 	},
 };
 
+/*
+ * Config file specification. Usage is:
+ *
+ * mkfs.xfs -c file=<name>
+ *
+ * A subopt is used for the filename so in future we can extend the behaviour
+ * of the config file (e.g. specified defaults rather than options) if we ever
+ * have a need to do that sort of thing.
+ */
+static struct opt_params copts = {
+	.name = 'c',
+	.subopts = {
+		[C_FILE] = "file",
+	},
+	.subopt_params = {
+		{ .index = C_FILE,
+		  .conflicts = { { NULL, LAST_CONFLICT } },
+		  .defaultval = SUBOPT_NEEDS_VAL,
+		},
+	},
+};
+
 static struct opt_params dopts = {
 	.name = 'd',
 	.subopts = {
@@ -740,6 +768,8 @@ struct cli_params {
 	int	sectorsize;
 	int	blocksize;
 
+	char	*cfgfile;
+
 	/* parameters that depend on sector/block size being validated. */
 	char	*dsize;
 	char	*agsize;
@@ -854,6 +884,7 @@ usage( void )
 {
 	fprintf(stderr, _("Usage: %s\n\
 /* blocksize */		[-b size=num]\n\
+/* config file */	[-c file=xxx]\n\
 /* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx,rmapbt=0|1,reflink=0|1]\n\
 /* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,\n\
 			    (sunit=value,swidth=value|su=num,sw=num|noalign),\n\
@@ -1377,6 +1408,23 @@ block_opts_parser(
 	return 0;
 }
 
+static int
+cfgfile_opts_parser(
+	struct opt_params	*opts,
+	int			subopt,
+	char			*value,
+	struct cli_params	*cli)
+{
+	switch (subopt) {
+	case C_FILE:
+		cli->cfgfile = getstr(value, opts, subopt);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int
 data_opts_parser(
 	struct opt_params	*opts,
@@ -1642,6 +1690,7 @@ static struct subopts {
 				  struct cli_params	*cli);
 } subopt_tab[] = {
 	{ 'b', &bopts, block_opts_parser },
+	{ 'c', &copts, cfgfile_opts_parser },
 	{ 'd', &dopts, data_opts_parser },
 	{ 'i', &iopts, inode_opts_parser },
 	{ 'l', &lopts, log_opts_parser },
@@ -3552,6 +3601,47 @@ check_root_ino(
 	}
 }
 
+/*
+ * INI file format option parser.
+ *
+ * This is called by the file parser library for every valid option it finds in
+ * the config file. The option is already broken down into a
+ * {section,name,value} tuple, so all we need to do is feed it to the correct
+ * suboption parser function and translate the return value.
+ *
+ * Returns 0 on failure, 1 for success.
+ */
+static int
+cfgfile_parse_ini(
+	void			*user,
+	const char		*section,
+	const char		*name,
+	const char		*value)
+{
+	struct cli_params	*cli = user;
+
+	fprintf(stderr, "Ini debug: file %s, section %s, name %s, value %s\n",
+		cli->cfgfile, section, name, value);
+
+	return 1;
+}
+
+void
+cfgfile_parse(
+	struct cli_params	*cli)
+{
+	if (!cli->cfgfile)
+		return;
+
+	if (ini_parse(cli->cfgfile, cfgfile_parse_ini, cli) < 0) {
+		fprintf(stderr, _("Error parsing config file %s. Aborting\n"),
+			cli->cfgfile);
+		exit(1);
+	}
+	printf(_("Parameters parsed from config file %s successfully\n"),
+		cli->cfgfile);
+}
+
 int
 main(
 	int			argc,
@@ -3638,13 +3728,14 @@ main(
 	memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
 	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
 		switch (c) {
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
 			break;
 		case 'b':
+		case 'c':
 		case 'd':
 		case 'i':
 		case 'l':
@@ -3688,6 +3779,14 @@ main(
 	} else
 		dfile = xi.dname;
 
+	/*
+	 * Now we have all the options parsed, we can read in the option file
+	 * specified on the command line via "-c file=xxx". Once we have all the
+	 * options from this file parsed, we can then proceed with parameter
+	 * and bounds checking and making the filesystem.
+	 */
+	cfgfile_parse(&cli);
+
 	protostring = setup_proto(protofile);
 
 	/*
-- 
2.28.0

[PATCH 3/3] mkfs: hook up suboption parsing to ini files

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now we have the config file parsing hooked up and feeding in
parameters to mkfs, wire the parameters up to the existing CLI
option parsing functions. THis gives the config file exactly the
same capabilities and constraints as the command line option
specification.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 include/linux.h |   2 +-
 mkfs/xfs_mkfs.c | 121 +++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 95 insertions(+), 28 deletions(-)

diff --git a/include/linux.h b/include/linux.h
index 57726bb12b74..03b3278bb895 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -92,7 +92,7 @@ static __inline__ void platform_uuid_unparse(uuid_t *uu, char *buffer)
 	uuid_unparse(*uu, buffer);
 }
 
-static __inline__ int platform_uuid_parse(char *buffer, uuid_t *uu)
+static __inline__ int platform_uuid_parse(const char *buffer, uuid_t *uu)
 {
 	return uuid_parse(buffer, *uu);
 }
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 6a373d614a56..deaed551b6d1 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -142,6 +142,13 @@ enum {
  * name MANDATORY
  *   Name is a single char, e.g., for '-d file', name is 'd'.
  *
+ * ini_name MANDATORY
+ *   Name is a string, not longer than MAX_INI_NAME_LEN, that is used as the
+ *   section name for this option set in INI format config files. The only
+ *   option set this is not required for is the command line config file
+ *   specification options, everything else must be configurable via config
+ *   files.
+ *
  * subopts MANDATORY
  *   Subopts is a list of strings naming suboptions. In the example above,
  *   it would contain "file". The last entry of this list has to be NULL.
@@ -200,6 +207,8 @@ enum {
  */
 struct opt_params {
 	const char	name;
+#define MAX_INI_NAME_LEN	32
+	const char	ini_name[MAX_INI_NAME_LEN];
 	const char	*subopts[MAX_SUBOPTS];
 
 	struct subopt_param {
@@ -227,6 +236,7 @@ static struct opt_params sopts;
 
 static struct opt_params bopts = {
 	.name = 'b',
+	.ini_name = "block",
 	.subopts = {
 		[B_SIZE] = "size",
 	},
@@ -266,6 +276,7 @@ static struct opt_params copts = {
 
 static struct opt_params dopts = {
 	.name = 'd',
+	.ini_name = "data",
 	.subopts = {
 		[D_AGCOUNT] = "agcount",
 		[D_FILE] = "file",
@@ -403,6 +414,7 @@ static struct opt_params dopts = {
 
 static struct opt_params iopts = {
 	.name = 'i',
+	.ini_name = "inode",
 	.subopts = {
 		[I_ALIGN] = "align",
 		[I_MAXPCT] = "maxpct",
@@ -464,6 +476,7 @@ static struct opt_params iopts = {
 
 static struct opt_params lopts = {
 	.name = 'l',
+	.ini_name = "log",
 	.subopts = {
 		[L_AGNUM] = "agnum",
 		[L_INTERNAL] = "internal",
@@ -563,6 +576,7 @@ static struct opt_params lopts = {
 
 static struct opt_params nopts = {
 	.name = 'n',
+	.ini_name = "naming",
 	.subopts = {
 		[N_SIZE] = "size",
 		[N_VERSION] = "version",
@@ -594,6 +608,7 @@ static struct opt_params nopts = {
 
 static struct opt_params ropts = {
 	.name = 'r',
+	.ini_name = "realtime",
 	.subopts = {
 		[R_EXTSIZE] = "extsize",
 		[R_SIZE] = "size",
@@ -644,6 +659,7 @@ static struct opt_params ropts = {
 
 static struct opt_params sopts = {
 	.name = 's',
+	.ini_name = "sector",
 	.subopts = {
 		[S_SIZE] = "size",
 		[S_SECTSIZE] = "sectsize",
@@ -674,6 +690,7 @@ static struct opt_params sopts = {
 
 static struct opt_params mopts = {
 	.name = 'm',
+	.ini_name = "metadata",
 	.subopts = {
 		[M_CRC] = "crc",
 		[M_FINOBT] = "finobt",
@@ -967,13 +984,24 @@ respec(
 
 static void
 unknown(
-	char		opt,
-	char		*s)
+	const char	opt,
+	const char	*s)
 {
 	fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
 	usage();
 }
 
+static void
+unknown_cfgfile_opt(
+	const char	*section,
+	const char	*name,
+	const char	*value)
+{
+	fprintf(stderr, _("unknown config file option: [%s]:%s=%s\n"),
+		section, name, value);
+	usage();
+}
+
 static void
 check_device_type(
 	const char	*name,
@@ -1379,7 +1407,7 @@ getnum(
  */
 static char *
 getstr(
-	char			*str,
+	const char		*str,
 	struct opt_params	*opts,
 	int			index)
 {
@@ -1388,14 +1416,14 @@ getstr(
 	/* empty strings for string options are not valid */
 	if (!str || *str == '\0')
 		reqval(opts->name, opts->subopts, index);
-	return str;
+	return (char *)str;
 }
 
 static int
 block_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1412,7 +1440,7 @@ static int
 cfgfile_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1429,7 +1457,7 @@ static int
 data_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1492,7 +1520,7 @@ static int
 inode_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1527,7 +1555,7 @@ static int
 log_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1573,7 +1601,7 @@ static int
 meta_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1607,7 +1635,7 @@ static int
 naming_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1636,7 +1664,7 @@ static int
 rtdev_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1666,7 +1694,7 @@ static int
 sector_opts_parser(
 	struct opt_params	*opts,
 	int			subopt,
-	char			*value,
+	const char		*value,
 	struct cli_params	*cli)
 {
 	switch (subopt) {
@@ -1682,23 +1710,22 @@ sector_opts_parser(
 }
 
 static struct subopts {
-	char		opt;
 	struct opt_params *opts;
 	int		(*parser)(struct opt_params	*opts,
 				  int			subopt,
-				  char			*value,
+				  const char		*value,
 				  struct cli_params	*cli);
 } subopt_tab[] = {
-	{ 'b', &bopts, block_opts_parser },
-	{ 'c', &copts, cfgfile_opts_parser },
-	{ 'd', &dopts, data_opts_parser },
-	{ 'i', &iopts, inode_opts_parser },
-	{ 'l', &lopts, log_opts_parser },
-	{ 'm', &mopts, meta_opts_parser },
-	{ 'n', &nopts, naming_opts_parser },
-	{ 'r', &ropts, rtdev_opts_parser },
-	{ 's', &sopts, sector_opts_parser },
-	{ '\0', NULL, NULL },
+	{ &bopts, block_opts_parser },
+	{ &copts, cfgfile_opts_parser },
+	{ &dopts, data_opts_parser },
+	{ &iopts, inode_opts_parser },
+	{ &lopts, log_opts_parser },
+	{ &mopts, meta_opts_parser },
+	{ &nopts, naming_opts_parser },
+	{ &ropts, rtdev_opts_parser },
+	{ &sopts, sector_opts_parser },
+	{ NULL, NULL },
 };
 
 static void
@@ -1712,12 +1739,12 @@ parse_subopts(
 	int		ret = 0;
 
 	while (sop->opts) {
-		if (sop->opt == opt)
+		if (opt && sop->opts->name == opt)
 			break;
 		sop++;
 	}
 
-	/* should never happen */
+	/* Should not happen */
 	if (!sop->opts)
 		return;
 
@@ -1735,6 +1762,44 @@ parse_subopts(
 	}
 }
 
+static void
+parse_cfgopt(
+	const char	*section,
+	const char	*name,
+	const char	*value,
+	struct cli_params *cli)
+{
+	struct subopts	*sop = &subopt_tab[0];
+	char		**subopts;
+	int		ret = 0;
+	int		i;
+
+	while (sop->opts) {
+		if (sop->opts->ini_name[0] != '\0' &&
+		    strcasecmp(section, sop->opts->ini_name) == 0)
+			break;
+		sop++;
+	}
+
+	/* Config files with unknown sections get caught here. */
+	if (!sop->opts)
+		goto unknown_opt;
+
+	subopts = (char **)sop->opts->subopts;
+	for (i = 0; i < MAX_SUBOPTS; i++) {
+		if (!subopts[i])
+			break;
+		if (strcasecmp(name, subopts[i]) == 0) {
+			ret = (sop->parser)(sop->opts, i, value, cli);
+			if (ret)
+				goto unknown_opt;
+			return;
+		}
+	}
+unknown_opt:
+	unknown_cfgfile_opt(section, name, value);
+}
+
 static void
 validate_sectorsize(
 	struct mkfs_params	*cfg,
@@ -3623,6 +3688,8 @@ cfgfile_parse_ini(
 	fprintf(stderr, "Ini debug: file %s, section %s, name %s, value %s\n",
 		cli->cfgfile, section, name, value);
 
+	parse_cfgopt(section, name, value, cli);
+
 	return 1;
 }
 
-- 
2.28.0

Re: [PATCH 1/3] build: add support for libinih for mkfs

[Eric Sandeen]

On 8/25/20 8:56 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Need to make sure the library is present so we can build mkfs with
> config file support.

Can you add https://github.com/benhoyt/inih to doc/INSTALL as a
dependency?

(that's probably pretty out of date now anyway but it seems worth
documenting any new requirement)

-Eric

Re: [PATCH 2/3] mkfs: add initial ini format config file parsing support

[Eric Sandeen]

On 8/25/20 8:56 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Add the framework that will allow the config file to be supplied on
> the CLI and passed to the library that will parse it. This does not
> yet do any option parsing from the config file.

so we have "-c $SUBOPT=file"

From what I read in the cover letter, and from checking in IRC it seems
like you envision the ability to also specify defaults from a config file
in the future; to that end it might be better to name this $SUBOPT
"options=" instead of "file=" as the latter is very generic.

Then in the future, we could have one or both of :

-c defaults=file1 -c options=file2

i.e. configure the defaults, then configure the options

I guess this is just RFC but you want probably to drop the "Ini debug:"
printf eventually.

This will need a man page update, of course.

I think it should explain where "file" will be looked for; I assume it
is either a full path, or a relative path to the current directory.

(In the future it would be nice to have mkfs.xfs search somewhere
under /etc for these files as well, but I'm not bikeshedding!)

-Eric

Re: [PATCH 1/3] build: add support for libinih for mkfs

[Dave Chinner]

On Wed, Aug 26, 2020 at 04:51:50PM -0500, Eric Sandeen wrote:
> On 8/25/20 8:56 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Need to make sure the library is present so we can build mkfs with
> > config file support.
> 
> Can you add https://github.com/benhoyt/inih to doc/INSTALL as a
> dependency?

Distros should be shipping that library, and configure asks you to
install it if it isn't present, just like it does for all the other
libraries xfsprogs depends on. We want users to install distro
libraries, not have to build dependencies from source and install
them...

> (that's probably pretty out of date now anyway but it seems worth
> documenting any new requirement)

Yeah, we need a lot more than just e2fsprogs-devel and UUIDs these
days. I'd prefer that doc/INSTALL just says "the configure script
will prompt you to install missing build dependencies" rather than
iterate them here and have the list be perpetually incomplete....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] mkfs: add initial ini format config file parsing support

[Dave Chinner]

On Wed, Aug 26, 2020 at 04:56:34PM -0500, Eric Sandeen wrote:
> On 8/25/20 8:56 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add the framework that will allow the config file to be supplied on
> > the CLI and passed to the library that will parse it. This does not
> > yet do any option parsing from the config file.
> 
> so we have "-c $SUBOPT=file"
> 
> From what I read in the cover letter, and from checking in IRC it seems
> like you envision the ability to also specify defaults from a config file
> in the future; to that end it might be better to name this $SUBOPT
> "options=" instead of "file=" as the latter is very generic.
> 
> Then in the future, we could have one or both of :
> 
> -c defaults=file1 -c options=file2
> 
> i.e. configure the defaults, then configure the options

Yup, makes sense. Will change.

> I guess this is just RFC but you want probably to drop the "Ini debug:"
> printf eventually.

Yeah, I've already removed that so I can run fstests....

> This will need a man page update, of course.

Eventually, yes :P

> I think it should explain where "file" will be looked for; I assume it
> is either a full path, or a relative path to the current directory.

It will work with either, just like all the other "file" parameters
passed to mkfs....

> (In the future it would be nice to have mkfs.xfs search somewhere
> under /etc for these files as well, but I'm not bikeshedding!)

Nope, I'm not doing that. Go away. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Eric Sandeen]

On 8/25/20 8:56 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Now we have the config file parsing hooked up and feeding in
> parameters to mkfs, wire the parameters up to the existing CLI
> option parsing functions. THis gives the config file exactly the
> same capabilities and constraints as the command line option
> specification.

And as such, as you already mentioned, respecifications on the command
line will fail.  That can be documented in the man page :)

The section names will need to be documented too.

(In a very much not-bikeshedding way, we could consider [b] rather than
[block] so you can cover it with "the section names match the option
characters, i.e. for -b one would use section name [b]" but I really don't
care and will not mention this again because as long as it's documented
it's fine.)

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  include/linux.h |   2 +-
>  mkfs/xfs_mkfs.c | 121 +++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 95 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux.h b/include/linux.h
> index 57726bb12b74..03b3278bb895 100644
> --- a/include/linux.h
> +++ b/include/linux.h
> @@ -92,7 +92,7 @@ static __inline__ void platform_uuid_unparse(uuid_t *uu, char *buffer)
>  	uuid_unparse(*uu, buffer);
>  }
>  
> -static __inline__ int platform_uuid_parse(char *buffer, uuid_t *uu)
> +static __inline__ int platform_uuid_parse(const char *buffer, uuid_t *uu)
>  {
>  	return uuid_parse(buffer, *uu);
>  }
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6a373d614a56..deaed551b6d1 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -142,6 +142,13 @@ enum {
>   * name MANDATORY
>   *   Name is a single char, e.g., for '-d file', name is 'd'.
>   *
> + * ini_name MANDATORY
> + *   Name is a string, not longer than MAX_INI_NAME_LEN, that is used as the
> + *   section name for this option set in INI format config files. The only
> + *   option set this is not required for is the command line config file
> + *   specification options, everything else must be configurable via config
> + *   files.

Nothing actually enforces this MANDATORY, right, this is just documentation.
So the fact that struct opt_params copts doesn't have it, is fine.

> @@ -967,13 +984,24 @@ respec(
>  
>  static void
>  unknown(
> -	char		opt,
> -	char		*s)
> +	const char	opt,
> +	const char	*s)

(can all of the constification could maybe go in its own patch just to cut
down on the patch doomscrolling or does it have to go with the other changes?)

>  {
>  	fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
>  	usage();
>  }
>  
> +static void
> +unknown_cfgfile_opt(
> +	const char	*section,
> +	const char	*name,
> +	const char	*value)
> +{
> +	fprintf(stderr, _("unknown config file option: [%s]:%s=%s\n"),
> +		section, name, value);

If we allow more than one -c subopt in the future we might want to print
the filename. Wouldn't /hurt/ to do so now, or just remember to do it if
we ever add a 2nd -c subopt.

> +	usage();
> +}
> +
>  static void
>  check_device_type(
>  	const char	*name,
> @@ -1379,7 +1407,7 @@ getnum(
>   */
>  static char *
>  getstr(
> -	char			*str,
> +	const char		*str,
>  	struct opt_params	*opts,
>  	int			index)
>  {
> @@ -1388,14 +1416,14 @@ getstr(
>  	/* empty strings for string options are not valid */
>  	if (!str || *str == '\0')
>  		reqval(opts->name, opts->subopts, index);
> -	return str;
> +	return (char *)str;

(what's the cast for?)


> @@ -1682,23 +1710,22 @@ sector_opts_parser(
>  }
>  
>  static struct subopts {
> -	char		opt;
>  	struct opt_params *opts;
>  	int		(*parser)(struct opt_params	*opts,
>  				  int			subopt,
> -				  char			*value,
> +				  const char		*value,
>  				  struct cli_params	*cli);
>  } subopt_tab[] = {
> -	{ 'b', &bopts, block_opts_parser },
> -	{ 'c', &copts, cfgfile_opts_parser },
> -	{ 'd', &dopts, data_opts_parser },
> -	{ 'i', &iopts, inode_opts_parser },
> -	{ 'l', &lopts, log_opts_parser },
> -	{ 'm', &mopts, meta_opts_parser },
> -	{ 'n', &nopts, naming_opts_parser },
> -	{ 'r', &ropts, rtdev_opts_parser },
> -	{ 's', &sopts, sector_opts_parser },
> -	{ '\0', NULL, NULL },
> +	{ &bopts, block_opts_parser },
> +	{ &copts, cfgfile_opts_parser },
> +	{ &dopts, data_opts_parser },
> +	{ &iopts, inode_opts_parser },
> +	{ &lopts, log_opts_parser },
> +	{ &mopts, meta_opts_parser },
> +	{ &nopts, naming_opts_parser },
> +	{ &ropts, rtdev_opts_parser },
> +	{ &sopts, sector_opts_parser },
> +	{ NULL, NULL },
>  };
>  
>  static void
> @@ -1712,12 +1739,12 @@ parse_subopts(
>  	int		ret = 0;
>  
>  	while (sop->opts) {
> -		if (sop->opt == opt)
> +		if (opt && sop->opts->name == opt)
>  			break;
>  		sop++;
>  	}
>  
> -	/* should never happen */
> +	/* Should not happen */

ok? :)

Overall this seems remarkably tidy, thanks.

-Eric

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

[Dave Chinner]

On Wed, Aug 26, 2020 at 05:21:13PM -0500, Eric Sandeen wrote:
> On 8/25/20 8:56 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Now we have the config file parsing hooked up and feeding in
> > parameters to mkfs, wire the parameters up to the existing CLI
> > option parsing functions. THis gives the config file exactly the
> > same capabilities and constraints as the command line option
> > specification.
> 
> And as such, as you already mentioned, respecifications on the command
> line will fail.  That can be documented in the man page :)
> 
> The section names will need to be documented too.
> 
> (In a very much not-bikeshedding way, we could consider [b] rather than
> [block] so you can cover it with "the section names match the option
> characters, i.e. for -b one would use section name [b]" but I really don't
> care and will not mention this again because as long as it's documented
> it's fine.)

No, I want the ini file to be human readable. The other thing to
consider here is that this is the equivalent of the long option
CLI parameter definition (i.e. -b size=foo vs --block="size=foo").
They'll get documented in the man page....

> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  include/linux.h |   2 +-
> >  mkfs/xfs_mkfs.c | 121 +++++++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 95 insertions(+), 28 deletions(-)
> > 
> > diff --git a/include/linux.h b/include/linux.h
> > index 57726bb12b74..03b3278bb895 100644
> > --- a/include/linux.h
> > +++ b/include/linux.h
> > @@ -92,7 +92,7 @@ static __inline__ void platform_uuid_unparse(uuid_t *uu, char *buffer)
> >  	uuid_unparse(*uu, buffer);
> >  }
> >  
> > -static __inline__ int platform_uuid_parse(char *buffer, uuid_t *uu)
> > +static __inline__ int platform_uuid_parse(const char *buffer, uuid_t *uu)
> >  {
> >  	return uuid_parse(buffer, *uu);
> >  }
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 6a373d614a56..deaed551b6d1 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -142,6 +142,13 @@ enum {
> >   * name MANDATORY
> >   *   Name is a single char, e.g., for '-d file', name is 'd'.
> >   *
> > + * ini_name MANDATORY
> > + *   Name is a string, not longer than MAX_INI_NAME_LEN, that is used as the
> > + *   section name for this option set in INI format config files. The only
> > + *   option set this is not required for is the command line config file
> > + *   specification options, everything else must be configurable via config
> > + *   files.
> 
> Nothing actually enforces this MANDATORY, right, this is just documentation.
> So the fact that struct opt_params copts doesn't have it, is fine.
> 
> > @@ -967,13 +984,24 @@ respec(
> >  
> >  static void
> >  unknown(
> > -	char		opt,
> > -	char		*s)
> > +	const char	opt,
> > +	const char	*s)
> 
> (can all of the constification could maybe go in its own patch just to cut
> down on the patch doomscrolling or does it have to go with the other changes?)

It is a result of the ini parser callback using "const char *" for
it's variables. I just whack-a-moled it to get it compile.

> >  {
> >  	fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
> >  	usage();
> >  }
> >  
> > +static void
> > +unknown_cfgfile_opt(
> > +	const char	*section,
> > +	const char	*name,
> > +	const char	*value)
> > +{
> > +	fprintf(stderr, _("unknown config file option: [%s]:%s=%s\n"),
> > +		section, name, value);
> 
> If we allow more than one -c subopt in the future we might want to print
> the filename. Wouldn't /hurt/ to do so now, or just remember to do it if
> we ever add a 2nd -c subopt.

OK.

> 
> > +	usage();
> > +}
> > +
> >  static void
> >  check_device_type(
> >  	const char	*name,
> > @@ -1379,7 +1407,7 @@ getnum(
> >   */
> >  static char *
> >  getstr(
> > -	char			*str,
> > +	const char		*str,
> >  	struct opt_params	*opts,
> >  	int			index)
> >  {
> > @@ -1388,14 +1416,14 @@ getstr(
> >  	/* empty strings for string options are not valid */
> >  	if (!str || *str == '\0')
> >  		reqval(opts->name, opts->subopts, index);
> > -	return str;
> > +	return (char *)str;
> 
> (what's the cast for?)

Because I got tired of whack-a-mole and these returned strings are
stored in the cli parameter structure and I didn't feel like chasing
the const rabbit down that hole.

i.e. the const on the input parameter goes as far as the input
parsing to verify it runs, and the verified input string that is
returned and stored is no longer const. Perhaps it would be better
to just strdup the string here and return that instead?

> > @@ -1682,23 +1710,22 @@ sector_opts_parser(
> >  }
> >  
> >  static struct subopts {
> > -	char		opt;
> >  	struct opt_params *opts;
> >  	int		(*parser)(struct opt_params	*opts,
> >  				  int			subopt,
> > -				  char			*value,
> > +				  const char		*value,
> >  				  struct cli_params	*cli);
> >  } subopt_tab[] = {
> > -	{ 'b', &bopts, block_opts_parser },
> > -	{ 'c', &copts, cfgfile_opts_parser },
> > -	{ 'd', &dopts, data_opts_parser },
> > -	{ 'i', &iopts, inode_opts_parser },
> > -	{ 'l', &lopts, log_opts_parser },
> > -	{ 'm', &mopts, meta_opts_parser },
> > -	{ 'n', &nopts, naming_opts_parser },
> > -	{ 'r', &ropts, rtdev_opts_parser },
> > -	{ 's', &sopts, sector_opts_parser },
> > -	{ '\0', NULL, NULL },
> > +	{ &bopts, block_opts_parser },
> > +	{ &copts, cfgfile_opts_parser },
> > +	{ &dopts, data_opts_parser },
> > +	{ &iopts, inode_opts_parser },
> > +	{ &lopts, log_opts_parser },
> > +	{ &mopts, meta_opts_parser },
> > +	{ &nopts, naming_opts_parser },
> > +	{ &ropts, rtdev_opts_parser },
> > +	{ &sopts, sector_opts_parser },
> > +	{ NULL, NULL },
> >  };
> >  
> >  static void
> > @@ -1712,12 +1739,12 @@ parse_subopts(
> >  	int		ret = 0;
> >  
> >  	while (sop->opts) {
> > -		if (sop->opt == opt)
> > +		if (opt && sop->opts->name == opt)
> >  			break;
> >  		sop++;
> >  	}
> >  
> > -	/* should never happen */
> > +	/* Should not happen */
> 
> ok? :)

Ah, I modified this function first, then realised that it would be
better to keep the subopt table iteration separate because we didn't
need to use getsubopt() to separate "name=value" strings. I'll clean
that up.

> Overall this seems remarkably tidy, thanks.

I've been saying config file parsing is simple and easy to fit into
the existing option parsing infrastructure for a long time. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com