All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support
@ 2018-06-12  0:30 Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 1/5] mkfs: distinguish between struct sb_feat_args and struct cli_params Luis R. Rodriguez
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

This is v6 series addresses the feedback from the last v5 series. The
biggest change is for dumping the opterr reset onto the shared helper
platform_getoptreset(), and addressing the TOCTOU race between stat-ing
and opening. The rest of the changes are all things which were mentioned
during review, the details listed below.

You can also get the code from my kernel.org xfsprogs-dev 20180611-own-parser-v6
branch [0].

Reviews, questions, or rants are greatly appreciated.

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

Changes on v6:

- Use -1 return value consistently when returning
- Fix use of one errno use
- Remove &confopts_tab[i] check which can never happen
- Fixed uninitialized return value use on parse_config_stream()
- Remove extra tag declaration on parse_config_stream()
- Address the TOCTOU race
- Move opterr to shorten the first parameter parsing on main(). We now
  just use platform_getoptreset() to reset the settings so we can parse
  more.

Changes on v5:

- We no force sysconfdir=/etc if not set so we also drop the last patch
  for debian as its no longer needed

- We use config_check_bool() as during review on the last series we
  failed to pick up on the fact that we were *not* detecting if a value
  for a field was greater than 1, so we were allowing for instance crc=1
  although this was silently mapping to crc=1. Note that although this
  issue was present, the check for conflicts is still in effect and
  functional. The only reason the issue with bools existed is we cast
  down the uint64_t to bool and that resolves the issue. For now all
  subuopt parsers use config_check_bool(). When and if other values are
  introduced they can change this.

- Use fstat() and openat() as requested instead of the odd checks for
  first and second characters. This feels nicer.

- Use negative return values and fixate errno in better locations.

- Drop the man pages and yield to Sandeed for that.

- Re-introduce the source type enum

- Dealing with a config file is now split into an open call and a
  separate parse file. This does read much better now.

- Fix a missing free(line) on the switch for parsing when we don't have
  an error.

- Adjust random error messages to reflect a bit better what is issue
  creeped up.

Changes on v4:

- Tons of man page documentation enhancements from Darrick.

- Adjusted a few error code messages/ paths to make more sense as per
  Darrick's feedback.

Changes on v3:

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

- Use uint64_t throughout all parsers as requested.

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

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

- Use getline() for fetching lines as requested

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

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

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

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

- Add respecification checks for a section.

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

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

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

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

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

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

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

Changes on v2:

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

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

- Start to split files off

- Duplicate a bit of items as suggested at LSFMM for the configuration
  parser structures. We can later consolidate if we think its really
  needed, however we want the freedom to change these as we see fit and
  most importantly keep the code apart.

- Conflict resolution and validation is managed now by piggy backing off
  of the idea of using the defaults to instantiate CLI parameters. CLI
  always overrides.

Luis R. Rodriguez (5):
  mkfs: distinguish between struct sb_feat_args and struct cli_params
  mkfs: move shared config structs and into their own headers
  mkfs: replace defaults source with an enum
  xfsprogs: reset opterr on platform_getoptreset()
  mkfs.xfs: add configuration file parsing support using our own parser

 configure.ac          |   1 +
 include/builddefs.in  |   2 +
 include/darwin.h      |   1 +
 include/gnukfreebsd.h |   1 +
 include/linux.h       |   1 +
 mkfs/Makefile         |   2 +-
 mkfs/config.c         | 670 ++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/config.h         | 121 +++++++++
 mkfs/xfs_mkfs.c       | 127 +++++-----
 9 files changed, 865 insertions(+), 61 deletions(-)
 create mode 100644 mkfs/config.c
 create mode 100644 mkfs/config.h

-- 
2.16.3


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v6 1/5] mkfs: distinguish between struct sb_feat_args and struct cli_params
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
@ 2018-06-12  0:30 ` Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 2/5] mkfs: move shared config structs and into their own headers Luis R. Rodriguez
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

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

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

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v6 2/5] mkfs: move shared config structs and into their own headers
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 1/5] mkfs: distinguish between struct sb_feat_args and struct cli_params Luis R. Rodriguez
@ 2018-06-12  0:30 ` Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 3/5] mkfs: replace defaults source with an enum Luis R. Rodriguez
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

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

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

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

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

diff --git a/mkfs/config.h b/mkfs/config.h
new file mode 100644
index 000000000000..e5ea968e2d65
--- /dev/null
+++ b/mkfs/config.h
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * Copyright (c) 2016-2017 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+#ifndef _XFS_MKFS_CONFIG_H
+#define _XFS_MKFS_CONFIG_H
+
+struct fsxattr;
+
+/*
+ * Shared superblock configuration options
+ *
+ * These options provide shared configuration tunables for the filesystem
+ * superblock. There are three possible sources for these options set, each
+ * source can overriding the later source:
+ *
+ * 	o built-in defaults
+ * 	o configuration file (XXX)
+ * 	o command line
+ *
+ * These values are not used directly - they are inputs into the mkfs geometry
+ * validation.
+ */
+struct sb_feat_args {
+	int	log_version;
+	int	attr_version;
+	int	dir_version;
+	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
+	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
+	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
+	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
+	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
+	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
+	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
+	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
+	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
+	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
+	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
+	bool	nodalign;
+	bool	nortalign;
+};
+
+/*
+ * Default filesystem features and configuration values
+ *
+ * This structure contains the default mkfs values that are to be used when
+ * a user does not specify the option on the command line. We do not use these
+ * values directly - they are inputs to the mkfs geometry validation and
+ * calculations.
+ */
+struct mkfs_default_params {
+	char	*source;	/* where the defaults came from */
+
+	int	sectorsize;
+	int	blocksize;
+
+	/* feature flags that are set */
+	struct sb_feat_args	sb_feat;
+
+	/* root inode characteristics */
+	struct fsxattr		fsx;
+};
+
+#endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 227613ef77d5..56e869d666be 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -21,6 +21,7 @@
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
 #include "fsgeom.h"
