* [PATCH 1/5] build: add support for libinih for mkfs
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
@ 2020-10-15 3:29 ` Dave Chinner
2020-10-15 5:40 ` Darrick J. Wong
2020-10-15 3:29 ` [PATCH 2/5] mkfs: add initial ini format config file parsing support Dave Chinner
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 3:29 UTC (permalink / raw)
To: linux-xfs
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 +++
doc/INSTALL | 5 +++++
include/builddefs.in | 1 +
m4/package_inih.m4 | 20 ++++++++++++++++++++
4 files changed, 29 insertions(+)
create mode 100644 m4/package_inih.m4
diff --git a/configure.ac b/configure.ac
index 4674673ed67c..dc57bbd7cd8c 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/doc/INSTALL b/doc/INSTALL
index d4395eefa834..2b04f9cfe108 100644
--- a/doc/INSTALL
+++ b/doc/INSTALL
@@ -28,6 +28,11 @@ Linux Instructions
(on an RPM based system) or the uuid-dev package (on a Debian system)
as some of the commands make use of the UUID library provided by these.
+ If your distro does not provide a libinih package, you can download and build
+ it from source from the upstream repository found at:
+
+ https://github.com/benhoyt/inih
+
To build the package and install it manually, use the following steps:
# make
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] build: add support for libinih for mkfs
2020-10-15 3:29 ` [PATCH 1/5] build: add support for libinih for mkfs Dave Chinner
@ 2020-10-15 5:40 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:21PM +1100, 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.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> configure.ac | 3 +++
> doc/INSTALL | 5 +++++
> include/builddefs.in | 1 +
> m4/package_inih.m4 | 20 ++++++++++++++++++++
> 4 files changed, 29 insertions(+)
> create mode 100644 m4/package_inih.m4
>
> diff --git a/configure.ac b/configure.ac
> index 4674673ed67c..dc57bbd7cd8c 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/doc/INSTALL b/doc/INSTALL
> index d4395eefa834..2b04f9cfe108 100644
> --- a/doc/INSTALL
> +++ b/doc/INSTALL
> @@ -28,6 +28,11 @@ Linux Instructions
> (on an RPM based system) or the uuid-dev package (on a Debian system)
> as some of the commands make use of the UUID library provided by these.
>
> + If your distro does not provide a libinih package, you can download and build
> + it from source from the upstream repository found at:
> +
> + https://github.com/benhoyt/inih
Someone's gonna have fun packaging this for RHEL. ;)
Oh look, a Debian package. Assuming the legal-minded are ok with us
mixing BSD and GPL2 code,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> +
> To build the package and install it manually, use the following steps:
>
> # make
> 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] mkfs: add initial ini format config file parsing support
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
2020-10-15 3:29 ` [PATCH 1/5] build: add support for libinih for mkfs Dave Chinner
@ 2020-10-15 3:29 ` Dave Chinner
2020-10-15 5:46 ` Darrick J. Wong
2020-10-15 3:29 ` [PATCH 3/5] mkfs: constify various strings Dave Chinner
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 3:29 UTC (permalink / raw)
To: linux-xfs
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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 115 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 8fe149d74b0a..e84be74fb100 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_OPTFILE = 0,
+ C_MAX_OPTS,
+};
+
enum {
D_AGCOUNT = 0,
D_FILE,
@@ -237,6 +243,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_OPTFILE] = "options",
+ },
+ .subopt_params = {
+ { .index = C_OPTFILE,
+ .conflicts = { { NULL, LAST_CONFLICT } },
+ .defaultval = SUBOPT_NEEDS_VAL,
+ },
+ },
+};
+
static struct opt_params dopts = {
.name = 'd',
.subopts = {
@@ -748,6 +776,8 @@ struct cli_params {
int sectorsize;
int blocksize;
+ char *cfgfile;
+
/* parameters that depend on sector/block size being validated. */
char *dsize;
char *agsize;
@@ -862,6 +892,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\
@@ -1385,6 +1416,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_OPTFILE:
+ cli->cfgfile = getstr(value, opts, subopt);
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int
data_opts_parser(
struct opt_params *opts,
@@ -1656,6 +1704,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 },
@@ -3562,6 +3611,61 @@ 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)
+{
+ int error;
+
+ if (!cli->cfgfile)
+ return;
+
+ error = ini_parse(cli->cfgfile, cfgfile_parse_ini, cli);
+ if (error) {
+ if (error > 0) {
+ fprintf(stderr,
+ _("%s: Unrecognised input on line %d. Aborting.\n"),
+ cli->cfgfile, error);
+ } else if (error == -2) {
+ fprintf(stderr,
+ _("Memory allocation failure parsing %s. Aborting.\n"),
+ cli->cfgfile);
+ } else {
+ fprintf(stderr,
+ _("Unable to open config file %s. Aborting.\n"),
+ cli->cfgfile);
+ }
+ exit(1);
+ }
+ printf(_("Parameters parsed from config file %s successfully\n"),
+ cli->cfgfile);
+}
+
int
main(
int argc,
@@ -3648,13 +3752,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':
@@ -3698,6 +3803,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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/5] mkfs: add initial ini format config file parsing support
2020-10-15 3:29 ` [PATCH 2/5] mkfs: add initial ini format config file parsing support Dave Chinner
@ 2020-10-15 5:46 ` Darrick J. Wong
2020-10-15 6:09 ` Dave Chinner
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:46 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:22PM +1100, 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.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mkfs/Makefile | 2 +-
> mkfs/xfs_mkfs.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 115 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 8fe149d74b0a..e84be74fb100 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_OPTFILE = 0,
> + C_MAX_OPTS,
> +};
> +
> enum {
> D_AGCOUNT = 0,
> D_FILE,
> @@ -237,6 +243,28 @@ static struct opt_params bopts = {
> },
> };
>
> +/*
> + * Config file specification. Usage is:
> + *
> + * mkfs.xfs -c file=<name>
I thought it was -c options=/dev/random ?
> + *
> + * 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_OPTFILE] = "options",
Sure looks that way here...
> + },
> + .subopt_params = {
> + { .index = C_OPTFILE,
> + .conflicts = { { NULL, LAST_CONFLICT } },
> + .defaultval = SUBOPT_NEEDS_VAL,
> + },
> + },
> +};
> +
> static struct opt_params dopts = {
> .name = 'd',
> .subopts = {
> @@ -748,6 +776,8 @@ struct cli_params {
> int sectorsize;
> int blocksize;
>
> + char *cfgfile;
> +
> /* parameters that depend on sector/block size being validated. */
> char *dsize;
> char *agsize;
> @@ -862,6 +892,7 @@ usage( void )
> {
> fprintf(stderr, _("Usage: %s\n\
> /* blocksize */ [-b size=num]\n\
> +/* config file */ [-c file=xxx]\n\
...but then we go back to -c file=...
> /* 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\
> @@ -1385,6 +1416,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_OPTFILE:
> + cli->cfgfile = getstr(value, opts, subopt);
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> static int
> data_opts_parser(
> struct opt_params *opts,
> @@ -1656,6 +1704,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 },
> @@ -3562,6 +3611,61 @@ 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
XFS, SAX style.
> + * 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)
> +{
> + int error;
> +
> + if (!cli->cfgfile)
> + return;
> +
> + error = ini_parse(cli->cfgfile, cfgfile_parse_ini, cli);
> + if (error) {
> + if (error > 0) {
> + fprintf(stderr,
> + _("%s: Unrecognised input on line %d. Aborting.\n"),
> + cli->cfgfile, error);
> + } else if (error == -2) {
> + fprintf(stderr,
> + _("Memory allocation failure parsing %s. Aborting.\n"),
> + cli->cfgfile);
> + } else {
> + fprintf(stderr,
> + _("Unable to open config file %s. Aborting.\n"),
> + cli->cfgfile);
I worry about libinih someday defining more negative error codes. -1 is
"open failed", -2 is OOM, and positive is the line number of a parsing
error, at least according to the documentation.
Maybe we should handle -1 specifically and use the else as a catchall
for unrecognized error codes?
> + }
> + exit(1);
> + }
> + printf(_("Parameters parsed from config file %s successfully\n"),
> + cli->cfgfile);
> +}
> +
> int
> main(
> int argc,
> @@ -3648,13 +3752,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':
> @@ -3698,6 +3803,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
-c options=xxx?
--D
> + * 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/5] mkfs: add initial ini format config file parsing support
2020-10-15 5:46 ` Darrick J. Wong
@ 2020-10-15 6:09 ` Dave Chinner
0 siblings, 0 replies; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 6:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Oct 14, 2020 at 10:46:33PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 15, 2020 at 02:29:22PM +1100, 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.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > mkfs/Makefile | 2 +-
> > mkfs/xfs_mkfs.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 115 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 8fe149d74b0a..e84be74fb100 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_OPTFILE = 0,
> > + C_MAX_OPTS,
> > +};
> > +
> > enum {
> > D_AGCOUNT = 0,
> > D_FILE,
> > @@ -237,6 +243,28 @@ static struct opt_params bopts = {
> > },
> > };
> >
> > +/*
> > + * Config file specification. Usage is:
> > + *
> > + * mkfs.xfs -c file=<name>
>
> I thought it was -c options=/dev/random ?
I fixed that, refreshed the patch and updated the change log! How
the hell did I lose that change?
> > { 'd', &dopts, data_opts_parser },
> > { 'i', &iopts, inode_opts_parser },
> > { 'l', &lopts, log_opts_parser },
> > @@ -3562,6 +3611,61 @@ 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
>
> XFS, SAX style.
>
> > + * 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)
> > +{
> > + int error;
> > +
> > + if (!cli->cfgfile)
> > + return;
> > +
> > + error = ini_parse(cli->cfgfile, cfgfile_parse_ini, cli);
> > + if (error) {
> > + if (error > 0) {
> > + fprintf(stderr,
> > + _("%s: Unrecognised input on line %d. Aborting.\n"),
> > + cli->cfgfile, error);
> > + } else if (error == -2) {
> > + fprintf(stderr,
> > + _("Memory allocation failure parsing %s. Aborting.\n"),
> > + cli->cfgfile);
> > + } else {
> > + fprintf(stderr,
> > + _("Unable to open config file %s. Aborting.\n"),
> > + cli->cfgfile);
>
> I worry about libinih someday defining more negative error codes. -1 is
> "open failed", -2 is OOM, and positive is the line number of a parsing
> error, at least according to the documentation.
>
> Maybe we should handle -1 specifically and use the else as a catchall
> for unrecognized error codes?
Makes sense.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] mkfs: constify various strings
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
2020-10-15 3:29 ` [PATCH 1/5] build: add support for libinih for mkfs Dave Chinner
2020-10-15 3:29 ` [PATCH 2/5] mkfs: add initial ini format config file parsing support Dave Chinner
@ 2020-10-15 3:29 ` Dave Chinner
2020-10-15 5:31 ` Darrick J. Wong
2020-10-15 3:29 ` [PATCH 4/5] mkfs: hook up suboption parsing to ini files Dave Chinner
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 3:29 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Because the ini parser uses const strings and so the opt parsing
needs to be told about it to avoid compiler warnings.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
include/linux.h | 2 +-
mkfs/xfs_mkfs.c | 28 ++++++++++++++--------------
2 files changed, 15 insertions(+), 15 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 e84be74fb100..99ce0dc48d3b 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -975,8 +975,8 @@ respec(
static void
unknown(
- char opt,
- char *s)
+ const char opt,
+ const char *s)
{
fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
usage();
@@ -1387,7 +1387,7 @@ getnum(
*/
static char *
getstr(
- char *str,
+ const char *str,
struct opt_params *opts,
int index)
{
@@ -1396,14 +1396,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) {
@@ -1420,7 +1420,7 @@ static int
cfgfile_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1437,7 +1437,7 @@ static int
data_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1506,7 +1506,7 @@ static int
inode_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1541,7 +1541,7 @@ static int
log_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1587,7 +1587,7 @@ static int
meta_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1621,7 +1621,7 @@ static int
naming_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1650,7 +1650,7 @@ static int
rtdev_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1680,7 +1680,7 @@ static int
sector_opts_parser(
struct opt_params *opts,
int subopt,
- char *value,
+ const char *value,
struct cli_params *cli)
{
switch (subopt) {
@@ -1700,7 +1700,7 @@ static struct subopts {
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 },
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/5] mkfs: constify various strings
2020-10-15 3:29 ` [PATCH 3/5] mkfs: constify various strings Dave Chinner
@ 2020-10-15 5:31 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:23PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Because the ini parser uses const strings and so the opt parsing
> needs to be told about it to avoid compiler warnings.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> include/linux.h | 2 +-
> mkfs/xfs_mkfs.c | 28 ++++++++++++++--------------
> 2 files changed, 15 insertions(+), 15 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 e84be74fb100..99ce0dc48d3b 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -975,8 +975,8 @@ respec(
>
> static void
> unknown(
> - char opt,
> - char *s)
> + const char opt,
> + const char *s)
> {
> fprintf(stderr, _("unknown option -%c %s\n"), opt, s);
> usage();
> @@ -1387,7 +1387,7 @@ getnum(
> */
> static char *
> getstr(
> - char *str,
> + const char *str,
> struct opt_params *opts,
> int index)
> {
> @@ -1396,14 +1396,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;
Hmm do any of the getstr callers actually change the return value?
Er... holy $bovine you have to change a lot of stuff everywhere to make
the const stick.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> }
>
> static int
> block_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1420,7 +1420,7 @@ static int
> cfgfile_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1437,7 +1437,7 @@ static int
> data_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1506,7 +1506,7 @@ static int
> inode_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1541,7 +1541,7 @@ static int
> log_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1587,7 +1587,7 @@ static int
> meta_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1621,7 +1621,7 @@ static int
> naming_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1650,7 +1650,7 @@ static int
> rtdev_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1680,7 +1680,7 @@ static int
> sector_opts_parser(
> struct opt_params *opts,
> int subopt,
> - char *value,
> + const char *value,
> struct cli_params *cli)
> {
> switch (subopt) {
> @@ -1700,7 +1700,7 @@ static struct subopts {
> 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 },
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] mkfs: hook up suboption parsing to ini files
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
` (2 preceding siblings ...)
2020-10-15 3:29 ` [PATCH 3/5] mkfs: constify various strings Dave Chinner
@ 2020-10-15 3:29 ` Dave Chinner
2020-10-15 5:24 ` Darrick J. Wong
2020-10-15 3:29 ` [PATCH 5/5] mkfs: document config files in mkfs.xfs(8) Dave Chinner
2020-10-15 5:13 ` [PATCH 0/5] mkfs: Configuration file defined options Darrick J. Wong
5 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 3:29 UTC (permalink / raw)
To: linux-xfs
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>
---
mkfs/xfs_mkfs.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 80 insertions(+), 15 deletions(-)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 99ce0dc48d3b..370ac6194e2f 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -143,6 +143,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.
@@ -201,6 +208,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 {
@@ -228,6 +237,7 @@ static struct opt_params sopts;
static struct opt_params bopts = {
.name = 'b',
+ .ini_name = "block",
.subopts = {
[B_SIZE] = "size",
},
@@ -267,6 +277,7 @@ static struct opt_params copts = {
static struct opt_params dopts = {
.name = 'd',
+ .ini_name = "data",
.subopts = {
[D_AGCOUNT] = "agcount",
[D_FILE] = "file",
@@ -411,6 +422,7 @@ static struct opt_params dopts = {
static struct opt_params iopts = {
.name = 'i',
+ .ini_name = "inode",
.subopts = {
[I_ALIGN] = "align",
[I_MAXPCT] = "maxpct",
@@ -472,6 +484,7 @@ static struct opt_params iopts = {
static struct opt_params lopts = {
.name = 'l',
+ .ini_name = "log",
.subopts = {
[L_AGNUM] = "agnum",
[L_INTERNAL] = "internal",
@@ -571,6 +584,7 @@ static struct opt_params lopts = {
static struct opt_params nopts = {
.name = 'n',
+ .ini_name = "naming",
.subopts = {
[N_SIZE] = "size",
[N_VERSION] = "version",
@@ -602,6 +616,7 @@ static struct opt_params nopts = {
static struct opt_params ropts = {
.name = 'r',
+ .ini_name = "realtime",
.subopts = {
[R_EXTSIZE] = "extsize",
[R_SIZE] = "size",
@@ -652,6 +667,7 @@ static struct opt_params ropts = {
static struct opt_params sopts = {
.name = 's',
+ .ini_name = "sector",
.subopts = {
[S_SIZE] = "size",
[S_SECTSIZE] = "sectsize",
@@ -682,6 +698,7 @@ static struct opt_params sopts = {
static struct opt_params mopts = {
.name = 'm',
+ .ini_name = "metadata",
.subopts = {
[M_CRC] = "crc",
[M_FINOBT] = "finobt",
@@ -982,6 +999,17 @@ unknown(
usage();
}
+static void
+invalid_cfgfile_opt(
+ const char *filename,
+ const char *section,
+ const char *name,
+ const char *value)
+{
+ fprintf(stderr, _("%s: invalid config file option: [%s]:%s=%s\n"),
+ filename, section, name, value);
+}
+
static void
check_device_type(
const char *name,
@@ -1696,23 +1724,22 @@ sector_opts_parser(
}
static struct subopts {
- char opt;
struct opt_params *opts;
int (*parser)(struct opt_params *opts,
int subopt,
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
@@ -1726,7 +1753,7 @@ parse_subopts(
int ret = 0;
while (sop->opts) {
- if (sop->opt == opt)
+ if (opt && sop->opts->name == opt)
break;
sop++;
}
@@ -1749,6 +1776,45 @@ parse_subopts(
}
}
+static bool
+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 invalid_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 invalid_opt;
+ return true;
+ }
+ }
+invalid_opt:
+ invalid_cfgfile_opt(cli->cfgfile, section, name, value);
+ return false;
+}
+
static void
validate_sectorsize(
struct mkfs_params *cfg,
@@ -3630,9 +3696,8 @@ cfgfile_parse_ini(
{
struct cli_params *cli = user;
- fprintf(stderr, "Ini debug: file %s, section %s, name %s, value %s\n",
- cli->cfgfile, section, name, value);
-
+ if (!parse_cfgopt(section, name, value, cli))
+ return 0;
return 1;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/5] mkfs: hook up suboption parsing to ini files
2020-10-15 3:29 ` [PATCH 4/5] mkfs: hook up suboption parsing to ini files Dave Chinner
@ 2020-10-15 5:24 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:24PM +1100, 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.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> mkfs/xfs_mkfs.c | 95 +++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 99ce0dc48d3b..370ac6194e2f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -143,6 +143,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.
I don't understand this last sentence.
OH, I get it:
"This field is required to connect each opt_params (that is to say, each
option class) to a section in the config file."
(and maybe a note in copts that config files cannot specify other config
files, so ini_name is not necessary there.)
> + *
> * 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.
> @@ -201,6 +208,8 @@ enum {
> */
> struct opt_params {
> const char name;
> +#define MAX_INI_NAME_LEN 32
> + const char ini_name[MAX_INI_NAME_LEN];
How about "ini_section" ?
--D
> const char *subopts[MAX_SUBOPTS];
>
> struct subopt_param {
> @@ -228,6 +237,7 @@ static struct opt_params sopts;
>
> static struct opt_params bopts = {
> .name = 'b',
> + .ini_name = "block",
> .subopts = {
> [B_SIZE] = "size",
> },
> @@ -267,6 +277,7 @@ static struct opt_params copts = {
>
> static struct opt_params dopts = {
> .name = 'd',
> + .ini_name = "data",
> .subopts = {
> [D_AGCOUNT] = "agcount",
> [D_FILE] = "file",
> @@ -411,6 +422,7 @@ static struct opt_params dopts = {
>
> static struct opt_params iopts = {
> .name = 'i',
> + .ini_name = "inode",
> .subopts = {
> [I_ALIGN] = "align",
> [I_MAXPCT] = "maxpct",
> @@ -472,6 +484,7 @@ static struct opt_params iopts = {
>
> static struct opt_params lopts = {
> .name = 'l',
> + .ini_name = "log",
> .subopts = {
> [L_AGNUM] = "agnum",
> [L_INTERNAL] = "internal",
> @@ -571,6 +584,7 @@ static struct opt_params lopts = {
>
> static struct opt_params nopts = {
> .name = 'n',
> + .ini_name = "naming",
> .subopts = {
> [N_SIZE] = "size",
> [N_VERSION] = "version",
> @@ -602,6 +616,7 @@ static struct opt_params nopts = {
>
> static struct opt_params ropts = {
> .name = 'r',
> + .ini_name = "realtime",
> .subopts = {
> [R_EXTSIZE] = "extsize",
> [R_SIZE] = "size",
> @@ -652,6 +667,7 @@ static struct opt_params ropts = {
>
> static struct opt_params sopts = {
> .name = 's',
> + .ini_name = "sector",
> .subopts = {
> [S_SIZE] = "size",
> [S_SECTSIZE] = "sectsize",
> @@ -682,6 +698,7 @@ static struct opt_params sopts = {
>
> static struct opt_params mopts = {
> .name = 'm',
> + .ini_name = "metadata",
> .subopts = {
> [M_CRC] = "crc",
> [M_FINOBT] = "finobt",
> @@ -982,6 +999,17 @@ unknown(
> usage();
> }
>
> +static void
> +invalid_cfgfile_opt(
> + const char *filename,
> + const char *section,
> + const char *name,
> + const char *value)
> +{
> + fprintf(stderr, _("%s: invalid config file option: [%s]:%s=%s\n"),
> + filename, section, name, value);
> +}
> +
> static void
> check_device_type(
> const char *name,
> @@ -1696,23 +1724,22 @@ sector_opts_parser(
> }
>
> static struct subopts {
> - char opt;
> struct opt_params *opts;
> int (*parser)(struct opt_params *opts,
> int subopt,
> 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
> @@ -1726,7 +1753,7 @@ parse_subopts(
> int ret = 0;
>
> while (sop->opts) {
> - if (sop->opt == opt)
> + if (opt && sop->opts->name == opt)
> break;
> sop++;
> }
> @@ -1749,6 +1776,45 @@ parse_subopts(
> }
> }
>
> +static bool
> +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 invalid_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 invalid_opt;
> + return true;
> + }
> + }
> +invalid_opt:
> + invalid_cfgfile_opt(cli->cfgfile, section, name, value);
> + return false;
> +}
> +
> static void
> validate_sectorsize(
> struct mkfs_params *cfg,
> @@ -3630,9 +3696,8 @@ cfgfile_parse_ini(
> {
> struct cli_params *cli = user;
>
> - fprintf(stderr, "Ini debug: file %s, section %s, name %s, value %s\n",
> - cli->cfgfile, section, name, value);
> -
> + if (!parse_cfgopt(section, name, value, cli))
> + return 0;
> return 1;
> }
>
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] mkfs: document config files in mkfs.xfs(8)
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
` (3 preceding siblings ...)
2020-10-15 3:29 ` [PATCH 4/5] mkfs: hook up suboption parsing to ini files Dave Chinner
@ 2020-10-15 3:29 ` Dave Chinner
2020-10-15 5:36 ` Darrick J. Wong
2020-10-15 5:13 ` [PATCH 0/5] mkfs: Configuration file defined options Darrick J. Wong
5 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 3:29 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
So people know it exists.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
man/man8/mkfs.xfs.8 | 113 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 106 insertions(+), 7 deletions(-)
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 0a7858748457..f70d91d994fa 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -122,8 +122,46 @@ If the size of the block or sector is not specified, the default sizes
Many feature options allow an optional argument of 0 or 1, to explicitly
disable or enable the functionality.
.SH OPTIONS
+Options may be specified either on the command line or in a configuration file.
+Not all command line options can be specified in configuration files; only the
+command line options followed by a
+.B [section]
+label can be used in a configuration file.
+.PP
+Options that can be used in configuration files are grouped into related
+sections containing multiple options.
+Both the command line and configuration file formats use the same section and
+option grouping.
+Configuration file section names are described command line options in the list
+below.
+Option names and values are the same for both command line
+and configuration file specification.
+.PP
+Options specified are the combined set of command line parameters and
+configuration file parameters.
+Duplicated options will result in a respecification error, regardless of the
+location they were specified at.
+.TP
+.BI \-c " configuration_file_option"
+This option specifies the files that mkfs configuration will be obtained from.
+The valid
+.I configuration_file_option
+is:
+.RS 1.2i
.TP
+.BI options= name
+The configuration options will be sourced from the file specified by the
+.I name
+option string.
+This option can be use either an absolute or relative path to the configuration
+file to be read.
+.RE
+.PP
+.PD 0
.BI \-b " block_size_options"
+.TP
+.BI [block]
+.PD
This option specifies the fundamental block size of the filesystem.
The valid
.I block_size_option
@@ -141,8 +179,12 @@ Although
will accept any of these values and create a valid filesystem,
XFS on Linux can only mount filesystems with pagesize or smaller blocks.
.RE
-.TP
+.PP
+.PD 0
.BI \-m " global_metadata_options"
+.TP
+.BI [metadata]
+.PD
These options specify metadata format options that either apply to the entire
filesystem or aren't easily characterised by a specific functionality group. The
valid
@@ -243,8 +285,12 @@ reflink-enabled XFS filesystems. To use filesystem DAX with XFS, specify the
.B \-m reflink=0
option to mkfs.xfs to disable the reflink feature.
.RE
-.TP
+.PP
+.PD 0
.BI \-d " data_section_options"
+.TP
+.BI [data]
+.PD
These options specify the location, size, and other parameters of the
data section of the filesystem. The valid
.I data_section_options
@@ -416,8 +462,12 @@ By default,
.B mkfs.xfs
will not write to the device if it suspects that there is a filesystem
or partition table on the device already.
-.TP
+.PP
+.PD 0
.BI \-i " inode_options"
+.TP
+.BI [inode]
+.PD
This option specifies the inode size of the filesystem, and other
inode allocation parameters.
The XFS inode contains a fixed-size part and a variable-size part.
@@ -537,8 +587,12 @@ accommodate a chunk of 64 inodes. Without this feature enabled, inode
allocations can fail with out of space errors under severe fragmented
free space conditions.
.RE
-.TP
+.PP
+.PD 0
.BI \-l " log_section_options"
+.TP
+.BI [log]
+.PD
These options specify the location, size, and other parameters of the
log section of the filesystem. The valid
.I log_section_options
@@ -651,8 +705,12 @@ is 1 (on) so you must specify
if you want to disable this feature for older kernels which don't support
it.
.RE
-.TP
+.PP
+.PD 0
.BI \-n " naming_options"
+.TP
+.BI [naming]
+.PD
These options specify the version and size parameters for the naming
(directory) area of the filesystem. The valid
.I naming_options
@@ -858,8 +916,12 @@ to be constructed;
the
.B \-q
flag suppresses this.
-.TP
+.PP
+.PD 0
.BI \-r " realtime_section_options"
+.TP
+.BI [realtime]
+.PD
These options specify the location, size, and other parameters of the
real-time section of the filesystem. The valid
.I realtime_section_options
@@ -893,8 +955,12 @@ or logical volume containing the section.
This option disables stripe size detection, enforcing a realtime device with no
stripe geometry.
.RE
-.TP
+.PP
+.PD 0
.BI \-s " sector_size_options"
+.TP
+.BI [sector]
+.PD
This option specifies the fundamental sector size of the filesystem.
The valid
.I sector_size_option
@@ -933,6 +999,39 @@ Do not attempt to discard blocks at mkfs time.
.TP
.B \-V
Prints the version number and exits.
+.SH Configuration File Format
+The configuration file uses a basic INI format to specify sections and options
+within a section.
+Section and option names are case sensitive.
+Section names must not contain whitespace.
+Options are name-value pairs, ended by the first whitespace in the line.
+Option names cannot contain whitespace.
+Full line comments can be added by starting a line with a # symbol.
+If values contain whitespace, then it must be quoted.
+.PP
+The following example configuration file sets the block size to 4096 bytes,
+turns on reverse mapping btrees and sets the inode size to 2048 bytes.
+.PP
+.PD 0
+# Example mkfs.xfs configuration file
+.HP
+.HP
+[block]
+.HP
+size=4k
+.HP
+.HP
+[metadata]
+.HP
+rmapbt=1
+.HP
+.HP
+[inode]
+.HP
+size=2048
+.HP
+.PD
+.PP
.SH SEE ALSO
.BR xfs (5),
.BR mkfs (8),
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] mkfs: document config files in mkfs.xfs(8)
2020-10-15 3:29 ` [PATCH 5/5] mkfs: document config files in mkfs.xfs(8) Dave Chinner
@ 2020-10-15 5:36 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:36 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:25PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> So people know it exists.
Yay documentation!
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> man/man8/mkfs.xfs.8 | 113 +++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 106 insertions(+), 7 deletions(-)
>
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 0a7858748457..f70d91d994fa 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -122,8 +122,46 @@ If the size of the block or sector is not specified, the default sizes
> Many feature options allow an optional argument of 0 or 1, to explicitly
> disable or enable the functionality.
> .SH OPTIONS
> +Options may be specified either on the command line or in a configuration file.
> +Not all command line options can be specified in configuration files; only the
> +command line options followed by a
> +.B [section]
> +label can be used in a configuration file.
> +.PP
> +Options that can be used in configuration files are grouped into related
> +sections containing multiple options.
> +Both the command line and configuration file formats use the same section and
"The command line options and configuration files use the same option
sections and grouping."?
> +option grouping.
> +Configuration file section names are described command line options in the list
/me stumbles over "are described command line options in the list".
"Configuration file section names are listed in the command line option
sections below."?
> +below.
> +Option names and values are the same for both command line
> +and configuration file specification.
> +.PP
> +Options specified are the combined set of command line parameters and
> +configuration file parameters.
> +Duplicated options will result in a respecification error, regardless of the
> +location they were specified at.
> +.TP
> +.BI \-c " configuration_file_option"
> +This option specifies the files that mkfs configuration will be obtained from.
> +The valid
> +.I configuration_file_option
> +is:
> +.RS 1.2i
> .TP
> +.BI options= name
> +The configuration options will be sourced from the file specified by the
> +.I name
> +option string.
> +This option can be use either an absolute or relative path to the configuration
> +file to be read.
> +.RE
> +.PP
> +.PD 0
> .BI \-b " block_size_options"
> +.TP
> +.BI [block]
We might want to call this out explicitly as the name of the section in
the config file.
--D
> +.PD
> This option specifies the fundamental block size of the filesystem.
> The valid
> .I block_size_option
> @@ -141,8 +179,12 @@ Although
> will accept any of these values and create a valid filesystem,
> XFS on Linux can only mount filesystems with pagesize or smaller blocks.
> .RE
> -.TP
> +.PP
> +.PD 0
> .BI \-m " global_metadata_options"
> +.TP
> +.BI [metadata]
> +.PD
> These options specify metadata format options that either apply to the entire
> filesystem or aren't easily characterised by a specific functionality group. The
> valid
> @@ -243,8 +285,12 @@ reflink-enabled XFS filesystems. To use filesystem DAX with XFS, specify the
> .B \-m reflink=0
> option to mkfs.xfs to disable the reflink feature.
> .RE
> -.TP
> +.PP
> +.PD 0
> .BI \-d " data_section_options"
> +.TP
> +.BI [data]
> +.PD
> These options specify the location, size, and other parameters of the
> data section of the filesystem. The valid
> .I data_section_options
> @@ -416,8 +462,12 @@ By default,
> .B mkfs.xfs
> will not write to the device if it suspects that there is a filesystem
> or partition table on the device already.
> -.TP
> +.PP
> +.PD 0
> .BI \-i " inode_options"
> +.TP
> +.BI [inode]
> +.PD
> This option specifies the inode size of the filesystem, and other
> inode allocation parameters.
> The XFS inode contains a fixed-size part and a variable-size part.
> @@ -537,8 +587,12 @@ accommodate a chunk of 64 inodes. Without this feature enabled, inode
> allocations can fail with out of space errors under severe fragmented
> free space conditions.
> .RE
> -.TP
> +.PP
> +.PD 0
> .BI \-l " log_section_options"
> +.TP
> +.BI [log]
> +.PD
> These options specify the location, size, and other parameters of the
> log section of the filesystem. The valid
> .I log_section_options
> @@ -651,8 +705,12 @@ is 1 (on) so you must specify
> if you want to disable this feature for older kernels which don't support
> it.
> .RE
> -.TP
> +.PP
> +.PD 0
> .BI \-n " naming_options"
> +.TP
> +.BI [naming]
> +.PD
> These options specify the version and size parameters for the naming
> (directory) area of the filesystem. The valid
> .I naming_options
> @@ -858,8 +916,12 @@ to be constructed;
> the
> .B \-q
> flag suppresses this.
> -.TP
> +.PP
> +.PD 0
> .BI \-r " realtime_section_options"
> +.TP
> +.BI [realtime]
> +.PD
> These options specify the location, size, and other parameters of the
> real-time section of the filesystem. The valid
> .I realtime_section_options
> @@ -893,8 +955,12 @@ or logical volume containing the section.
> This option disables stripe size detection, enforcing a realtime device with no
> stripe geometry.
> .RE
> -.TP
> +.PP
> +.PD 0
> .BI \-s " sector_size_options"
> +.TP
> +.BI [sector]
> +.PD
> This option specifies the fundamental sector size of the filesystem.
> The valid
> .I sector_size_option
> @@ -933,6 +999,39 @@ Do not attempt to discard blocks at mkfs time.
> .TP
> .B \-V
> Prints the version number and exits.
> +.SH Configuration File Format
> +The configuration file uses a basic INI format to specify sections and options
> +within a section.
> +Section and option names are case sensitive.
> +Section names must not contain whitespace.
> +Options are name-value pairs, ended by the first whitespace in the line.
> +Option names cannot contain whitespace.
> +Full line comments can be added by starting a line with a # symbol.
> +If values contain whitespace, then it must be quoted.
> +.PP
> +The following example configuration file sets the block size to 4096 bytes,
> +turns on reverse mapping btrees and sets the inode size to 2048 bytes.
> +.PP
> +.PD 0
> +# Example mkfs.xfs configuration file
> +.HP
> +.HP
> +[block]
> +.HP
> +size=4k
> +.HP
> +.HP
> +[metadata]
> +.HP
> +rmapbt=1
> +.HP
> +.HP
> +[inode]
> +.HP
> +size=2048
> +.HP
> +.PD
> +.PP
> .SH SEE ALSO
> .BR xfs (5),
> .BR mkfs (8),
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mkfs: Configuration file defined options
2020-10-15 3:29 [PATCH 0/5] mkfs: Configuration file defined options Dave Chinner
` (4 preceding siblings ...)
2020-10-15 3:29 ` [PATCH 5/5] mkfs: document config files in mkfs.xfs(8) Dave Chinner
@ 2020-10-15 5:13 ` Darrick J. Wong
2020-10-15 5:32 ` Dave Chinner
5 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 02:29:20PM +1100, Dave Chinner wrote:
> Version 2:
>
> - "-c file=xxx" > "-c options=xxx"
> - split out constification into new patch
> - removed debug output
> - fixed some comments
> - added man page stuff
>
> 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 options=foo -d file=1,size=100m blah
> 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 options=foo -m crc=1 -d file=1,size=100m blah
> -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 config files, 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.
>
> However, 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.
>
> This has only been given a basic smoke testing right now (see above!
> :). I need to get Darrick's tests from the previous round of config
This was in the v1 series; have you gotten Darrick's fstests to do more
substantial testing? ;)
--D
> file bikeshedding working in my test environment to do more
> substantial testing of this....
>
> Cheers,
>
> Dave.
>
>
> Dave Chinner (5):
> build: add support for libinih for mkfs
> mkfs: add initial ini format config file parsing support
> mkfs: constify various strings
> mkfs: hook up suboption parsing to ini files
> mkfs: document config files in mkfs.xfs(8)
>
> configure.ac | 3 +
> doc/INSTALL | 5 +
> include/builddefs.in | 1 +
> include/linux.h | 2 +-
> m4/package_inih.m4 | 20 ++++
> man/man8/mkfs.xfs.8 | 113 +++++++++++++++++++--
> mkfs/Makefile | 2 +-
> mkfs/xfs_mkfs.c | 228 ++++++++++++++++++++++++++++++++++++++-----
> 8 files changed, 340 insertions(+), 34 deletions(-)
> create mode 100644 m4/package_inih.m4
>
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 0/5] mkfs: Configuration file defined options
2020-10-15 5:13 ` [PATCH 0/5] mkfs: Configuration file defined options Darrick J. Wong
@ 2020-10-15 5:32 ` Dave Chinner
2020-10-15 5:39 ` Darrick J. Wong
0 siblings, 1 reply; 15+ messages in thread
From: Dave Chinner @ 2020-10-15 5:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Oct 14, 2020 at 10:13:00PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 15, 2020 at 02:29:20PM +1100, Dave Chinner wrote:
> > Version 2:
> >
> > - "-c file=xxx" > "-c options=xxx"
> > - split out constification into new patch
> > - removed debug output
> > - fixed some comments
> > - added man page stuff
> >
> > 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 options=foo -d file=1,size=100m blah
> > 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 options=foo -m crc=1 -d file=1,size=100m blah
> > -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 config files, 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.
> >
> > However, 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.
> >
> > This has only been given a basic smoke testing right now (see above!
> > :). I need to get Darrick's tests from the previous round of config
>
> This was in the v1 series; have you gotten Darrick's fstests to do more
> substantial testing? ;)
I got as far as asking you "where did you get your INI format
specification from?" because the tests assume stuff that I don't
think is valid about whitespace and comment structure in the format.
And then I disappeared down a rathole of "there is no one true
specification for INI files" and then something else came up....
I have not got back to culling the whitespace craziness from those
tests yet. The only format I'm considering supporting is what the
library itself actually supports, and that means random whitespace
in names, values and section headers will be a bad configuration
file format error.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/5] mkfs: Configuration file defined options
2020-10-15 5:32 ` Dave Chinner
@ 2020-10-15 5:39 ` Darrick J. Wong
0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-10-15 5:39 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Oct 15, 2020 at 04:32:34PM +1100, Dave Chinner wrote:
> On Wed, Oct 14, 2020 at 10:13:00PM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 15, 2020 at 02:29:20PM +1100, Dave Chinner wrote:
> > > Version 2:
> > >
> > > - "-c file=xxx" > "-c options=xxx"
> > > - split out constification into new patch
> > > - removed debug output
> > > - fixed some comments
> > > - added man page stuff
> > >
> > > 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 options=foo -d file=1,size=100m blah
> > > 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 options=foo -m crc=1 -d file=1,size=100m blah
> > > -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 config files, 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.
> > >
> > > However, 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.
> > >
> > > This has only been given a basic smoke testing right now (see above!
> > > :). I need to get Darrick's tests from the previous round of config
> >
> > This was in the v1 series; have you gotten Darrick's fstests to do more
> > substantial testing? ;)
>
> I got as far as asking you "where did you get your INI format
> specification from?" because the tests assume stuff that I don't
> think is valid about whitespace and comment structure in the format.
> And then I disappeared down a rathole of "there is no one true
> specification for INI files" and then something else came up....
>
> I have not got back to culling the whitespace craziness from those
> tests yet. The only format I'm considering supporting is what the
> library itself actually supports, and that means random whitespace
> in names, values and section headers will be a bad configuration
> file format error.
Yeah. The ini format craziness was a direct result of me trying to
(re)interpret the format based on the line formats that the previous
round of patches coded into mkfs. Perhaps the only parts easily
salvageable are the ones that actually test the config file options
themselves...
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 15+ messages in thread