+#include "config.h"
 
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
@@ -706,39 +707,6 @@ cli_opt_set(
 	       opts->subopt_params[subopt].str_seen;
 }
 
-/*
- * Shared superblock configuration options
- *
- * These options provide shared configuration tunables for the filesystem
- * superblock. There are three possible sources for these options set, each
- * source can overriding the later source:
- *
- * 	o built-in defaults
- * 	o configuration file (XXX)
- * 	o command line
- *
- * These values are not used directly - they are inputs into the mkfs geometry
- * validation.
- */
-struct sb_feat_args {
-	int	log_version;
-	int	attr_version;
-	int	dir_version;
-	bool	inode_align;		/* XFS_SB_VERSION_ALIGNBIT */
-	bool	nci;			/* XFS_SB_VERSION_BORGBIT */
-	bool	lazy_sb_counters;	/* XFS_SB_VERSION2_LAZYSBCOUNTBIT */
-	bool	parent_pointers;	/* XFS_SB_VERSION2_PARENTBIT */
-	bool	projid32bit;		/* XFS_SB_VERSION2_PROJID32BIT */
-	bool	crcs_enabled;		/* XFS_SB_VERSION2_CRCBIT */
-	bool	dirftype;		/* XFS_SB_VERSION2_FTYPE */
-	bool	finobt;			/* XFS_SB_FEAT_RO_COMPAT_FINOBT */
-	bool	spinodes;		/* XFS_SB_FEAT_INCOMPAT_SPINODES */
-	bool	rmapbt;			/* XFS_SB_FEAT_RO_COMPAT_RMAPBT */
-	bool	reflink;		/* XFS_SB_FEAT_RO_COMPAT_REFLINK */
-	bool	nodalign;
-	bool	nortalign;
-};
-
 /*
  * Options configured on the command line.
  *
@@ -850,27 +818,6 @@ struct mkfs_params {
 	struct sb_feat_args	sb_feat;
 };
 
-/*
- * Default filesystem features and configuration values
- *
- * This structure contains the default mkfs values that are to be used when
- * a user does not specify the option on the command line. We do not use these
- * values directly - they are inputs to the mkfs geometry validation and
- * calculations.
- */
-struct mkfs_default_params {
-	char	*source;	/* where the defaults came from */
-
-	int	sectorsize;
-	int	blocksize;
-
-	/* feature flags that are set */
-	struct sb_feat_args	sb_feat;
-
-	/* root inode characteristics */
-	struct fsxattr		fsx;
-};
-
 static void __attribute__((noreturn))
 usage( void )
 {
-- 
2.16.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v6 3/5] mkfs: replace defaults source with an enum
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 1/5] mkfs: distinguish between struct sb_feat_args and struct cli_params Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 2/5] mkfs: move shared config structs and into their own headers Luis R. Rodriguez
@ 2018-06-12  0:30 ` Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 4/5] xfsprogs: reset opterr on platform_getoptreset() Luis R. Rodriguez
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

Using an enum will let us later just use a switch statement to print
out the source, this makes sources easier to document, update and
manage.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 mkfs/config.h   | 22 +++++++++++++++++++++-
 mkfs/xfs_mkfs.c |  5 +++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/mkfs/config.h b/mkfs/config.h
index e5ea968e2d65..a3c6c99c3064 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -54,6 +54,17 @@ struct sb_feat_args {
 	bool	nortalign;
 };
 
+/*
+ * File configuration type settings
+ *
+ * These are the different possibilities by which you can end up parsing
+ * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
+ * file parsed and we are using the built-in defaults on this code.
+ */
+enum default_params_type {
+	DEFAULTS_BUILTIN = 0,
+};
+
 /*
  * Default filesystem features and configuration values
  *
@@ -63,7 +74,7 @@ struct sb_feat_args {
  * calculations.
  */
 struct mkfs_default_params {
-	char	*source;	/* where the defaults came from */
+	enum default_params_type type; /* where the defaults came from */
 
 	int	sectorsize;
 	int	blocksize;
@@ -75,4 +86,13 @@ struct mkfs_default_params {
 	struct fsxattr		fsx;
 };
 
+static inline const char *default_type_str(enum default_params_type type)
+{
+	switch (type) {
+	case DEFAULTS_BUILTIN:
+		return _("package built-in definitions");
+	}
+	return _("Unkown\n");
+}
+
 #endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 56e869d666be..2e344aa9dad1 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3722,7 +3722,7 @@ main(
 
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
-		.source = _("package build definitions"),
+		.type = DEFAULTS_BUILTIN,
 		.sectorsize = XFS_MIN_SECTORSIZE,
 		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
 		.sb_feat = {
@@ -3762,7 +3762,8 @@ main(
 	 * implemented, emit a message to indicate where the defaults being
 	 * used came from.
 	 *
-	 * printf(_("Default configuration sourced from %s\n"), dft.source);
+	 * printf(_("Default configuration sourced from %s\n"),
+	 *	  default_type_str(dft.type));
 	 */
 
 	/* copy new defaults into CLI parsing structure */
-- 
2.16.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v6 4/5] xfsprogs: reset opterr on platform_getoptreset()
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
                   ` (2 preceding siblings ...)
  2018-06-12  0:30 ` [PATCH v6 3/5] mkfs: replace defaults source with an enum Luis R. Rodriguez
@ 2018-06-12  0:30 ` Luis R. Rodriguez
  2018-06-12  0:30 ` [PATCH v6 5/5] mkfs.xfs: add configuration file parsing support using our own parser Luis R. Rodriguez
  2018-06-12 16:46 ` [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Eric Sandeen
  5 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

Setting opterr = 0 is useful if you want to only parse a subset of
paremeters first. Later you will have to set opterr = 1 again, but
we already have a platform_getoptreset() and its expected to be used
in these contexts so just sprinkle it there.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 include/darwin.h      | 1 +
 include/gnukfreebsd.h | 1 +
 include/linux.h       | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/darwin.h b/include/darwin.h
index 2632e1d6f08c..4c23a1a3f739 100644
--- a/include/darwin.h
+++ b/include/darwin.h
@@ -79,6 +79,7 @@ static __inline__ void platform_getoptreset(void)
 {
 	extern int optreset;
 	optreset = 0;
+	opterr = 1;
 }
 
 static __inline__ int platform_uuid_compare(uuid_t *uu1, uuid_t *uu2)
diff --git a/include/gnukfreebsd.h b/include/gnukfreebsd.h
index 1db3f4f010e3..26e5d9e08cd5 100644
--- a/include/gnukfreebsd.h
+++ b/include/gnukfreebsd.h
@@ -83,6 +83,7 @@ static __inline__ void platform_getoptreset(void)
 {
 	extern int optind;
 	optind = 0;
+	opterr = 1;
 }
 
 static __inline__ int platform_uuid_compare(uuid_t *uu1, uuid_t *uu2)
diff --git a/include/linux.h b/include/linux.h
index 1998941a6df5..a9273239c419 100644
--- a/include/linux.h
+++ b/include/linux.h
@@ -88,6 +88,7 @@ static __inline__ void platform_getoptreset(void)
 {
 	extern int optind;
 	optind = 0;
+	opterr = 1;
 }
 
 static __inline__ int platform_uuid_compare(uuid_t *uu1, uuid_t *uu2)
-- 
2.16.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v6 5/5] mkfs.xfs: add configuration file parsing support using our own parser
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
                   ` (3 preceding siblings ...)
  2018-06-12  0:30 ` [PATCH v6 4/5] xfsprogs: reset opterr on platform_getoptreset() Luis R. Rodriguez
@ 2018-06-12  0:30 ` Luis R. Rodriguez
  2018-06-12 16:46 ` [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Eric Sandeen
  5 siblings, 0 replies; 7+ messages in thread
From: Luis R. Rodriguez @ 2018-06-12  0:30 UTC (permalink / raw)
  To: sandeen, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak, Luis R. Rodriguez

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

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

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

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

The search path for the configuration file will be:

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

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

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

  mkfs.xfs -N

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

[data]
noalign=0

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

[log]
lazy-count=1

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

[naming]
ftype=1

[rtdev]
noalign=0

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 configure.ac         |   1 +
 include/builddefs.in |   2 +
 mkfs/Makefile        |   2 +-
 mkfs/config.c        | 670 +++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/config.h        |  25 +-
 mkfs/xfs_mkfs.c      |  72 +++++-
 6 files changed, 758 insertions(+), 14 deletions(-)
 create mode 100644 mkfs/config.c

diff --git a/configure.ac b/configure.ac
index 508eefede073..64bf41aef00c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -5,6 +5,7 @@ AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_SRCDIR([include/libxfs.h])
 AC_CONFIG_HEADER(include/platform_defs.h)
 AC_PREFIX_DEFAULT(/usr)
+test "$sysconfdir" = '${prefix}/etc' && sysconfdir=/etc
 
 AC_PROG_INSTALL
 AC_PROG_LIBTOOL
diff --git a/include/builddefs.in b/include/builddefs.in
index 8aac06cf90dc..e1ee9f7ba086 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -62,6 +62,7 @@ PKG_LIB_DIR	= @libdir@@libdirsuffix@
 PKG_INC_DIR	= @includedir@/xfs
 DK_INC_DIR	= @includedir@/disk
 PKG_MAN_DIR	= @mandir@
+PKG_ETC_DIR	= @sysconfdir@
 PKG_DOC_DIR	= @datadir@/doc/@pkg_name@
 PKG_LOCALE_DIR	= @datadir@/locale
 
@@ -196,6 +197,7 @@ endif
 
 GCFLAGS = $(DEBUG) \
 	  -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\"  \
+	  -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\"  \
 	  -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs
 
 ifeq ($(ENABLE_GETTEXT),yes)
diff --git a/mkfs/Makefile b/mkfs/Makefile
index c84f9b6ae63b..c7815b3e106b 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = mkfs.xfs
 
 HFILES =
-CFILES = proto.c xfs_mkfs.c
+CFILES = proto.c xfs_mkfs.c config.c
 
 LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \
 	$(LIBUUID)
diff --git a/mkfs/config.c b/mkfs/config.c
new file mode 100644
index 000000000000..1dafe2eb3b97
--- /dev/null
+++ b/mkfs/config.c
@@ -0,0 +1,670 @@
+/*
+ * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+#include <ctype.h>
+#include <dirent.h>
+#include <fcntl.h>
+
+#include "libxfs.h"
+#include "config.h"
+
+/*
+ * Enums for each configuration option. All these currently match the CLI
+ * parameters for now but this may change later, so we keep all this code
+ * and definitions separate. The rules for configuration parameters may also
+ * differ.
+ *
+ * We only provide definitions for what we currently support parsing.
+ */
+
+enum data_subopts {
+	D_NOALIGN = 0,
+};
+
+enum inode_subopts {
+	I_ALIGN = 0,
+	I_PROJID32BIT,
+	I_SPINODES,
+};
+
+enum log_subopts {
+	L_LAZYSBCNTR = 0,
+};
+
+enum metadata_subopts {
+	M_CRC = 0,
+	M_FINOBT,
+	M_RMAPBT,
+	M_REFLINK,
+};
+
+enum naming_subopts {
+	N_FTYPE = 0,
+};
+
+enum rtdev_subopts {
+	R_NOALIGN = 0,
+};
+
+/* Just define the max options array size manually right now */
+#define MAX_SUBOPTS	5
+
+static int config_check_bool(
+	uint64_t			value)
+{
+	if (value > 1)
+		goto out;
+
+	return 0;
+out:
+	errno = ERANGE;
+	return -1;
+}
+
+
+static int
+data_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum data_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case D_NOALIGN:
+		dft->sb_feat.nodalign = value;
+		return 0;
+	}
+	return -1;
+}
+
+static int
+inode_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum inode_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case I_ALIGN:
+		dft->sb_feat.inode_align = value;
+		return 0;
+	case I_PROJID32BIT:
+		dft->sb_feat.projid32bit = value;
+		return 0;
+	case I_SPINODES:
+		dft->sb_feat.spinodes = value;
+		return 0;
+	}
+	return -1;
+}
+
+static int
+log_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum log_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case L_LAZYSBCNTR:
+		dft->sb_feat.lazy_sb_counters = value;
+		return 0;
+	}
+	return -1;
+}
+
+static int
+metadata_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum metadata_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case M_CRC:
+		dft->sb_feat.crcs_enabled = value;
+		if (dft->sb_feat.crcs_enabled)
+			dft->sb_feat.dirftype = true;
+		return 0;
+	case M_FINOBT:
+		dft->sb_feat.finobt = value;
+		return 0;
+	case M_RMAPBT:
+		dft->sb_feat.rmapbt = value;
+		return 0;
+	case M_REFLINK:
+		dft->sb_feat.reflink = value;
+		return 0;
+	}
+	return -1;
+}
+
+static int
+naming_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum naming_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case N_FTYPE:
+		dft->sb_feat.dirftype = value;
+		return 0;
+	}
+	return -1;
+}
+
+static int
+rtdev_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum rtdev_subopts	subopt = psubopt;
+
+	if (config_check_bool(value) != 0)
+		return -1;
+
+	switch (subopt) {
+	case R_NOALIGN:
+		dft->sb_feat.nortalign = value;
+		return 0;
+	}
+	return -1;
+}
+
+struct confopts {
+	const char		*name;
+	const char		*subopts[MAX_SUBOPTS];
+	int			(*parser)(struct mkfs_default_params *dft,
+					  int psubopt, uint64_t value);
+	bool			seen;
+} confopts_tab[] = {
+	{
+		.name = "data",
+		.subopts = {
+			[D_NOALIGN] = "noalign",
+			NULL
+		},
+		.parser = data_config_parser,
+	},
+	{
+		.name = "inode",
+		.subopts = {
+			[I_ALIGN] = "align",
+			[I_PROJID32BIT] = "projid32bit",
+			[I_SPINODES] = "sparse",
+			NULL
+		},
+		.parser = inode_config_parser,
+	},
+	{
+		.name = "log",
+		.subopts = {
+			[L_LAZYSBCNTR] = "lazy-count",
+			NULL
+		},
+		.parser = log_config_parser,
+	},
+	{
+		.name = "naming",
+		.subopts = {
+			[N_FTYPE] = "ftype",
+			NULL
+		},
+		.parser = naming_config_parser,
+	},
+	{
+		.name = "rtdev",
+		.subopts = {
+			[R_NOALIGN] = "noalign",
+			NULL
+		},
+		.parser = rtdev_config_parser,
+	},
+	{
+		.name = "metadata",
+		.subopts = {
+			[M_CRC] = "crc",
+			[M_FINOBT] = "finobt",
+			[M_RMAPBT] = "rmapbt",
+			[M_REFLINK] = "reflink",
+			NULL
+		},
+		.parser = metadata_config_parser,
+	},
+};
+
+static struct confopts *
+get_confopts(
+	const char	*section)
+{
+	unsigned int	i;
+	struct confopts	*opts;
+
+	for (i=0; i < ARRAY_SIZE(confopts_tab); i++) {
+		opts = &confopts_tab[i];
+		if (strcmp(opts->name, section) == 0)
+			return opts;
+	}
+	errno = EINVAL;
+	return NULL;
+}
+
+enum parse_line_type {
+	PARSE_COMMENT = 0,
+	PARSE_EMPTY,
+	PARSE_SECTION,
+	PARSE_TAG_VALUE,
+	PARSE_INVALID,
+	PARSE_EOF,
+};
+
+static bool
+isempty(
+	const char	*line,
+	ssize_t		linelen)
+{
+	ssize_t		i = 0;
+	char		p;
+
+	while (i < linelen) {
+		p = line[i++];
+
+		/* tab or space */
+		if (!isblank(p))
+			return false;
+	}
+
+	return true;
+}
+
+static bool
+iscomment(
+	const char	*line,
+	ssize_t		linelen)
+{
+	ssize_t		i = 0;
+	char		p;
+
+	while (i != linelen) {
+		p = line[i];
+		i++;
+
+		/* tab or space */
+		if (isblank(p))
+			continue;
+
+		if (p == '#')
+			return true;
+
+		return false;
+	}
+
+	return false;
+}
+
+static enum parse_line_type
+parse_get_line_type(
+	const char	*line,
+	ssize_t		linelen,
+	char		**tag,
+	uint64_t	*value)
+{
+	int		ret;
+	uint64_t	u64_value;
+
+	if (isempty(line, linelen))
+		return PARSE_EMPTY;
+
+	if (iscomment(line, linelen))
+		return PARSE_COMMENT;
+
+	/* check if we have a section header */
+	ret = sscanf(line, " [%m[^]]]", tag);
+	if (ret == 1)
+		return  PARSE_SECTION;
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	/* should be a "tag = value" config option */
+	ret = sscanf(line, " %m[^ \t=] = %" PRIu64 " ", tag, &u64_value);
+	if (ret == 2) {
+		*value = u64_value;
+
+		return PARSE_TAG_VALUE;
+	}
+
+	if (ret == EOF)
+		return PARSE_EOF;
+
+	errno = EINVAL;
+	return PARSE_INVALID;
+}
+
+static int
+parse_config_stream(
+	struct mkfs_default_params	*dft,
+	const char 			*config_file,
+	FILE				*fp)
+{
+	int				ret = -1;
+	char				*line = NULL;
+	ssize_t				linelen;
+	size_t				len = 0, lineno = 0;
+	uint64_t			value;
+	enum parse_line_type		parse_type;
+	struct confopts			*confopt = NULL;
+	int				subopt;
+	char *tag = NULL;
+
+	while ((linelen = getline(&line, &len, fp)) != -1) {
+		char *ignore_value;
+		char *p;
+
+		lineno++;
+
+		/*
+		 * tag is allocated for us by scanf(), it must freed only on any
+		 * successful parse of a section or tag-value pair.
+		 */
+		parse_type = parse_get_line_type(line, linelen, &tag, &value);
+
+		switch (parse_type) {
+		case PARSE_EMPTY:
+		case PARSE_COMMENT:
+			/* Nothing tag to free for these */
+			continue;
+		case PARSE_EOF:
+			break;
+		case PARSE_INVALID:
+			ret = -1;
+			fprintf(stderr, _("Invalid line %s:%zu : %s\n"),
+					  config_file, lineno, line);
+			goto out;
+		case PARSE_SECTION:
+			confopt = get_confopts(tag);
+			if (!confopt) {
+				fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"),
+						config_file, lineno, tag);
+				goto out_free_tag;
+			}
+			if (!confopt->subopts) {
+				fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"),
+						config_file, lineno, tag);
+				goto out_free_tag;
+			}
+			if (confopt->seen) {
+				errno = EINVAL;
+				fprintf(stderr, _("Section '%s' respecified\n"),
+						tag);
+				goto out_free_tag;
+			}
+			confopt->seen = true;
+			free(tag);
+			break;
+		case PARSE_TAG_VALUE:
+			if (!confopt) {
+				fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"),
+						config_file, lineno, line);
+				goto out_free_tag;
+			}
+
+			/*
+			 * We re-use the line buffer allocated by getline(),
+			 * however line must be kept pointing to its original
+			 * value to free it later. A separate pointer is needed
+			 * as getsubopt() will otherwise muck with the value
+			 * passed.
+			 */
+			p = line;
+
+			/*
+			 * Trims white spaces. getsubopt() does not grok
+			 * white space, it would fail otherwise.
+			 */
+			snprintf(p, len, "%s=%lu", tag, value);
+
+			/* Not needed anymore */
+			free(tag);
+
+			/*
+			 * We only use getsubopt() to validate the possible
+			 * subopt, we already parsed the value and its already
+			 * in a more preferred data type.
+			 */
+			subopt = getsubopt(&p, (char **) confopt->subopts,
+					   &ignore_value);
+
+			ret = confopt->parser(dft, subopt, value);
+			if (ret) {
+				errno = EINVAL;
+				fprintf(stderr, _("Error parsine line %s:%zu : %s\n"),
+						config_file, lineno, line);
+				goto out;
+			}
+
+			break;
+		}
+		free(line);
+		line = NULL;
+	}
+
+out:
+	/* We must free even if getline() failed */
+	if (line)
+		free(line);
+	return ret;
+
+out_free_tag:
+	if (tag)
+		free(tag);
+	ret = -1;
+	goto out;
+}
+
+static int
+config_stat_check(
+	struct stat		*sp)
+{
+	if (!S_ISREG(sp->st_mode)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/* Anything beyond 1 MiB is kind of silly right now */
+	if (sp->st_size > 1 * 1024 * 1024) {
+		errno = E2BIG;
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * If the file is not found -1 is returned and errno set. Otherwise
+ * the file descriptor is returned.
+ */
+int
+open_cli_config(
+	int			dirfd,
+	const char		*cli_config_file,
+	char			**fpath)
+{
+	int			fd = -1, len, ret;
+	struct stat		st;
+
+	fd = openat(AT_FDCWD, cli_config_file, O_NOFOLLOW, O_RDONLY);
+	if (fd < 0) {
+		len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR,
+			       cli_config_file);
+		/* Indicates truncation */
+		if (len >= PATH_MAX) {
+			errno = ENAMETOOLONG;
+			goto out;
+		}
+
+		fd = openat(dirfd, cli_config_file, O_NOFOLLOW, O_RDONLY);
+		if (fd < 0)
+			goto out;
+
+		ret = fstatat(dirfd, cli_config_file, &st,
+			      AT_SYMLINK_NOFOLLOW);
+		if (ret != 0)
+			goto err_out_close;
+
+		ret = config_stat_check(&st);
+		if (ret != 0)
+			goto err_out_close;
+
+		goto out;
+	}
+
+	memcpy(*fpath, cli_config_file, strlen(cli_config_file));
+
+	ret = fstatat(AT_FDCWD, cli_config_file, &st, AT_SYMLINK_NOFOLLOW);
+	if (ret != 0)
+		goto err_out_close;
+
+	ret = config_stat_check(&st);
+	if (ret != 0)
+		goto err_out_close;
+out:
+	return fd;
+err_out_close:
+	close(fd);
+	return -1;
+}
+
+int
+open_config_file(
+	const char			*cli_config_file,
+	struct mkfs_default_params	*dft,
+	char				**fpath)
+{
+	int			dirfd, fd = -1, len, ret;
+	struct stat		st;
+
+	*fpath = malloc(PATH_MAX);
+	if (!*fpath)
+		return -1;
+
+	memset(*fpath, 0, PATH_MAX);
+
+	dirfd = open(MKFS_XFS_CONF_DIR, O_PATH|O_NOFOLLOW|O_DIRECTORY);
+
+	if (cli_config_file) {
+		if (strlen(cli_config_file) > PATH_MAX) {
+			errno = ENAMETOOLONG;
+			goto out;
+		}
+		fd = open_cli_config(dirfd, cli_config_file, fpath);
+		goto out;
+	}
+
+	fd = openat(dirfd, "default", O_NOFOLLOW, O_RDONLY);
+	if (fd < 0)
+		goto out;
+
+	dft->type = DEFAULTS_CONFIG;
+
+	len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR, "default");
+	/* Indicates truncation */
+	if (len >= PATH_MAX) {
+		errno = ENAMETOOLONG;
+		goto err_out_close;
+	}
+
+	ret = fstatat(dirfd, "default", &st, AT_SYMLINK_NOFOLLOW);
+	if (ret != 0)
+		goto err_out_close;
+
+	ret = config_stat_check(&st);
+	if (ret != 0)
+		goto err_out_close;
+
+out:
+	if (fd < 0) {
+		if (dft->type != DEFAULTS_BUILTIN) {
+			fprintf(stderr, _("Unable to open %s config file: %s : %s\n"),
+					default_type_str(dft->type), *fpath,
+					strerror(errno));
+			free(*fpath);
+			exit(1);
+		}
+	}
+	if (dirfd >= 0)
+		close(dirfd);
+	return fd;
+
+err_out_close:
+	close(fd);
+	fd = -1;
+	goto out;
+}
+
+/*
+ * This is only called *iff* there is a configuration file which we know we
+ * *must* parse.
+ */
+int
+parse_defaults_file(
+	int					fd,
+	struct mkfs_default_params		*dft,
+	const char				*config_file)
+{
+	FILE			*fp;
+	int			ret;
+
+	fp = fdopen(fd, "r");
+	if (!fp)
+		goto out;
+
+	ret = parse_config_stream(dft, config_file, fp);
+	if (ret) {
+		fclose(fp);
+		goto out;
+	}
+
+	printf(_("config-file=%s\n"), config_file);
+
+	return 0;
+out:
+	return -1;
+}
diff --git a/mkfs/config.h b/mkfs/config.h
index a3c6c99c3064..163d1e893eab 100644
--- a/mkfs/config.h
+++ b/mkfs/config.h
@@ -19,6 +19,8 @@
 #ifndef _XFS_MKFS_CONFIG_H
 #define _XFS_MKFS_CONFIG_H
 
+#define MKFS_XFS_CONF_DIR      ROOT_SYSCONFDIR "/xfs/mkfs"
+
 struct fsxattr;
 
 /*
@@ -29,7 +31,7 @@ struct fsxattr;
  * source can overriding the later source:
  *
  * 	o built-in defaults
- * 	o configuration file (XXX)
+ * 	o configuration file
  * 	o command line
  *
  * These values are not used directly - they are inputs into the mkfs geometry
@@ -60,9 +62,14 @@ struct sb_feat_args {
  * These are the different possibilities by which you can end up parsing
  * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
  * file parsed and we are using the built-in defaults on this code.
+ * DEFAULTS_CONFIG means the default configuration file was found and used.
+ * DEFAULTS_CLI_CONFIG means the user asked for a custom configuration type
+ * through the command line interface and it was used.
  */
 enum default_params_type {
 	DEFAULTS_BUILTIN = 0,
+	DEFAULTS_CONFIG,
+	DEFAULTS_CLI_CONFIG,
 };
 
 /*
@@ -91,8 +98,24 @@ static inline const char *default_type_str(enum default_params_type type)
 	switch (type) {
 	case DEFAULTS_BUILTIN:
 		return _("package built-in definitions");
+	case DEFAULTS_CONFIG:
+		return _("package default config file");
+	case DEFAULTS_CLI_CONFIG:
+		return _("CLI supplied file");
 	}
 	return _("Unkown\n");
 }
 
+int
+open_config_file(
+	const char			*cli_config_file,
+	struct mkfs_default_params	*dft,
+	char				**fpath);
+
+int
+parse_defaults_file(
+	int				fd,
+	struct mkfs_default_params	*dft,
+	const char			*config_file);
+
 #endif /* _XFS_MKFS_CONFIG_H */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e344aa9dad1..752358abd7cd 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3744,6 +3744,9 @@ main(
 			.nortalign = false,
 		},
 	};
+	char			*cli_config_file = NULL;
+	char			*config_file = NULL;
+	int			fd, ret;
 
 	platform_uuid_generate(&cli.uuid);
 	progname = basename(argv[0]);
@@ -3752,26 +3755,71 @@ main(
 	textdomain(PACKAGE);
 
 	/*
-	 * TODO: Sourcing defaults from a config file
-	 *
 	 * Before anything else, see if there's a config file with different
-	 * defaults. If a file exists in <package location>, read in the new
-	 * default values and overwrite them in the &dft structure. This way the
-	 * new defaults will apply before we parse the CLI, and the CLI will
-	 * still be able to override them. When more than one source is
-	 * implemented, emit a message to indicate where the defaults being
-	 * used came from.
+	 * defaults. If the CLI specified a full path we use and require that.
+	 * If a relative path was provided on the CLI we search the allowed
+	 * search paths for the file. If no config file was specified on the
+	 * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if
+	 * present, however this file is optional.
 	 *
-	 * printf(_("Default configuration sourced from %s\n"),
-	 *	  default_type_str(dft.type));
+	 * If a configuration file is found we use it to help overwrite default
+	 * values in the &dft structure. This way the new defaults will apply
+	 * before we parse the CLI, and the user will still be able to override
+	 * them through the CLI.
 	 */
 
-	/* copy new defaults into CLI parsing structure */
+	/*
+	 * Pull config line options from command line
+	 */
+	 opterr = 0;
+	 while ((c = getopt(argc, argv, "-c:")) != EOF) {
+		switch (c) {
+		case 'c':
+			if (cli_config_file) {
+				fprintf(stderr, _("respecification of configuration not allowed\n"));
+				exit(1);
+			}
+			cli_config_file = optarg;
+			dft.type = DEFAULTS_CLI_CONFIG;
+			break;
+		default:
+			continue;
+		}
+	}
+
+	fd = open_config_file(cli_config_file, &dft, &config_file);
+	if (fd >= 0) {
+		ret = parse_defaults_file(fd, &dft, config_file);
+		if (ret) {
+			fprintf(stderr, _("Error parsing %s config file: %s : %s\n"),
+					default_type_str(dft.type),
+					config_file,
+					strerror(errno));
+			free(config_file);
+			close(fd);
+			exit(1);
+		}
+		free(config_file);
+		close(fd);
+	}
+
+	printf(_("Default configuration sourced from %s\n"),
+	       default_type_str(dft.type));
+
+	/*
+	 * Done parsing defaults now, so memcpy defaults into CLI
+	 * structure, reset getopt and start parsing CLI options
+	 */
 	memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
 	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	platform_getoptreset();
+
+	while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
 		switch (c) {
+		case 'c':
+			/* already validated and parsed, ignore */
+			break;
 		case 'C':
 		case 'f':
 			force_overwrite = 1;
-- 
2.16.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support
  2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
                   ` (4 preceding siblings ...)
  2018-06-12  0:30 ` [PATCH v6 5/5] mkfs.xfs: add configuration file parsing support using our own parser Luis R. Rodriguez
@ 2018-06-12 16:46 ` Eric Sandeen
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Sandeen @ 2018-06-12 16:46 UTC (permalink / raw)
  To: Luis R. Rodriguez, linux-xfs
  Cc: darrick.wong, jack, jeffm, okurz, lpechacek, jtulak

On 6/11/18 7:30 PM, Luis R. Rodriguez wrote:
> This is v6 series addresses the feedback from the last v5 series. The
> biggest change is for dumping the opterr reset onto the shared helper
> platform_getoptreset(), and addressing the TOCTOU race between stat-ing
> and opening. The rest of the changes are all things which were mentioned
> during review, the details listed below.

Ok, I've pushed this to a temporary mkfs-config branch, based
on the current for-next.  I'll send a handful of followup patches
and then other folks who've been watching (djwong, dchinner?) can
send patches or highlight other issues or concerns that are outstanding.

If all goes well we can build up something we're happy with on that
branch and merge it to for-next & hopefully xfsprogs-4.17, yet.

Thanks,
-Eric

> 
> You can also get the code from my kernel.org xfsprogs-dev 20180611-own-parser-v6
> branch [0].
> 
> Reviews, questions, or rants are greatly appreciated.
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/xfsprogs-dev.git/log/?h=20180611-own-parser-v6
> 
> Changes on v6:
> 
> - Use -1 return value consistently when returning
> - Fix use of one errno use
> - Remove &confopts_tab[i] check which can never happen
> - Fixed uninitialized return value use on parse_config_stream()
> - Remove extra tag declaration on parse_config_stream()
> - Address the TOCTOU race
> - Move opterr to shorten the first parameter parsing on main(). We now
>   just use platform_getoptreset() to reset the settings so we can parse
>   more.
> 
> Changes on v5:
> 
> - We no force sysconfdir=/etc if not set so we also drop the last patch
>   for debian as its no longer needed
> 
> - We use config_check_bool() as during review on the last series we
>   failed to pick up on the fact that we were *not* detecting if a value
>   for a field was greater than 1, so we were allowing for instance crc=1
>   although this was silently mapping to crc=1. Note that although this
>   issue was present, the check for conflicts is still in effect and
>   functional. The only reason the issue with bools existed is we cast
>   down the uint64_t to bool and that resolves the issue. For now all
>   subuopt parsers use config_check_bool(). When and if other values are
>   introduced they can change this.
> 
> - Use fstat() and openat() as requested instead of the odd checks for
>   first and second characters. This feels nicer.
> 
> - Use negative return values and fixate errno in better locations.
> 
> - Drop the man pages and yield to Sandeed for that.
> 
> - Re-introduce the source type enum
> 
> - Dealing with a config file is now split into an open call and a
>   separate parse file. This does read much better now.
> 
> - Fix a missing free(line) on the switch for parsing when we don't have
>   an error.
> 
> - Adjust random error messages to reflect a bit better what is issue
>   creeped up.
> 
> Changes on v4:
> 
> - Tons of man page documentation enhancements from Darrick.
> 
> - Adjusted a few error code messages/ paths to make more sense as per
>   Darrick's feedback.
> 
> Changes on v3:
> 
> - Prefix errors on the configuration parsing with config file used and
>   exact line.
> 
> - Use uint64_t throughout all parsers as requested.
> 
> - Parse the CLI early for -c as I originally had implemented this on v1
>   but now platform_getoptreset() to reset the opts.
> 
> - Drop the enum for the source type for the configuration file type,
>   note that since we rely on the stack to set the variables we should
>   then always set the dft.src on main().
> 
> - Use getline() for fetching lines as requested
> 
> - Add a special iscomment() and isempty() to handle all space and
>   comment parsing. Now when PARSE_INVALID issued we really mean it.
> 
> - Embrace latest bikeshed preferences:
>         config.c, config.h, random function names
> 
> - Add man pages to be parsed via configure so that @sysconfigdir@ gets
>   properly parsed
> 
> - get_confopts() now iterates over the known sections tab and gives you
>   the right struct right away.
> 
> - Add respecification checks for a section.
> 
> - Add respecification checks for -c, -c is only allowed once.
> 
> - Add --sysconfigdir to debian/rules, note that if you *don't* set
>   --sysconfigdir your man pages will end up with ${prefix}. We could
>   add a secondary target parsing just in case, or we can do some hacks
>   with autoconf for this, but I don't think its worth it. Other packages
>   deal with this by having ./configure always run with --sysconfigdir
>   set (see systemd for instance).
> 
> - Reduced the number of declared enums, only enums for the config
>   subparams which are currently supported for parsing are declared.
> 
> - mkfs.xfs -c foo now will search for $PWD/foo and if that fails the
>   sysconfigdir/mkfs.xfs.d/foo.
> 
> - mkfs.xfs -c ../foo works and so should ./foo, etc.
> 
> - The MKFS_XFS_CONFIG variable support was dropped in favor or just
>   allowing the user to specify the full path now.
> 
> - Embraces xfsprogs coding style, c'est la vie.
> 
> Changes on v2:
> 
> - Stayed with our own parser as its the smallest and we're willing to
>   maintain it as its simple and clear.
> 
> - Use -c for the configuration file, and drop the "type" nomenclature to
>   avoid confusion on the interwebs.
> 
> - Start to split files off
> 
> - Duplicate a bit of items as suggested at LSFMM for the configuration
>   parser structures. We can later consolidate if we think its really
>   needed, however we want the freedom to change these as we see fit and
>   most importantly keep the code apart.
> 
> - Conflict resolution and validation is managed now by piggy backing off
>   of the idea of using the defaults to instantiate CLI parameters. CLI
>   always overrides.
> 
> Luis R. Rodriguez (5):
>   mkfs: distinguish between struct sb_feat_args and struct cli_params
>   mkfs: move shared config structs and into their own headers
>   mkfs: replace defaults source with an enum
>   xfsprogs: reset opterr on platform_getoptreset()
>   mkfs.xfs: add configuration file parsing support using our own parser
> 
>  configure.ac          |   1 +
>  include/builddefs.in  |   2 +
>  include/darwin.h      |   1 +
>  include/gnukfreebsd.h |   1 +
>  include/linux.h       |   1 +
>  mkfs/Makefile         |   2 +-
>  mkfs/config.c         | 670 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  mkfs/config.h         | 121 +++++++++
>  mkfs/xfs_mkfs.c       | 127 +++++-----
>  9 files changed, 865 insertions(+), 61 deletions(-)
>  create mode 100644 mkfs/config.c
>  create mode 100644 mkfs/config.h
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-06-12 16:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-12  0:30 [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Luis R. Rodriguez
2018-06-12  0:30 ` [PATCH v6 1/5] mkfs: distinguish between struct sb_feat_args and struct cli_params Luis R. Rodriguez
2018-06-12  0:30 ` [PATCH v6 2/5] mkfs: move shared config structs and into their own headers Luis R. Rodriguez
2018-06-12  0:30 ` [PATCH v6 3/5] mkfs: replace defaults source with an enum Luis R. Rodriguez
2018-06-12  0:30 ` [PATCH v6 4/5] xfsprogs: reset opterr on platform_getoptreset() Luis R. Rodriguez
2018-06-12  0:30 ` [PATCH v6 5/5] mkfs.xfs: add configuration file parsing support using our own parser Luis R. Rodriguez
2018-06-12 16:46 ` [PATCH v6 0/5] xfsprogs: add mkfs.xfs configuration file parsing support Eric Sandeen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.