| Message ID | 20180529220603.29420-4-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote: > You may want to stick to specific set of configuration options when > creating filesystems with mkfs.xfs -- sometimes due to pure technical > reasons, but some other times to ensure systems remain compatible as > new features are introduced with older kernels, or if you always want > to take advantage of some new feature which would otherwise typically > be disruptive. > > This adds support for parsing a configuration file to override defaults > parameters to be used for mkfs.xfs. > > We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for > different configuration files, if none is specified we look for the > default configuration file, /etc/mkfs.xfs.d/default. You can override > with -c. For instance, if you specify: > > mkfs.xfs -c experimental -f /dev/loop0 > > The search path for the configuration file will be: > > 1) $PWD/experimental > 2) /etc/mkfs.xfs.d/experimental > > Absolute paths are supported, in which case they will be used directly > and the mkfs.xfs.d directory is ignored. > > To verify what configuration file is used on a system use the typical: > > mkfs.xfs -N > > There is only a subset of options allowed to be set on the configuration > file. The default parameters you can override on a configuration file and > their current built-in default settings are: > > [data] > noalign=0 > > [inode] > align=1 > projid32bit=1 > sparse=0 > > [log] > lazy-count=1 > > [metadata] > crc=1 > finobt=1 > rmapbt=0 > reflink=0 > > [naming] > ftype=1 > > [rtdev] > noalign=0 > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > configure.ac | 6 +- > include/builddefs.in | 2 + > man/man5/Makefile | 2 + > man/man5/mkfs.xfs.d.5.in | 151 ++++++++ > man/man8/Makefile | 2 + > man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} | 27 ++ > mkfs/Makefile | 2 +- > mkfs/config.c | 645 +++++++++++++++++++++++++++++++++ > mkfs/config.h | 10 +- > mkfs/xfs_mkfs.c | 76 +++- > 10 files changed, 909 insertions(+), 14 deletions(-) > create mode 100644 man/man5/mkfs.xfs.d.5.in > rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%) > create mode 100644 mkfs/config.c > > diff --git a/configure.ac b/configure.ac > index 508eefede073..94c5bda725f2 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -233,5 +233,9 @@ AC_CHECK_SIZEOF([char *]) > AC_TYPE_UMODE_T > AC_MANUAL_FORMAT > > -AC_CONFIG_FILES([include/builddefs]) > +AC_CONFIG_FILES([ > + include/builddefs > + man/man5/mkfs.xfs.d.5 > + man/man8/mkfs.xfs.8 > +]) > AC_OUTPUT > diff --git a/include/builddefs.in b/include/builddefs.in > index 8aac06cf90dc..e1ee9f7ba086 100644 > --- a/include/builddefs.in > +++ b/include/builddefs.in > @@ -62,6 +62,7 @@ PKG_LIB_DIR = @libdir@@libdirsuffix@ > PKG_INC_DIR = @includedir@/xfs > DK_INC_DIR = @includedir@/disk > PKG_MAN_DIR = @mandir@ > +PKG_ETC_DIR = @sysconfdir@ > PKG_DOC_DIR = @datadir@/doc/@pkg_name@ > PKG_LOCALE_DIR = @datadir@/locale > > @@ -196,6 +197,7 @@ endif > > GCFLAGS = $(DEBUG) \ > -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\" \ > + -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\" \ > -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs > > ifeq ($(ENABLE_GETTEXT),yes) > diff --git a/man/man5/Makefile b/man/man5/Makefile > index fe0aef6f016b..0b33122b064e 100644 > --- a/man/man5/Makefile > +++ b/man/man5/Makefile > @@ -19,3 +19,5 @@ install : default > $(INSTALL) -m 755 -d $(MAN_DEST) > $(INSTALL_MAN) > install-dev : > + > +LDIRT += mkfs.xfs.d.5 > diff --git a/man/man5/mkfs.xfs.d.5.in b/man/man5/mkfs.xfs.d.5.in > new file mode 100644 > index 000000000000..287877ce029d > --- /dev/null > +++ b/man/man5/mkfs.xfs.d.5.in > @@ -0,0 +1,151 @@ > +.TH mkfs.xfs.d 5 > +.SH NAME > +mkfs.xfs.d \- mkfs.xfs configuration directory > +.SH DESCRIPTION > +.B mkfs.xfs (8) > +uses a set of initial default parameters for configuration. > + > +The built-in mkfs defaults are decided by the XFS community. New features are > +only enabled by default when the community consider them stable. One can > +override these defaults on the > +.B mkfs.xfs (8) > +command line, but there are cases where it is desirable for the distro or the > +system administrator to establish their own supported defaults in a uniform > +manner, regardless of the version of > +.B mkfs.xfs (8) > +used. This may be desirable for example on systems with old kernels > +where the built-in default > +.B mkfs.xfs (8) > +parameters create a filesystem that is not supported by the old kernel. > +In such situations it would also be unclear what parameters are needed to > +produce a compatible filesystem, having a configuration file present ensures > +that if newer versions of > +.B mkfs.xfs (8) > +are deployed, creating a filesystem will remain compatible. Overriding > +.B mkfs.xfs (8) > +built-in defaults may also be desirable if you have a series of systems with > +different kernels and want to be able to create filesystems which all systems > +are able to support properly. > +.PP > +The XFS configuration directory > +.B mkfs.xfs.d (5) > +can be used as a home to define different configuration files which can be used > +to override the built-in default parameters by > +.B mkfs.xfs (8). > +If the > +.B -c > +parameter is not used, the default configuration file: > +.IP > +.I @sysconfdir@/mkfs.xfs.d/default > +.PP > +will be looked for first and if present will be used to override > +.B mkfs.xfs (8) > +built-in default parameters. > +.PP > +You can override the default configuration file by specifying its name when > +using > +.B mkfs.xfs (8) > +by using the > +.B -c > +parameter. > +.PP > +If the path does not start with a '.', the current working directory is > +searched for the file. If the file is not found there, the > +.B mkfs.xfs.d (5) > +directory is searched for the file. > +.PP > +If > +.B -c > +is used with relative path with which has a leading '.' character, the given > +path is used directly, so the configuration file will be relative to the > +current working directory. > +.PP > +If the > +.B -c > +argument starts with a '/', it is considered an absolute path, and opened. > +.PP > +For example: > +.IP > +.B mkfs.xfs -c experimental -f /dev/sda1 > +.PP > +Will make > +.B mkfs.xfs (8) > +look for the following configuration files and use the first one it finds: > +.IP > +.B $PWD/experimental > +.br > +.B @sysconfdir@/mkfs.xfs.d/experimental > +.PP > +If you used an absolute path, for example: > +.IP > +.B mkfs.xfs -c /tmp/experimental -f /dev/sda1 > +.PP > +Then only the configuration file /tmp/experimental will be looked for and > +used if present. > +.PP > +If you use the > +.B -c > +parameter the configuration file must be present and must parse correctly. > +.PP > +Parameters passed to the > +.B mkfs.xfs (8) > +command line always override any defaults set by the configuration file. > +.PP > +.B mkfs.xfs (8) > +will always describe what configuration file was used, if any > +was used at all. To verify which configuration file would be used prior to > +execution of > +.B mkfs.xfs (8) > +you can use > +.I mkfs.xfs -N. > +.PP > +.SH DEFAULT PARAMETERS > +Default parameters for > +.B mkfs.xfs (8) > +consists of a small subset of the parameters one can set with on the command > +line. Currently all default parameters can only be either enabled or disabled, > +you can set their value to 1 to enable or 0 to disable. Below we list the > +different supported default parameters which can be defined on configuration > +files, along with the current built-in setting. > +.PP > +.BI [data] > +.br > +.BI noalign=0 > +.PP > +.BI [inode] > +.br > +.BI align=1 > +.br > +.BI projid32bit=1 > +.br > +.BI sparse=0 > +.PP > +.BI [log] > +.br > +.BI lazy-count=1 > +.PP > +.BI [metadata] > +.br > +.BI crc=1 > +.br > +.BI finobt=1 > +.br > +.BI rmapbt=0 > +.br > +.BI reflink=0 > +.PP > +.BI [naming] > +.br > +.BI ftype=1 > +.PP > +.BI [rtdev] > +.br > +.BI noalign=0 > +.PP > +.SH SEE ALSO > +.BR mkfs.xfs (8), > +.BR xfsctl (3), > +.BR xfs_info (8), > +.BR xfs_admin (8), > +.BR xfsdump (8), > +.BR xfsrestore (8). > diff --git a/man/man8/Makefile b/man/man8/Makefile > index 36620da172ae..2a6548079997 100644 > --- a/man/man8/Makefile > +++ b/man/man8/Makefile > @@ -19,3 +19,5 @@ install : default > $(INSTALL) -m 755 -d $(MAN_DEST) > $(INSTALL_MAN) > install-dev : > + > +LDIRT += mkfs.xfs.8 > diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8.in > similarity index 96% > rename from man/man8/mkfs.xfs.8 > rename to man/man8/mkfs.xfs.8.in > index 4b8c78c37806..81e2753bd2b5 100644 > --- a/man/man8/mkfs.xfs.8 > +++ b/man/man8/mkfs.xfs.8.in > @@ -83,6 +83,24 @@ and > .B \-l internal \-l size=10m > are equivalent. > .PP > +An optional XFS configuration file directory > +.B mkfs.xfs.d (5) > +exists to help fine tune different default parameters which can be used when > +calling > +.B mkfs.xfs (8). > +If present, and if > +.B -c > +is not used, the default configuration file @sysconfigdir@/mkfs.xfs.d/default > +will be used to override system build-in defaults. Refer to mkfs.xfs.d (5) > +for a list of current defaults and further details. > +Command line arguments directly passed to > +.B mkfs.xfs (8) > +will always override parameters set in the configuration file. > +You can override the configuration file used by using the > +.B -c > +parameter, further explained below and in > +.B mkfs.xfs.d (5) > +.PP > In the descriptions below, sizes are given in sectors, bytes, blocks, > kilobytes, megabytes, gigabytes, etc. > Sizes are treated as hexadecimal if prefixed by 0x or 0X, > @@ -123,6 +141,14 @@ Many feature options allow an optional argument of 0 or 1, to explicitly > disable or enable the functionality. > .SH OPTIONS > .TP > +.BI \-c " configuration-file" > +Override the configuration file used. If a relative path is given the search > +path for the configuration file is your current directory and then the > +.B mkfs.xfs.d (5) > +directory. If an absolute path is given it is used directly. For more details > +refer to > +.B mkfs.xfs.d (5) > +.TP > .BI \-b " block_size_options" > This option specifies the fundamental block size of the filesystem. > The valid > @@ -923,6 +949,7 @@ Prints the version number and exits. > .SH SEE ALSO > .BR xfs (5), > .BR mkfs (8), > +.BR mkfs.xfs.d (5), > .BR mount (8), > .BR xfs_info (8), > .BR xfs_admin (8). > diff --git a/mkfs/Makefile b/mkfs/Makefile > index c84f9b6ae63b..c7815b3e106b 100644 > --- a/mkfs/Makefile > +++ b/mkfs/Makefile > @@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs > LTCOMMAND = mkfs.xfs > > HFILES = > -CFILES = proto.c xfs_mkfs.c > +CFILES = proto.c xfs_mkfs.c config.c > > LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \ > $(LIBUUID) > diff --git a/mkfs/config.c b/mkfs/config.c > new file mode 100644 > index 000000000000..0b4aebe51903 > --- /dev/null > +++ b/mkfs/config.c > @@ -0,0 +1,645 @@ > +/* > + * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it would be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. > + */ > + > +#include <ctype.h> > +#include <dirent.h> > +#include <fcntl.h> > + > +#include "libxfs.h" > +#include "config.h" > + > +/* > + * Enums for each configuration option. All these currently match the CLI > + * parameters for now but this may change later, so we keep all this code > + * and definitions separate. The rules for configuration parameters may also > + * differ. > + * > + * We only provide definitions for what we currently support parsing. > + */ > + > +enum data_subopts { > + D_NOALIGN = 0, > +}; > + > +enum inode_subopts { > + I_ALIGN = 0, > + I_PROJID32BIT, > + I_SPINODES, > +}; > + > +enum log_subopts { > + L_LAZYSBCNTR = 0, > +}; > + > +enum metadata_subopts { > + M_CRC = 0, > + M_FINOBT, > + M_RMAPBT, > + M_REFLINK, > +}; > + > +enum naming_subopts { > + N_FTYPE = 0, > +}; > + > +enum rtdev_subopts { > + R_NOALIGN = 0, > +}; > + > +/* Just define the max options array size manually right now */ > +#define MAX_SUBOPTS 4 > + > +static int > +data_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum data_subopts subopt = psubopt; > + > + switch (subopt) { > + case D_NOALIGN: > + dft->sb_feat.nodalign = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +inode_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum inode_subopts subopt = psubopt; > + > + switch (subopt) { > + case I_ALIGN: > + dft->sb_feat.inode_align = value; > + return 0; > + case I_PROJID32BIT: > + dft->sb_feat.projid32bit = value; > + return 0; > + case I_SPINODES: > + dft->sb_feat.spinodes = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +log_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum log_subopts subopt = psubopt; > + > + switch (subopt) { > + case L_LAZYSBCNTR: > + dft->sb_feat.lazy_sb_counters = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +metadata_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum metadata_subopts subopt = psubopt; > + > + switch (subopt) { > + case M_CRC: > + dft->sb_feat.crcs_enabled = value; > + if (dft->sb_feat.crcs_enabled) > + dft->sb_feat.dirftype = true; > + return 0; > + case M_FINOBT: > + dft->sb_feat.finobt = value; > + return 0; > + case M_RMAPBT: > + dft->sb_feat.rmapbt = value; > + return 0; > + case M_REFLINK: > + dft->sb_feat.reflink = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +naming_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum naming_subopts subopt = psubopt; > + > + switch (subopt) { > + case N_FTYPE: > + dft->sb_feat.dirftype = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +rtdev_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum rtdev_subopts subopt = psubopt; > + > + switch (subopt) { > + case R_NOALIGN: > + dft->sb_feat.nortalign = value; > + return 0; > + } > + return EINVAL; > +} > + > +struct confopts { > + const char *name; > + const char *subopts[MAX_SUBOPTS]; > + int (*parser)(struct mkfs_default_params *dft, > + int psubopt, uint64_t value); > + bool seen; > +} confopts_tab[] = { > + { > + .name = "data", > + .subopts = { > + [D_NOALIGN] = "noalign", > + }, > + .parser = data_config_parser, > + }, > + { > + .name = "inode", > + .subopts = { > + [I_ALIGN] = "align", > + [I_PROJID32BIT] = "projid32bit", > + [I_SPINODES] = "sparse", > + }, > + .parser = inode_config_parser, > + }, > + { > + .name = "log", > + .subopts = { > + [L_LAZYSBCNTR] = "lazy-count", > + }, > + .parser = log_config_parser, > + }, > + { > + .name = "naming", > + .subopts = { > + [N_FTYPE] = "ftype", > + }, > + .parser = naming_config_parser, > + }, > + { > + .name = "rtdev", > + .subopts = { > + [R_NOALIGN] = "noalign", > + }, > + .parser = rtdev_config_parser, > + }, > + { > + .name = "metadata", > + .subopts = { > + [M_CRC] = "crc", > + [M_FINOBT] = "finobt", > + [M_RMAPBT] = "rmapbt", > + [M_REFLINK] = "reflink", > + }, > + .parser = metadata_config_parser, > + }, > +}; > + > +static struct confopts * > +get_confopts( > + const char *section) > +{ > + unsigned int i; > + struct confopts *opts; > + > + for (i=0; i < ARRAY_SIZE(confopts_tab); i++) { > + opts = &confopts_tab[i]; > + if (!opts) > + return NULL; > + if (strcmp(opts->name, section) == 0) { > + return opts; > + } > + } > + return NULL; > +} > + > +enum parse_line_type { > + PARSE_COMMENT = 0, > + PARSE_EMPTY, > + PARSE_SECTION, > + PARSE_TAG_VALUE, > + PARSE_INVALID, > + PARSE_EOF, > +}; > + > +static bool > +isempty( > + const char *line, > + ssize_t linelen) > +{ > + ssize_t i = 0; > + char p; > + > + while (i != linelen) { > + p = line[i]; > + i++; > + > + /* tab or space */ > + if (isblank(p)) > + continue; > + else > + return false; > + } > + > + return true; > +} > + > +static bool > +iscomment( > + const char *line, > + ssize_t linelen) > +{ > + ssize_t i = 0; > + char p; > + > + while (i != linelen) { > + p = line[i]; > + i++; > + > + /* tab or space */ > + if (isblank(p)) > + continue; > + > + if (p == '#') > + return true; > + > + return false; > + } > + > + return false; > +} > + > +static int > +parse_line_section( > + const char *line, > + char **tag) > +{ > + return sscanf(line, " [%m[^]]]", tag); > +} > + > +static int > +parse_line_tag_value( > + const char *line, > + char **tag, > + uint64_t *value) > +{ > + return sscanf(line, " %m[^ \t=]" > + " = " > + "%lu ", > + tag, value); > +} > + > +static enum parse_line_type > +parse_get_line_type( > + const char *line, > + ssize_t linelen, > + char **tag, > + uint64_t *value) > +{ > + int ret; > + uint64_t u64_value; > + > + if (isempty(line, linelen)) > + return PARSE_EMPTY; > + > + if (iscomment(line, linelen)) > + return PARSE_COMMENT; > + > + ret = parse_line_section(line, tag); > + if (ret == 1) > + return PARSE_SECTION; > + > + if (ret == EOF) > + return PARSE_EOF; > + > + ret = parse_line_tag_value(line, tag, &u64_value); > + if (ret == 2) { > + *value = u64_value; > + > + return PARSE_TAG_VALUE; > + } > + > + if (ret == EOF) > + return PARSE_EOF; > + > + return PARSE_INVALID; > +} > + > +static int > +parse_config_stream( > + struct mkfs_default_params *dft, > + const char *config_file, > + FILE *fp) > +{ > + int ret; > + char *line = NULL; > + ssize_t linelen; > + size_t len = 0, lineno = 0; > + uint64_t value; > + enum parse_line_type parse_type; > + struct confopts *confopt = NULL; > + int subopt; > + > + while ((linelen = getline(&line, &len, fp)) != -1) { > + char *ignore_value; > + char *p, *tag = NULL; > + > + lineno++; > + > + /* > + * tag is allocated for us by scanf(), it must freed only on any > + * successful parse of a section or tag-value pair. > + */ > + parse_type = parse_get_line_type(line, linelen, &tag, &value); > + > + switch (parse_type) { > + case PARSE_EMPTY: > + case PARSE_COMMENT: > + /* Nothing tag to free for these */ > + continue; > + case PARSE_EOF: > + break; > + case PARSE_INVALID: > + ret = EINVAL; > + fprintf(stderr, _("Invalid line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + case PARSE_SECTION: > + confopt = get_confopts(tag); > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; > + } > + if (!confopt->subopts) { > + ret = EINVAL; > + fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; > + } > + if (confopt->seen) { > + ret = EINVAL; > + fprintf(stderr, _("Section '%s' respecified\n"), > + tag); > + free(tag); > + goto out; > + } > + confopt->seen = true; > + free(tag); > + break; > + case PARSE_TAG_VALUE: > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"), > + config_file, lineno, line); > + free(tag); > + goto out; > + } > + > + /* > + * We re-use the line buffer allocated by getline(), > + * however line must be kept pointing to its original > + * value to free it later. A separate pointer is needed > + * as getsubopt() will otherwise muck with the value > + * passed. > + */ > + p = line; > + > + /* > + * Trims white spaces. getsubopt() does not grok > + * white space, it would fail otherwise. > + */ > + snprintf(p, len, "%s=%lu", tag, value); > + > + /* Not needed anymore */ > + free(tag); > + > + /* > + * We only use getsubopt() to validate the possible > + * subopt, we already parsed the value and its already > + * in a more preferred data type. > + */ > + subopt = getsubopt(&p, (char **) confopt->subopts, > + &ignore_value); > + > + ret = confopt->parser(dft, subopt, value); > + if (ret) { > + fprintf(stderr, _("Error parsine line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + } > + > + break; > + } > + } > +out: > + /* We must free even if getline() failed */ > + free(line); > + return ret; > +} > + > +static const char *conf_paths[] = { > + ".", > + MKFS_XFS_CONF_DIR, > +}; > + > +/* > + * If the file is not found -1 is returned and errno set. Otherwise > + * the file descriptor is returned. > + */ > +int > +open_cli_config( > + char *cli_config_file, > + char **fpath) > +{ > + int fd, len; > + char *final_path = NULL; > + char *relative_path= NULL; > + unsigned int i; > + > + if (strlen(cli_config_file) > 2) { > + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '/') > + final_path = cli_config_file; > + else > + relative_path = cli_config_file; > + } else if (strlen(cli_config_file) == 1) { > + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { > + errno = EINVAL; > + return -1; > + } else > + relative_path = cli_config_file; > + } > + > + if (final_path) { > + fd = open(final_path, O_RDONLY); > + if (fd >= 0) > + memcpy(*fpath, final_path, strlen(final_path)); > + return fd; > + } > + > + /* We finally know the path is relative but just to be sure */ > + if (!relative_path) { > + errno = ENXIO; > + return -1; > + } > + > + for (i = 0; i < ARRAY_SIZE(conf_paths); i++) { > + memset(*fpath, 0, PATH_MAX); > + /* > + * For current directory evaluation, skip concatenating the > + * ./ from the file passed. We only concatenate for the other > + * paths we look up on. > + */ > + if (i == 0) > + memcpy(*fpath, relative_path, strlen(relative_path)); > + else { > + len = snprintf(*fpath, PATH_MAX, "%s/%s", conf_paths[i], > + relative_path); > + /* Indicates truncation */ > + if (len >= PATH_MAX) { > + errno = ENAMETOOLONG; > + return -1; > + } > + } > + fd = open(*fpath, O_RDONLY); > + if (fd < 0) > + continue; > + return fd; > + } > + > + errno = ENOENT; > + return -1; > +} > + > +/* > + * This is only called *iff* there is a configuration file which we know we > + * *must* parse. > + * > + * If default_fd is set and is a valid file descriptor then the configuration > + * file passed is the system default configuraiton file, and we already know > + * it exists. If default_fd is not set we assume we've been passed a > + * configuration file from the command line and must it must exist, otherwise > + * we have to error out. > + */ > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file) > +{ > + char *fpath; > + int fd; > + FILE *fp; > + int ret; > + struct stat sp; > + > + if (strlen(config_file) > PATH_MAX) > + return ENAMETOOLONG; > + > + fpath = malloc(PATH_MAX); > + if (!fpath) > + return ENOMEM; > + memset(fpath, 0, PATH_MAX); > + > + if (default_fd < 0) { > + fd = open_cli_config(config_file, &fpath); > + if (fd < 0) { > + free(fpath); > + return errno; > + } > + } else { > + fd = default_fd; > + memcpy(fpath, config_file, strlen(config_file)); > + } > + > + /* > + * At this point we know we have a valid file descriptor and have > + * figured out the path to the file used on fpath. Get the file stream > + * and do a bit of sanity checks before parsing the file. > + */ > + > + fp = fdopen(fd, "r"); > + if (!fp) { > + perror(fpath); It occurred to me just now (sorry...) that the caller of this function prints out a string with strerror() contents, so the perror here is unnecessary since the caller will cough up an error anyway. Then all of these constructions here become: fp = fdopen(...); if (!fp) return -1; ret = fstat(...); if (ret) goto out; if (S_ISDIR(...)) { errno = EISDIR; goto out; } ... return 0; out: return -1; and the call site now becomes: if (parse_defaults_file(...)) { fprintf(stderr, "%s: file is bad: %s\n", path, strerror(errno)); ... } Granted maybe we should just merge this and do all those cleanups separately. > + ret = errno; > + goto out_close_fd; > + } > + > + ret = fstat(fd, &sp); > + if (ret) { > + ret = errno; > + fprintf(stderr, _("Could not fstat() config file: %s: %s\n"), > + fpath, strerror(errno)); > + goto out; > + } > + > + if (S_ISDIR(sp.st_mode)) { > + ret = EBADF; ret = EISDIR? > + fprintf(stderr, _("Config file is a directory: %s\n"), fpath); > + goto out; > + } > + > + /* Anything beyond 1 MiB is kind of silly right now */ > + if (sp.st_size > 1 * 1024 * 1024) { > + ret = E2BIG; > + goto out; > + } > + > + ret = parse_config_stream(dft, config_file, fp); > + if (ret) > + goto out; > + > + printf(_("config-file=%s\n"), fpath); > + > +out: > + fclose(fp); > +out_close_fd: > + close(fd); > + free(fpath); > + return ret; > +} > diff --git a/mkfs/config.h b/mkfs/config.h > index e5ea968e2d65..0f429d9b7fd7 100644 > --- a/mkfs/config.h > +++ b/mkfs/config.h > @@ -19,6 +19,8 @@ > #ifndef _XFS_MKFS_CONFIG_H > #define _XFS_MKFS_CONFIG_H > > +#define MKFS_XFS_CONF_DIR ROOT_SYSCONFDIR "/mkfs.xfs.d" > + > struct fsxattr; > > /* > @@ -29,7 +31,7 @@ struct fsxattr; > * source can overriding the later source: > * > * o built-in defaults > - * o configuration file (XXX) > + * o configuration file > * o command line > * > * These values are not used directly - they are inputs into the mkfs geometry > @@ -75,4 +77,10 @@ struct mkfs_default_params { > struct fsxattr fsx; > }; > > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file); > + > #endif /* _XFS_MKFS_CONFIG_H */ > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 4664e507afbf..81ef7ab584ed 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3743,6 +3743,11 @@ main( > .nortalign = false, > }, > }; > + char *default_config = MKFS_XFS_CONF_DIR "/default"; > + char *cli_config_file = NULL; > + char *config_file = NULL; > + int default_fd = -1; > + int ret; > > platform_uuid_generate(&cli.uuid); > progname = basename(argv[0]); > @@ -3751,25 +3756,74 @@ main( > textdomain(PACKAGE); > > /* > - * TODO: Sourcing defaults from a config file > - * > * Before anything else, see if there's a config file with different > - * defaults. If a file exists in <package location>, read in the new > - * default values and overwrite them in the &dft structure. This way the > - * new defaults will apply before we parse the CLI, and the CLI will > - * still be able to override them. When more than one source is > - * implemented, emit a message to indicate where the defaults being > - * used came from. > + * defaults. If the CLI specified a full path we use and require that. > + * If a relative path was provided on the CLI we search the allowed > + * search paths for the file. If no config file was specified on the > + * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if > + * present, however this file is optional. > * > - * printf(_("Default configuration sourced from %s\n"), dft.source); > + * If a configuration file is found we use it to help overwrite default > + * values in the &dft structure. This way the new defaults will apply > + * before we parse the CLI, and the user will still be able to override > + * them through the CLI. > + */ > + > + /* > + * Pull config line options from command line > */ > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > + switch (c) { > + case 'c': > + if (cli_config_file) { > + fprintf(stderr, _("respecification of configuration not allowed\n")); > + exit(1); > + } > + cli_config_file = optarg; > + dft.source = _("command line"); > + break; > + default: > + continue; > + } > + } > + > + if (cli_config_file) > + config_file = cli_config_file; > + else { > + default_fd = open(default_config, O_RDONLY); > + if (default_fd >= 0) { > + dft.source = _("system default configuration file"); > + config_file = default_config; > + } > + } > + > + if (config_file) { > + /* If default_fd is set it will be closed for us */ > + ret = parse_defaults_file(&dft, default_fd, config_file); > + if (ret) { > + fprintf(stderr, _("Error parsing %s config file: %s : %s\n"), > + dft.source, config_file, > + strerror(ret)); > + exit(1); > + } > + } > > - /* copy new defaults into CLI parsing structure */ > + printf(_("Default configuration sourced from %s\n"), dft.source); > + > + /* > + * Done parsing defaults now, so memcpy defaults into CLI > + * structure, reset getopt and start parsing CLI options > + */ > memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat)); > memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx)); > > - while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > + platform_getoptreset(); > + > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > switch (c) { > + case 'c': > + /* already validated and parsed, ignore */ > + break; > case 'C': > case 'f': > force_overwrite = 1; > -- > 2.16.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/29/18 5:06 PM, Luis R. Rodriguez wrote: > +/* > + * If the file is not found -1 is returned and errno set. Otherwise > + * the file descriptor is returned. > + */ > +int > +open_cli_config( > + char *cli_config_file, > + char **fpath) > +{ > + int fd, len; > + char *final_path = NULL; > + char *relative_path= NULL; > + unsigned int i; > + > + if (strlen(cli_config_file) > 2) { > + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '/') > + final_path = cli_config_file; > + else > + relative_path = cli_config_file; > + } else if (strlen(cli_config_file) == 1) { > + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { > + errno = EINVAL; > + return -1; > + } else > + relative_path = cli_config_file; > + } so you have 2 cases, strlen > 2 and strlen == 1. What about == 2? # ls /a /a # mkfs/mkfs.xfs -c /a /dev/sdb1 Error parsing command line config file: /a : No such device or address so that looks like a bug here. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote: > You may want to stick to specific set of configuration options when > creating filesystems with mkfs.xfs -- sometimes due to pure technical > reasons, but some other times to ensure systems remain compatible as > new features are introduced with older kernels, or if you always want > to take advantage of some new feature which would otherwise typically > be disruptive. > > This adds support for parsing a configuration file to override defaults > parameters to be used for mkfs.xfs. ..... > +static int > +data_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum data_subopts subopt = psubopt; This is unnecessary. enums and ints are the same thing when it comes to comparisons. > + > + switch (subopt) { > + case D_NOALIGN: > + dft->sb_feat.nodalign = value; > + return 0; > + } > + return EINVAL; Negative errors, please. All xfsprogs use negative error numbers like the kernel now, so please don't take us back to the bad old days of having to change error signs depending on where the error came from.... Actually, I'm surprised that the compiler isn't complaining about not having a default statement in the switch statement. Can they be structured like the cli options to avoid this in future? switch (subopt) { case D_NOALIGN: ..... break; default: return -EINVAL; } return 0; > +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) { while (i < linelen) { > + p = line[i]; > + i++; p = line[i++]; > + > + /* tab or space */ > + if (isblank(p)) > + continue; > + else > + return false; > + } 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 int > +parse_line_section( > + const char *line, > + char **tag) > +{ > + return sscanf(line, " [%m[^]]]", tag); > +} > + > +static int > +parse_line_tag_value( > + const char *line, > + char **tag, > + uint64_t *value) > +{ > + return sscanf(line, " %m[^ \t=]" > + " = " > + "%lu ", > + tag, value); > +} %lu won't match uint64_t on 32 bit builds. Doesn't this need to be PRIu64 to be correct? Hmmm, I thought the compiler checked this format stuff and threw warnings when you get it wrong? Also, I'm not sure there's any value to these single line functions. Why not just call them directly in the next function? > + > +static enum parse_line_type > +parse_get_line_type( > + const char *line, > + ssize_t linelen, > + char **tag, > + uint64_t *value) > +{ > + int ret; > + uint64_t u64_value; > + > + if (isempty(line, linelen)) > + return PARSE_EMPTY; > + > + if (iscomment(line, linelen)) > + return PARSE_COMMENT; > + > + ret = parse_line_section(line, tag); > + if (ret == 1) > + return PARSE_SECTION; i.e. /* check if we have a section header */ ret = sscanf(line, " [%m[^]]]", tag); if (ret == 1) return PARSE_SECTION; > + > + if (ret == EOF) > + return PARSE_EOF; > + > + ret = parse_line_tag_value(line, tag, &u64_value); > + if (ret == 2) { > + *value = u64_value; > + > + return PARSE_TAG_VALUE; > + } /* should be a "tag = value" config option */ ret = sscanf(line, " %m[^ \t=] = %" PRIu64 " ", tag, value); if (ret == 2) return PARSE_TAG_VALUE; > + > + if (ret == EOF) > + return PARSE_EOF; > + > + return PARSE_INVALID; > +} > + > +static int > +parse_config_stream( > + struct mkfs_default_params *dft, > + const char *config_file, > + FILE *fp) > +{ > + int ret; > + char *line = NULL; > + ssize_t linelen; > + size_t len = 0, lineno = 0; > + uint64_t value; > + enum parse_line_type parse_type; > + struct confopts *confopt = NULL; > + int subopt; > + > + while ((linelen = getline(&line, &len, fp)) != -1) { > + char *ignore_value; > + char *p, *tag = NULL; > + > + lineno++; > + > + /* > + * tag is allocated for us by scanf(), it must freed only on any > + * successful parse of a section or tag-value pair. > + */ > + parse_type = parse_get_line_type(line, linelen, &tag, &value); > + > + switch (parse_type) { > + case PARSE_EMPTY: > + case PARSE_COMMENT: > + /* Nothing tag to free for these */ > + continue; > + case PARSE_EOF: > + break; > + case PARSE_INVALID: > + ret = EINVAL; Negative errors. > + fprintf(stderr, _("Invalid line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + case PARSE_SECTION: > + confopt = get_confopts(tag); > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; goto out_free_tag; .... out_free_tag: free(tag); ret = -EINVAL; goto out; Same for all thers others. > + } > + if (!confopt->subopts) { > + ret = EINVAL; > + fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; > + } > + if (confopt->seen) { > + ret = EINVAL; > + fprintf(stderr, _("Section '%s' respecified\n"), > + tag); > + free(tag); > + goto out; > + } > + confopt->seen = true; > + free(tag); > + break; > + case PARSE_TAG_VALUE: > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"), > + config_file, lineno, line); > + free(tag); > + goto out; > + } > + > + /* > + * We re-use the line buffer allocated by getline(), > + * however line must be kept pointing to its original > + * value to free it later. A separate pointer is needed > + * as getsubopt() will otherwise muck with the value > + * passed. > + */ > + p = line; > + > + /* > + * Trims white spaces. getsubopt() does not grok > + * white space, it would fail otherwise. > + */ > + snprintf(p, len, "%s=%lu", tag, value); > + > + /* Not needed anymore */ > + free(tag); > + > + /* > + * We only use getsubopt() to validate the possible > + * subopt, we already parsed the value and its already > + * in a more preferred data type. > + */ > + subopt = getsubopt(&p, (char **) confopt->subopts, > + &ignore_value); This hoop-jumping cruft can all be replaced with a simple loop that just compares the tag to the subopts array: for (i = 0; i < MAX_SUBOPTS; i++) { if (!confopt->subopts[i]) { /* option not found, error out */ goto out_free_tag; } if (strcmp(confopt->subopts[i], tag) == 0) break; } free(tag); ret = confopt->parser(dft, i, value); > + if (ret) { > + fprintf(stderr, _("Error parsine line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + } > + > + break; > + } > + } > +out: > + /* We must free even if getline() failed */ > + free(line); > + return ret; > +} > + > +static const char *conf_paths[] = { > + ".", > + MKFS_XFS_CONF_DIR, > +}; > + > +/* > + * If the file is not found -1 is returned and errno set. Otherwise > + * the file descriptor is returned. > + */ > +int > +open_cli_config( > + char *cli_config_file, > + char **fpath) > +{ > + int fd, len; > + char *final_path = NULL; > + char *relative_path= NULL; > + unsigned int i; > + > + if (strlen(cli_config_file) > 2) { > + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '/') > + final_path = cli_config_file; > + else > + relative_path = cli_config_file; > + } else if (strlen(cli_config_file) == 1) { > + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { > + errno = EINVAL; > + return -1; > + } else > + relative_path = cli_config_file; > + } This seems somewhat complex, and it doesn't check what the target of the path is. fstatat(AT_FDCWD, fpath, &st, AT_SYMLINK_NOFOLLOW); will tell us if the file exists, it doesn't end in a symlink and it will resolve both both relative and absolute paths correctly. If it succeeds, then we can check that the path points to a regular file (and not, say, a block device) so that this sort of thing: [30/5/18 12:08] <sandeen> mkfs/mkfs.xfs -c /dev/sdb1 [30/5/18 12:08] <sandeen> Invalid line /dev/sdb1:1 : XFSB Gives a sensible error before we get to parsing. If the file is found and isn't a regular file, then abort. If the file isn't found, then we need to ensure that we have been passed a name without any directory component as we are going to search the sysconf dir for that file. That's where basename(3) comes in - if the name returned by basename(3) doesn't match fpath, then there was a directory component in fpath, and we reject it and abort. Then we can open the sysconf directory w/ O_PATH|O_DIRECTORY, then do the same fstatat check and open it w/ openat(): fstatat(dirfd, fpath, &st, AT_SYMLINK_NOFOLLOW); /* check return */ openat(dirfd, fpath, O_NOFOLLOW, O_RDONLY); IOWs, I don't think we should be looking at just the first two characters of the supplied filename to determine the action to take, nor do i think we shoul dbe trying to resolve relative paths under the sysconf dir. > +/* > + * This is only called *iff* there is a configuration file which we know we > + * *must* parse. > + * > + * If default_fd is set and is a valid file descriptor then the configuration > + * file passed is the system default configuraiton file, and we already know > + * it exists. If default_fd is not set we assume we've been passed a > + * configuration file from the command line and must it must exist, otherwise > + * we have to error out. > + */ > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file) > +{ > + char *fpath; > + int fd; > + FILE *fp; > + int ret; > + struct stat sp; > + > + if (strlen(config_file) > PATH_MAX) > + return ENAMETOOLONG; > + > + fpath = malloc(PATH_MAX); > + if (!fpath) > + return ENOMEM; > + memset(fpath, 0, PATH_MAX); > + > + if (default_fd < 0) { > + fd = open_cli_config(config_file, &fpath); > + if (fd < 0) { > + free(fpath); > + return errno; > + } > + } else { > + fd = default_fd; > + memcpy(fpath, config_file, strlen(config_file)); > + } This is messy - opening the file shoul dbe split from parsing the config file. I make more comments about the reason below. But I think this should end up as: int open_config_file( const char *config_file, char **source) { /* * XXX: should asprintf the source string so it's got the * config_file opened in it. */ dft.source = _("Built in defaults"); dirfd = open(sysconfdir, O_PATH|O_NOFOLLOW); if (config_file) { fd = open_cli_config(dirfd, config_file) if (fd > 0) *source = _("CLI supplied file"); goto out; } fd = openat(dirfd, "default") if (fd > 0) *source = ("Package default config file") out: close(dirfd); return fd; } and so be completely separated from the act of parsing the config file. > + > + /* > + * At this point we know we have a valid file descriptor and have > + * figured out the path to the file used on fpath. Get the file stream > + * and do a bit of sanity checks before parsing the file. > + */ > + > + fp = fdopen(fd, "r"); > + if (!fp) { > + perror(fpath); > + ret = errno; > + goto out_close_fd; > + } > + > + ret = fstat(fd, &sp); > + if (ret) { > + ret = errno; > + fprintf(stderr, _("Could not fstat() config file: %s: %s\n"), > + fpath, strerror(errno)); > + goto out; > + } > + > + if (S_ISDIR(sp.st_mode)) { > + ret = EBADF; if (!S_ISREG(sp.st_mode)) { ret = -EINVAL; > + fprintf(stderr, _("Config file is a directory: %s\n"), fpath); > + goto out; > + } > + > + /* Anything beyond 1 MiB is kind of silly right now */ > + if (sp.st_size > 1 * 1024 * 1024) { > + ret = E2BIG; > + goto out; > + } As mentioned about, these checks should be done before we even open the file. > + ret = parse_config_stream(dft, config_file, fp); > + if (ret) > + goto out; > + > + printf(_("config-file=%s\n"), fpath); > + > +out: > + fclose(fp); > +out_close_fd: > + close(fd); > + free(fpath); > + return ret; > +} > diff --git a/mkfs/config.h b/mkfs/config.h > index e5ea968e2d65..0f429d9b7fd7 100644 > --- a/mkfs/config.h > +++ b/mkfs/config.h > @@ -19,6 +19,8 @@ > #ifndef _XFS_MKFS_CONFIG_H > #define _XFS_MKFS_CONFIG_H > > +#define MKFS_XFS_CONF_DIR ROOT_SYSCONFDIR "/mkfs.xfs.d" > + > struct fsxattr; > > /* > @@ -29,7 +31,7 @@ struct fsxattr; > * source can overriding the later source: > * > * o built-in defaults > - * o configuration file (XXX) > + * o configuration file > * o command line > * > * These values are not used directly - they are inputs into the mkfs geometry > @@ -75,4 +77,10 @@ struct mkfs_default_params { > struct fsxattr fsx; > }; > > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file); > + > #endif /* _XFS_MKFS_CONFIG_H */ > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 4664e507afbf..81ef7ab584ed 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3743,6 +3743,11 @@ main( > .nortalign = false, > }, > }; > + char *default_config = MKFS_XFS_CONF_DIR "/default"; > + char *cli_config_file = NULL; > + char *config_file = NULL; > + int default_fd = -1; > + int ret; > > platform_uuid_generate(&cli.uuid); > progname = basename(argv[0]); > @@ -3751,25 +3756,74 @@ main( > textdomain(PACKAGE); > > /* > - * TODO: Sourcing defaults from a config file > - * > * Before anything else, see if there's a config file with different > - * defaults. If a file exists in <package location>, read in the new > - * default values and overwrite them in the &dft structure. This way the > - * new defaults will apply before we parse the CLI, and the CLI will > - * still be able to override them. When more than one source is > - * implemented, emit a message to indicate where the defaults being > - * used came from. > + * defaults. If the CLI specified a full path we use and require that. > + * If a relative path was provided on the CLI we search the allowed > + * search paths for the file. If no config file was specified on the > + * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if > + * present, however this file is optional. > * > - * printf(_("Default configuration sourced from %s\n"), dft.source); > + * If a configuration file is found we use it to help overwrite default > + * values in the &dft structure. This way the new defaults will apply > + * before we parse the CLI, and the user will still be able to override > + * them through the CLI. > + */ > + > + /* > + * Pull config line options from command line > */ > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { Why specify all the parameters? We're only interested in the "c:" parameter here, so that's the only one we need to specify, right? Otherwise we've got two config option strings we need to keep i check... > + switch (c) { > + case 'c': > + if (cli_config_file) { > + fprintf(stderr, _("respecification of configuration not allowed\n")); > + exit(1); > + } > + cli_config_file = optarg; > + dft.source = _("command line"); > + break; > + default: > + continue; > + } > + } > + > + if (cli_config_file) > + config_file = cli_config_file; > + else { > + default_fd = open(default_config, O_RDONLY); > + if (default_fd >= 0) { > + dft.source = _("system default configuration file"); > + config_file = default_config; > + } > + } > + > + if (config_file) { > + /* If default_fd is set it will be closed for us */ > + ret = parse_defaults_file(&dft, default_fd, config_file); > + if (ret) { > + fprintf(stderr, _("Error parsing %s config file: %s : %s\n"), > + dft.source, config_file, > + strerror(ret)); > + exit(1); > + } > + } It doesn't make sense to me to open the default config file here before we know if it going to be needed. I would suggest that this get split into two parts - one to find and open the config file, the other to parse the opened file. i.e.: fd = open_config_file(cli_config_file, &dft.source); if (fd >= 0) { ret = parse_defaults_file(fd, &dft); .... close(fd); } Cheers, Dave.
Hi Luis. Luis R. Rodriguez - 30.05.18, 00:06: > You may want to stick to specific set of configuration options when > creating filesystems with mkfs.xfs -- sometimes due to pure technical > reasons, but some other times to ensure systems remain compatible as > new features are introduced with older kernels, or if you always want > to take advantage of some new feature which would otherwise typically > be disruptive. > > This adds support for parsing a configuration file to override > defaults parameters to be used for mkfs.xfs. > > We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow > for different configuration files, if none is specified we look for > the default configuration file, /etc/mkfs.xfs.d/default. You can > override with -c. For instance, if you specify: > > mkfs.xfs -c experimental -f /dev/loop0 Just two considerations (I am myself not sure ATM whether it makes sense to implement them): Usually for "*.d" directories in /etc all configuration files are parsed, yet you parse only one which makes perfect sense for the usecase, but may not be what the admin expects. So it may make sense to use a different directory name. Also in case you ever want to implement default mount options or whatever… it may make sense to use /etc/xfs as a base directory for everything (unless that is taken by something else, but I think X11 font server stuff is inside /etc/X11 if at all present). > The search path for the configuration file will be: > > 1) $PWD/experimental > 2) /etc/mkfs.xfs.d/experimental > > Absolute paths are supported, in which case they will be used directly > and the mkfs.xfs.d directory is ignored. > > To verify what configuration file is used on a system use the typical: > > mkfs.xfs -N > > There is only a subset of options allowed to be set on the > configuration file. The default parameters you can override on a > configuration file and their current built-in default settings are: […]
On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote: > You may want to stick to specific set of configuration options when > creating filesystems with mkfs.xfs -- sometimes due to pure technical > reasons, but some other times to ensure systems remain compatible as > new features are introduced with older kernels, or if you always want > to take advantage of some new feature which would otherwise typically > be disruptive. > > This adds support for parsing a configuration file to override defaults > parameters to be used for mkfs.xfs. > > We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for > different configuration files, if none is specified we look for the > default configuration file, /etc/mkfs.xfs.d/default. You can override > with -c. For instance, if you specify: > > mkfs.xfs -c experimental -f /dev/loop0 > > The search path for the configuration file will be: > > 1) $PWD/experimental > 2) /etc/mkfs.xfs.d/experimental > > Absolute paths are supported, in which case they will be used directly > and the mkfs.xfs.d directory is ignored. > > To verify what configuration file is used on a system use the typical: > > mkfs.xfs -N > > There is only a subset of options allowed to be set on the configuration > file. The default parameters you can override on a configuration file and > their current built-in default settings are: > > [data] > noalign=0 > > [inode] > align=1 > projid32bit=1 > sparse=0 > > [log] > lazy-count=1 > > [metadata] > crc=1 > finobt=1 > rmapbt=0 > reflink=0 > > [naming] > ftype=1 > > [rtdev] > noalign=0 > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> > --- > configure.ac | 6 +- > include/builddefs.in | 2 + > man/man5/Makefile | 2 + > man/man5/mkfs.xfs.d.5.in | 151 ++++++++ > man/man8/Makefile | 2 + > man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} | 27 ++ > mkfs/Makefile | 2 +- > mkfs/config.c | 645 +++++++++++++++++++++++++++++++++ > mkfs/config.h | 10 +- > mkfs/xfs_mkfs.c | 76 +++- > 10 files changed, 909 insertions(+), 14 deletions(-) > create mode 100644 man/man5/mkfs.xfs.d.5.in > rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%) > create mode 100644 mkfs/config.c > > diff --git a/configure.ac b/configure.ac > index 508eefede073..94c5bda725f2 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -233,5 +233,9 @@ AC_CHECK_SIZEOF([char *]) > AC_TYPE_UMODE_T > AC_MANUAL_FORMAT > > -AC_CONFIG_FILES([include/builddefs]) > +AC_CONFIG_FILES([ > + include/builddefs > + man/man5/mkfs.xfs.d.5 > + man/man8/mkfs.xfs.8 Wait a minute, AC_CONFIG_FILES is for generating configuration files to feed to the build. Using it for manpages means that the manpages are only regenerated from the source files at configure time, so if I edit mkfs.xfs.8.in and run make, the manpage doesn't get regenerated. I suggest a man/man[58]/Makefile rule similar to the one in scrub/Makefile that generates xfs_scrub_all. --D > +]) > AC_OUTPUT > diff --git a/include/builddefs.in b/include/builddefs.in > index 8aac06cf90dc..e1ee9f7ba086 100644 > --- a/include/builddefs.in > +++ b/include/builddefs.in > @@ -62,6 +62,7 @@ PKG_LIB_DIR = @libdir@@libdirsuffix@ > PKG_INC_DIR = @includedir@/xfs > DK_INC_DIR = @includedir@/disk > PKG_MAN_DIR = @mandir@ > +PKG_ETC_DIR = @sysconfdir@ > PKG_DOC_DIR = @datadir@/doc/@pkg_name@ > PKG_LOCALE_DIR = @datadir@/locale > > @@ -196,6 +197,7 @@ endif > > GCFLAGS = $(DEBUG) \ > -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\" \ > + -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\" \ > -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs > > ifeq ($(ENABLE_GETTEXT),yes) > diff --git a/man/man5/Makefile b/man/man5/Makefile > index fe0aef6f016b..0b33122b064e 100644 > --- a/man/man5/Makefile > +++ b/man/man5/Makefile > @@ -19,3 +19,5 @@ install : default > $(INSTALL) -m 755 -d $(MAN_DEST) > $(INSTALL_MAN) > install-dev : > + > +LDIRT += mkfs.xfs.d.5 > diff --git a/man/man5/mkfs.xfs.d.5.in b/man/man5/mkfs.xfs.d.5.in > new file mode 100644 > index 000000000000..287877ce029d > --- /dev/null > +++ b/man/man5/mkfs.xfs.d.5.in > @@ -0,0 +1,151 @@ > +.TH mkfs.xfs.d 5 > +.SH NAME > +mkfs.xfs.d \- mkfs.xfs configuration directory > +.SH DESCRIPTION > +.B mkfs.xfs (8) > +uses a set of initial default parameters for configuration. > + > +The built-in mkfs defaults are decided by the XFS community. New features are > +only enabled by default when the community consider them stable. One can > +override these defaults on the > +.B mkfs.xfs (8) > +command line, but there are cases where it is desirable for the distro or the > +system administrator to establish their own supported defaults in a uniform > +manner, regardless of the version of > +.B mkfs.xfs (8) > +used. This may be desirable for example on systems with old kernels > +where the built-in default > +.B mkfs.xfs (8) > +parameters create a filesystem that is not supported by the old kernel. > +In such situations it would also be unclear what parameters are needed to > +produce a compatible filesystem, having a configuration file present ensures > +that if newer versions of > +.B mkfs.xfs (8) > +are deployed, creating a filesystem will remain compatible. Overriding > +.B mkfs.xfs (8) > +built-in defaults may also be desirable if you have a series of systems with > +different kernels and want to be able to create filesystems which all systems > +are able to support properly. > +.PP > +The XFS configuration directory > +.B mkfs.xfs.d (5) > +can be used as a home to define different configuration files which can be used > +to override the built-in default parameters by > +.B mkfs.xfs (8). > +If the > +.B -c > +parameter is not used, the default configuration file: > +.IP > +.I @sysconfdir@/mkfs.xfs.d/default > +.PP > +will be looked for first and if present will be used to override > +.B mkfs.xfs (8) > +built-in default parameters. > +.PP > +You can override the default configuration file by specifying its name when > +using > +.B mkfs.xfs (8) > +by using the > +.B -c > +parameter. > +.PP > +If the path does not start with a '.', the current working directory is > +searched for the file. If the file is not found there, the > +.B mkfs.xfs.d (5) > +directory is searched for the file. > +.PP > +If > +.B -c > +is used with relative path with which has a leading '.' character, the given > +path is used directly, so the configuration file will be relative to the > +current working directory. > +.PP > +If the > +.B -c > +argument starts with a '/', it is considered an absolute path, and opened. > +.PP > +For example: > +.IP > +.B mkfs.xfs -c experimental -f /dev/sda1 > +.PP > +Will make > +.B mkfs.xfs (8) > +look for the following configuration files and use the first one it finds: > +.IP > +.B $PWD/experimental > +.br > +.B @sysconfdir@/mkfs.xfs.d/experimental > +.PP > +If you used an absolute path, for example: > +.IP > +.B mkfs.xfs -c /tmp/experimental -f /dev/sda1 > +.PP > +Then only the configuration file /tmp/experimental will be looked for and > +used if present. > +.PP > +If you use the > +.B -c > +parameter the configuration file must be present and must parse correctly. > +.PP > +Parameters passed to the > +.B mkfs.xfs (8) > +command line always override any defaults set by the configuration file. > +.PP > +.B mkfs.xfs (8) > +will always describe what configuration file was used, if any > +was used at all. To verify which configuration file would be used prior to > +execution of > +.B mkfs.xfs (8) > +you can use > +.I mkfs.xfs -N. > +.PP > +.SH DEFAULT PARAMETERS > +Default parameters for > +.B mkfs.xfs (8) > +consists of a small subset of the parameters one can set with on the command > +line. Currently all default parameters can only be either enabled or disabled, > +you can set their value to 1 to enable or 0 to disable. Below we list the > +different supported default parameters which can be defined on configuration > +files, along with the current built-in setting. > +.PP > +.BI [data] > +.br > +.BI noalign=0 > +.PP > +.BI [inode] > +.br > +.BI align=1 > +.br > +.BI projid32bit=1 > +.br > +.BI sparse=0 > +.PP > +.BI [log] > +.br > +.BI lazy-count=1 > +.PP > +.BI [metadata] > +.br > +.BI crc=1 > +.br > +.BI finobt=1 > +.br > +.BI rmapbt=0 > +.br > +.BI reflink=0 > +.PP > +.BI [naming] > +.br > +.BI ftype=1 > +.PP > +.BI [rtdev] > +.br > +.BI noalign=0 > +.PP > +.SH SEE ALSO > +.BR mkfs.xfs (8), > +.BR xfsctl (3), > +.BR xfs_info (8), > +.BR xfs_admin (8), > +.BR xfsdump (8), > +.BR xfsrestore (8). > diff --git a/man/man8/Makefile b/man/man8/Makefile > index 36620da172ae..2a6548079997 100644 > --- a/man/man8/Makefile > +++ b/man/man8/Makefile > @@ -19,3 +19,5 @@ install : default > $(INSTALL) -m 755 -d $(MAN_DEST) > $(INSTALL_MAN) > install-dev : > + > +LDIRT += mkfs.xfs.8 > diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8.in > similarity index 96% > rename from man/man8/mkfs.xfs.8 > rename to man/man8/mkfs.xfs.8.in > index 4b8c78c37806..81e2753bd2b5 100644 > --- a/man/man8/mkfs.xfs.8 > +++ b/man/man8/mkfs.xfs.8.in > @@ -83,6 +83,24 @@ and > .B \-l internal \-l size=10m > are equivalent. > .PP > +An optional XFS configuration file directory > +.B mkfs.xfs.d (5) > +exists to help fine tune different default parameters which can be used when > +calling > +.B mkfs.xfs (8). > +If present, and if > +.B -c > +is not used, the default configuration file @sysconfigdir@/mkfs.xfs.d/default > +will be used to override system build-in defaults. Refer to mkfs.xfs.d (5) > +for a list of current defaults and further details. > +Command line arguments directly passed to > +.B mkfs.xfs (8) > +will always override parameters set in the configuration file. > +You can override the configuration file used by using the > +.B -c > +parameter, further explained below and in > +.B mkfs.xfs.d (5) > +.PP > In the descriptions below, sizes are given in sectors, bytes, blocks, > kilobytes, megabytes, gigabytes, etc. > Sizes are treated as hexadecimal if prefixed by 0x or 0X, > @@ -123,6 +141,14 @@ Many feature options allow an optional argument of 0 or 1, to explicitly > disable or enable the functionality. > .SH OPTIONS > .TP > +.BI \-c " configuration-file" > +Override the configuration file used. If a relative path is given the search > +path for the configuration file is your current directory and then the > +.B mkfs.xfs.d (5) > +directory. If an absolute path is given it is used directly. For more details > +refer to > +.B mkfs.xfs.d (5) > +.TP > .BI \-b " block_size_options" > This option specifies the fundamental block size of the filesystem. > The valid > @@ -923,6 +949,7 @@ Prints the version number and exits. > .SH SEE ALSO > .BR xfs (5), > .BR mkfs (8), > +.BR mkfs.xfs.d (5), > .BR mount (8), > .BR xfs_info (8), > .BR xfs_admin (8). > diff --git a/mkfs/Makefile b/mkfs/Makefile > index c84f9b6ae63b..c7815b3e106b 100644 > --- a/mkfs/Makefile > +++ b/mkfs/Makefile > @@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs > LTCOMMAND = mkfs.xfs > > HFILES = > -CFILES = proto.c xfs_mkfs.c > +CFILES = proto.c xfs_mkfs.c config.c > > LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \ > $(LIBUUID) > diff --git a/mkfs/config.c b/mkfs/config.c > new file mode 100644 > index 000000000000..0b4aebe51903 > --- /dev/null > +++ b/mkfs/config.c > @@ -0,0 +1,645 @@ > +/* > + * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it would be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. > + */ > + > +#include <ctype.h> > +#include <dirent.h> > +#include <fcntl.h> > + > +#include "libxfs.h" > +#include "config.h" > + > +/* > + * Enums for each configuration option. All these currently match the CLI > + * parameters for now but this may change later, so we keep all this code > + * and definitions separate. The rules for configuration parameters may also > + * differ. > + * > + * We only provide definitions for what we currently support parsing. > + */ > + > +enum data_subopts { > + D_NOALIGN = 0, > +}; > + > +enum inode_subopts { > + I_ALIGN = 0, > + I_PROJID32BIT, > + I_SPINODES, > +}; > + > +enum log_subopts { > + L_LAZYSBCNTR = 0, > +}; > + > +enum metadata_subopts { > + M_CRC = 0, > + M_FINOBT, > + M_RMAPBT, > + M_REFLINK, > +}; > + > +enum naming_subopts { > + N_FTYPE = 0, > +}; > + > +enum rtdev_subopts { > + R_NOALIGN = 0, > +}; > + > +/* Just define the max options array size manually right now */ > +#define MAX_SUBOPTS 4 > + > +static int > +data_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum data_subopts subopt = psubopt; > + > + switch (subopt) { > + case D_NOALIGN: > + dft->sb_feat.nodalign = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +inode_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum inode_subopts subopt = psubopt; > + > + switch (subopt) { > + case I_ALIGN: > + dft->sb_feat.inode_align = value; > + return 0; > + case I_PROJID32BIT: > + dft->sb_feat.projid32bit = value; > + return 0; > + case I_SPINODES: > + dft->sb_feat.spinodes = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +log_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum log_subopts subopt = psubopt; > + > + switch (subopt) { > + case L_LAZYSBCNTR: > + dft->sb_feat.lazy_sb_counters = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +metadata_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum metadata_subopts subopt = psubopt; > + > + switch (subopt) { > + case M_CRC: > + dft->sb_feat.crcs_enabled = value; > + if (dft->sb_feat.crcs_enabled) > + dft->sb_feat.dirftype = true; > + return 0; > + case M_FINOBT: > + dft->sb_feat.finobt = value; > + return 0; > + case M_RMAPBT: > + dft->sb_feat.rmapbt = value; > + return 0; > + case M_REFLINK: > + dft->sb_feat.reflink = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +naming_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum naming_subopts subopt = psubopt; > + > + switch (subopt) { > + case N_FTYPE: > + dft->sb_feat.dirftype = value; > + return 0; > + } > + return EINVAL; > +} > + > +static int > +rtdev_config_parser( > + struct mkfs_default_params *dft, > + int psubopt, > + uint64_t value) > +{ > + enum rtdev_subopts subopt = psubopt; > + > + switch (subopt) { > + case R_NOALIGN: > + dft->sb_feat.nortalign = value; > + return 0; > + } > + return EINVAL; > +} > + > +struct confopts { > + const char *name; > + const char *subopts[MAX_SUBOPTS]; > + int (*parser)(struct mkfs_default_params *dft, > + int psubopt, uint64_t value); > + bool seen; > +} confopts_tab[] = { > + { > + .name = "data", > + .subopts = { > + [D_NOALIGN] = "noalign", > + }, > + .parser = data_config_parser, > + }, > + { > + .name = "inode", > + .subopts = { > + [I_ALIGN] = "align", > + [I_PROJID32BIT] = "projid32bit", > + [I_SPINODES] = "sparse", > + }, > + .parser = inode_config_parser, > + }, > + { > + .name = "log", > + .subopts = { > + [L_LAZYSBCNTR] = "lazy-count", > + }, > + .parser = log_config_parser, > + }, > + { > + .name = "naming", > + .subopts = { > + [N_FTYPE] = "ftype", > + }, > + .parser = naming_config_parser, > + }, > + { > + .name = "rtdev", > + .subopts = { > + [R_NOALIGN] = "noalign", > + }, > + .parser = rtdev_config_parser, > + }, > + { > + .name = "metadata", > + .subopts = { > + [M_CRC] = "crc", > + [M_FINOBT] = "finobt", > + [M_RMAPBT] = "rmapbt", > + [M_REFLINK] = "reflink", > + }, > + .parser = metadata_config_parser, > + }, > +}; > + > +static struct confopts * > +get_confopts( > + const char *section) > +{ > + unsigned int i; > + struct confopts *opts; > + > + for (i=0; i < ARRAY_SIZE(confopts_tab); i++) { > + opts = &confopts_tab[i]; > + if (!opts) > + return NULL; > + if (strcmp(opts->name, section) == 0) { > + return opts; > + } > + } > + return NULL; > +} > + > +enum parse_line_type { > + PARSE_COMMENT = 0, > + PARSE_EMPTY, > + PARSE_SECTION, > + PARSE_TAG_VALUE, > + PARSE_INVALID, > + PARSE_EOF, > +}; > + > +static bool > +isempty( > + const char *line, > + ssize_t linelen) > +{ > + ssize_t i = 0; > + char p; > + > + while (i != linelen) { > + p = line[i]; > + i++; > + > + /* tab or space */ > + if (isblank(p)) > + continue; > + else > + return false; > + } > + > + return true; > +} > + > +static bool > +iscomment( > + const char *line, > + ssize_t linelen) > +{ > + ssize_t i = 0; > + char p; > + > + while (i != linelen) { > + p = line[i]; > + i++; > + > + /* tab or space */ > + if (isblank(p)) > + continue; > + > + if (p == '#') > + return true; > + > + return false; > + } > + > + return false; > +} > + > +static int > +parse_line_section( > + const char *line, > + char **tag) > +{ > + return sscanf(line, " [%m[^]]]", tag); > +} > + > +static int > +parse_line_tag_value( > + const char *line, > + char **tag, > + uint64_t *value) > +{ > + return sscanf(line, " %m[^ \t=]" > + " = " > + "%lu ", > + tag, value); > +} > + > +static enum parse_line_type > +parse_get_line_type( > + const char *line, > + ssize_t linelen, > + char **tag, > + uint64_t *value) > +{ > + int ret; > + uint64_t u64_value; > + > + if (isempty(line, linelen)) > + return PARSE_EMPTY; > + > + if (iscomment(line, linelen)) > + return PARSE_COMMENT; > + > + ret = parse_line_section(line, tag); > + if (ret == 1) > + return PARSE_SECTION; > + > + if (ret == EOF) > + return PARSE_EOF; > + > + ret = parse_line_tag_value(line, tag, &u64_value); > + if (ret == 2) { > + *value = u64_value; > + > + return PARSE_TAG_VALUE; > + } > + > + if (ret == EOF) > + return PARSE_EOF; > + > + return PARSE_INVALID; > +} > + > +static int > +parse_config_stream( > + struct mkfs_default_params *dft, > + const char *config_file, > + FILE *fp) > +{ > + int ret; > + char *line = NULL; > + ssize_t linelen; > + size_t len = 0, lineno = 0; > + uint64_t value; > + enum parse_line_type parse_type; > + struct confopts *confopt = NULL; > + int subopt; > + > + while ((linelen = getline(&line, &len, fp)) != -1) { > + char *ignore_value; > + char *p, *tag = NULL; > + > + lineno++; > + > + /* > + * tag is allocated for us by scanf(), it must freed only on any > + * successful parse of a section or tag-value pair. > + */ > + parse_type = parse_get_line_type(line, linelen, &tag, &value); > + > + switch (parse_type) { > + case PARSE_EMPTY: > + case PARSE_COMMENT: > + /* Nothing tag to free for these */ > + continue; > + case PARSE_EOF: > + break; > + case PARSE_INVALID: > + ret = EINVAL; > + fprintf(stderr, _("Invalid line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + case PARSE_SECTION: > + confopt = get_confopts(tag); > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; > + } > + if (!confopt->subopts) { > + ret = EINVAL; > + fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"), > + config_file, lineno, tag); > + free(tag); > + goto out; > + } > + if (confopt->seen) { > + ret = EINVAL; > + fprintf(stderr, _("Section '%s' respecified\n"), > + tag); > + free(tag); > + goto out; > + } > + confopt->seen = true; > + free(tag); > + break; > + case PARSE_TAG_VALUE: > + if (!confopt) { > + ret = EINVAL; > + fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"), > + config_file, lineno, line); > + free(tag); > + goto out; > + } > + > + /* > + * We re-use the line buffer allocated by getline(), > + * however line must be kept pointing to its original > + * value to free it later. A separate pointer is needed > + * as getsubopt() will otherwise muck with the value > + * passed. > + */ > + p = line; > + > + /* > + * Trims white spaces. getsubopt() does not grok > + * white space, it would fail otherwise. > + */ > + snprintf(p, len, "%s=%lu", tag, value); > + > + /* Not needed anymore */ > + free(tag); > + > + /* > + * We only use getsubopt() to validate the possible > + * subopt, we already parsed the value and its already > + * in a more preferred data type. > + */ > + subopt = getsubopt(&p, (char **) confopt->subopts, > + &ignore_value); > + > + ret = confopt->parser(dft, subopt, value); > + if (ret) { > + fprintf(stderr, _("Error parsine line %s:%zu : %s\n"), > + config_file, lineno, line); > + goto out; > + } > + > + break; > + } > + } > +out: > + /* We must free even if getline() failed */ > + free(line); > + return ret; > +} > + > +static const char *conf_paths[] = { > + ".", > + MKFS_XFS_CONF_DIR, > +}; > + > +/* > + * If the file is not found -1 is returned and errno set. Otherwise > + * the file descriptor is returned. > + */ > +int > +open_cli_config( > + char *cli_config_file, > + char **fpath) > +{ > + int fd, len; > + char *final_path = NULL; > + char *relative_path= NULL; > + unsigned int i; > + > + if (strlen(cli_config_file) > 2) { > + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') > + final_path = cli_config_file; > + else if (cli_config_file[0] == '/') > + final_path = cli_config_file; > + else > + relative_path = cli_config_file; > + } else if (strlen(cli_config_file) == 1) { > + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { > + errno = EINVAL; > + return -1; > + } else > + relative_path = cli_config_file; > + } > + > + if (final_path) { > + fd = open(final_path, O_RDONLY); > + if (fd >= 0) > + memcpy(*fpath, final_path, strlen(final_path)); > + return fd; > + } > + > + /* We finally know the path is relative but just to be sure */ > + if (!relative_path) { > + errno = ENXIO; > + return -1; > + } > + > + for (i = 0; i < ARRAY_SIZE(conf_paths); i++) { > + memset(*fpath, 0, PATH_MAX); > + /* > + * For current directory evaluation, skip concatenating the > + * ./ from the file passed. We only concatenate for the other > + * paths we look up on. > + */ > + if (i == 0) > + memcpy(*fpath, relative_path, strlen(relative_path)); > + else { > + len = snprintf(*fpath, PATH_MAX, "%s/%s", conf_paths[i], > + relative_path); > + /* Indicates truncation */ > + if (len >= PATH_MAX) { > + errno = ENAMETOOLONG; > + return -1; > + } > + } > + fd = open(*fpath, O_RDONLY); > + if (fd < 0) > + continue; > + return fd; > + } > + > + errno = ENOENT; > + return -1; > +} > + > +/* > + * This is only called *iff* there is a configuration file which we know we > + * *must* parse. > + * > + * If default_fd is set and is a valid file descriptor then the configuration > + * file passed is the system default configuraiton file, and we already know > + * it exists. If default_fd is not set we assume we've been passed a > + * configuration file from the command line and must it must exist, otherwise > + * we have to error out. > + */ > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file) > +{ > + char *fpath; > + int fd; > + FILE *fp; > + int ret; > + struct stat sp; > + > + if (strlen(config_file) > PATH_MAX) > + return ENAMETOOLONG; > + > + fpath = malloc(PATH_MAX); > + if (!fpath) > + return ENOMEM; > + memset(fpath, 0, PATH_MAX); > + > + if (default_fd < 0) { > + fd = open_cli_config(config_file, &fpath); > + if (fd < 0) { > + free(fpath); > + return errno; > + } > + } else { > + fd = default_fd; > + memcpy(fpath, config_file, strlen(config_file)); > + } > + > + /* > + * At this point we know we have a valid file descriptor and have > + * figured out the path to the file used on fpath. Get the file stream > + * and do a bit of sanity checks before parsing the file. > + */ > + > + fp = fdopen(fd, "r"); > + if (!fp) { > + perror(fpath); > + ret = errno; > + goto out_close_fd; > + } > + > + ret = fstat(fd, &sp); > + if (ret) { > + ret = errno; > + fprintf(stderr, _("Could not fstat() config file: %s: %s\n"), > + fpath, strerror(errno)); > + goto out; > + } > + > + if (S_ISDIR(sp.st_mode)) { > + ret = EBADF; > + fprintf(stderr, _("Config file is a directory: %s\n"), fpath); > + goto out; > + } > + > + /* Anything beyond 1 MiB is kind of silly right now */ > + if (sp.st_size > 1 * 1024 * 1024) { > + ret = E2BIG; > + goto out; > + } > + > + ret = parse_config_stream(dft, config_file, fp); > + if (ret) > + goto out; > + > + printf(_("config-file=%s\n"), fpath); > + > +out: > + fclose(fp); > +out_close_fd: > + close(fd); > + free(fpath); > + return ret; > +} > diff --git a/mkfs/config.h b/mkfs/config.h > index e5ea968e2d65..0f429d9b7fd7 100644 > --- a/mkfs/config.h > +++ b/mkfs/config.h > @@ -19,6 +19,8 @@ > #ifndef _XFS_MKFS_CONFIG_H > #define _XFS_MKFS_CONFIG_H > > +#define MKFS_XFS_CONF_DIR ROOT_SYSCONFDIR "/mkfs.xfs.d" > + > struct fsxattr; > > /* > @@ -29,7 +31,7 @@ struct fsxattr; > * source can overriding the later source: > * > * o built-in defaults > - * o configuration file (XXX) > + * o configuration file > * o command line > * > * These values are not used directly - they are inputs into the mkfs geometry > @@ -75,4 +77,10 @@ struct mkfs_default_params { > struct fsxattr fsx; > }; > > +int > +parse_defaults_file( > + struct mkfs_default_params *dft, > + int default_fd, > + char *config_file); > + > #endif /* _XFS_MKFS_CONFIG_H */ > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 4664e507afbf..81ef7ab584ed 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3743,6 +3743,11 @@ main( > .nortalign = false, > }, > }; > + char *default_config = MKFS_XFS_CONF_DIR "/default"; > + char *cli_config_file = NULL; > + char *config_file = NULL; > + int default_fd = -1; > + int ret; > > platform_uuid_generate(&cli.uuid); > progname = basename(argv[0]); > @@ -3751,25 +3756,74 @@ main( > textdomain(PACKAGE); > > /* > - * TODO: Sourcing defaults from a config file > - * > * Before anything else, see if there's a config file with different > - * defaults. If a file exists in <package location>, read in the new > - * default values and overwrite them in the &dft structure. This way the > - * new defaults will apply before we parse the CLI, and the CLI will > - * still be able to override them. When more than one source is > - * implemented, emit a message to indicate where the defaults being > - * used came from. > + * defaults. If the CLI specified a full path we use and require that. > + * If a relative path was provided on the CLI we search the allowed > + * search paths for the file. If no config file was specified on the > + * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if > + * present, however this file is optional. > * > - * printf(_("Default configuration sourced from %s\n"), dft.source); > + * If a configuration file is found we use it to help overwrite default > + * values in the &dft structure. This way the new defaults will apply > + * before we parse the CLI, and the user will still be able to override > + * them through the CLI. > + */ > + > + /* > + * Pull config line options from command line > */ > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > + switch (c) { > + case 'c': > + if (cli_config_file) { > + fprintf(stderr, _("respecification of configuration not allowed\n")); > + exit(1); > + } > + cli_config_file = optarg; > + dft.source = _("command line"); > + break; > + default: > + continue; > + } > + } > + > + if (cli_config_file) > + config_file = cli_config_file; > + else { > + default_fd = open(default_config, O_RDONLY); > + if (default_fd >= 0) { > + dft.source = _("system default configuration file"); > + config_file = default_config; > + } > + } > + > + if (config_file) { > + /* If default_fd is set it will be closed for us */ > + ret = parse_defaults_file(&dft, default_fd, config_file); > + if (ret) { > + fprintf(stderr, _("Error parsing %s config file: %s : %s\n"), > + dft.source, config_file, > + strerror(ret)); > + exit(1); > + } > + } > > - /* copy new defaults into CLI parsing structure */ > + printf(_("Default configuration sourced from %s\n"), dft.source); > + > + /* > + * Done parsing defaults now, so memcpy defaults into CLI > + * structure, reset getopt and start parsing CLI options > + */ > memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat)); > memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx)); > > - while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > + platform_getoptreset(); > + > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > switch (c) { > + case 'c': > + /* already validated and parsed, ignore */ > + break; > case 'C': > case 'f': > force_overwrite = 1; > -- > 2.16.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 30, 2018 at 01:33:29PM +1000, Dave Chinner wrote: > On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote: > > You may want to stick to specific set of configuration options when > > creating filesystems with mkfs.xfs -- sometimes due to pure technical > > reasons, but some other times to ensure systems remain compatible as > > new features are introduced with older kernels, or if you always want > > to take advantage of some new feature which would otherwise typically > > be disruptive. > > > > This adds support for parsing a configuration file to override defaults > > parameters to be used for mkfs.xfs. > ..... > > +static int > > +data_config_parser( > > + struct mkfs_default_params *dft, > > + int psubopt, > > + uint64_t value) > > +{ > > + enum data_subopts subopt = psubopt; > > This is unnecessary. enums and ints are the same thing when it comes > to comparisons. Yes but I have a reason to use enum: if you expand on an enum entry for a subopt, the compiler will warn if the respective subopt parser fails to have an entry for it. This is possible when and if you have no default. This believe this is purely a bikeshed coding preference style, but its what I prefer. Let me know what you prefer. > > + > > + switch (subopt) { > > + case D_NOALIGN: > > + dft->sb_feat.nodalign = value; > > + return 0; > > + } > > + return EINVAL; > > Negative errors, please. All xfsprogs use negative error numbers > like the kernel now, so please don't take us back to the bad old > days of having to change error signs depending on where the error > came from.... OK! I used positive values as we want these errors then to be described later with strerror(ret) but right now this return value comes from what parse_defaults_file() returns, I suppose I can just set errno to the positive values and return -1 or whatever. So the caller as Darrick suggests: if (parse_defaults_file()) { fprintf(stderr, _(fError parsing %s config file: %s : %s\n") dft.source, config_file, strerror(ret)); ... } > Actually, I'm surprised that the compiler isn't complaining about > not having a default statement in the switch statement. Its not complaining as *all* the enums for each type are addressed :) > Can they be > structured like the cli options to avoid this in future? > > switch (subopt) { > case D_NOALIGN: > ..... > break; > default: > return -EINVAL; > } > return 0; If we do that we'd loose out on the compiler feature I described above. Let me know your preference. > > +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) { > > while (i < linelen) { amended > > > + p = line[i]; > > + i++; > > p = line[i++]; amended > > > + > > + /* tab or space */ > > + if (isblank(p)) > > + continue; > > + else > > + return false; > > + } > > if (!isblank(p)) > return false; amended > } > > + > > + return true; > > +} > > + > > +static bool > > +iscomment( > > + const char *line, > > + ssize_t linelen) > > +{ > > + ssize_t i = 0; > > + char p; > > + > > + while (i != linelen) { > > + p = line[i]; > > + i++; > > + > > + /* tab or space */ > > + if (isblank(p)) > > + continue; > > + > > + if (p == '#') > > + return true; > > + > > + return false; > > + } > > + > > + return false; > > +} > > + > > +static int > > +parse_line_section( > > + const char *line, > > + char **tag) > > +{ > > + return sscanf(line, " [%m[^]]]", tag); > > +} > > + > > +static int > > +parse_line_tag_value( > > + const char *line, > > + char **tag, > > + uint64_t *value) > > +{ > > + return sscanf(line, " %m[^ \t=]" > > + " = " > > + "%lu ", > > + tag, value); > > +} > > %lu won't match uint64_t on 32 bit builds. Doesn't this need to be > PRIu64 to be correct? PRIu64 indeed seems to be the right thing to use for uint64_t for c99. > Hmmm, I thought the compiler checked this > format stuff and threw warnings when you get it wrong? I get no compiler errors. > Also, I'm not sure there's any value to these single line functions. > Why not just call them directly in the next function? Because they are descriptive and saves us the comment. You seem to prefer to have the comment so I will use that. > > + > > +static enum parse_line_type > > +parse_get_line_type( > > + const char *line, > > + ssize_t linelen, > > + char **tag, > > + uint64_t *value) > > +{ > > + int ret; > > + uint64_t u64_value; > > + > > + if (isempty(line, linelen)) > > + return PARSE_EMPTY; > > + > > + if (iscomment(line, linelen)) > > + return PARSE_COMMENT; > > + > > + ret = parse_line_section(line, tag); > > + if (ret == 1) > > + return PARSE_SECTION; > > i.e. > /* check if we have a section header */ > ret = sscanf(line, " [%m[^]]]", tag); > if (ret == 1) > return PARSE_SECTION; OK > > + > > + if (ret == EOF) > > + return PARSE_EOF; > > + > > + ret = parse_line_tag_value(line, tag, &u64_value); > > + if (ret == 2) { > > + *value = u64_value; > > + > > + return PARSE_TAG_VALUE; > > + } > > /* should be a "tag = value" config option */ > ret = sscanf(line, " %m[^ \t=] = %" PRIu64 " ", tag, value); > if (ret == 2) > return PARSE_TAG_VALUE; Sure. > > + > > + if (ret == EOF) > > + return PARSE_EOF; > > + > > + return PARSE_INVALID; > > +} > > + > > +static int > > +parse_config_stream( > > + struct mkfs_default_params *dft, > > + const char *config_file, > > + FILE *fp) > > +{ > > + int ret; > > + char *line = NULL; > > + ssize_t linelen; > > + size_t len = 0, lineno = 0; > > + uint64_t value; > > + enum parse_line_type parse_type; > > + struct confopts *confopt = NULL; > > + int subopt; > > + > > + while ((linelen = getline(&line, &len, fp)) != -1) { > > + char *ignore_value; > > + char *p, *tag = NULL; > > + > > + lineno++; > > + > > + /* > > + * tag is allocated for us by scanf(), it must freed only on any > > + * successful parse of a section or tag-value pair. > > + */ > > + parse_type = parse_get_line_type(line, linelen, &tag, &value); > > + > > + switch (parse_type) { > > + case PARSE_EMPTY: > > + case PARSE_COMMENT: > > + /* Nothing tag to free for these */ > > + continue; > > + case PARSE_EOF: > > + break; > > + case PARSE_INVALID: > > + ret = EINVAL; > > Negative errors. Alright. > > + fprintf(stderr, _("Invalid line %s:%zu : %s\n"), > > + config_file, lineno, line); > > + goto out; > > + case PARSE_SECTION: > > + confopt = get_confopts(tag); > > + if (!confopt) { > > + ret = EINVAL; > > + fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"), > > + config_file, lineno, tag); > > + free(tag); > > + goto out; > > goto out_free_tag; > .... > out_free_tag: > free(tag); > ret = -EINVAL; > goto out; > > Same for all thers others. OK. > > + } > > + if (!confopt->subopts) { > > + ret = EINVAL; > > + fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"), > > + config_file, lineno, tag); > > + free(tag); > > + goto out; > > + } > > + if (confopt->seen) { > > + ret = EINVAL; > > + fprintf(stderr, _("Section '%s' respecified\n"), > > + tag); > > + free(tag); > > + goto out; > > + } > > + confopt->seen = true; > > + free(tag); > > + break; > > + case PARSE_TAG_VALUE: > > + if (!confopt) { > > + ret = EINVAL; > > + fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"), > > + config_file, lineno, line); > > + free(tag); > > + goto out; > > + } > > + > > + /* > > + * We re-use the line buffer allocated by getline(), > > + * however line must be kept pointing to its original > > + * value to free it later. A separate pointer is needed > > + * as getsubopt() will otherwise muck with the value > > + * passed. > > + */ > > + p = line; > > + > > + /* > > + * Trims white spaces. getsubopt() does not grok > > + * white space, it would fail otherwise. > > + */ > > + snprintf(p, len, "%s=%lu", tag, value); > > + > > + /* Not needed anymore */ > > + free(tag); > > + > > + /* > > + * We only use getsubopt() to validate the possible > > + * subopt, we already parsed the value and its already > > + * in a more preferred data type. > > + */ > > + subopt = getsubopt(&p, (char **) confopt->subopts, > > + &ignore_value); > > This hoop-jumping cruft can all be replaced with a simple loop that > just compares the tag to the subopts array: > > for (i = 0; i < MAX_SUBOPTS; i++) { > if (!confopt->subopts[i]) { On the kernel it would be but on userspace this could be uninitialized? > /* option not found, error out */ > goto out_free_tag; > } > > if (strcmp(confopt->subopts[i], tag) == 0) > break; > } > free(tag); > > ret = confopt->parser(dft, i, value); > > + if (ret) { > > + fprintf(stderr, _("Error parsine line %s:%zu : %s\n"), > > + config_file, lineno, line); > > + goto out; > > + } > > + > > + break; > > + } > > + } > > +out: > > + /* We must free even if getline() failed */ > > + free(line); > > + return ret; > > +} > > + > > +static const char *conf_paths[] = { > > + ".", > > + MKFS_XFS_CONF_DIR, > > +}; > > + > > +/* > > + * If the file is not found -1 is returned and errno set. Otherwise > > + * the file descriptor is returned. > > + */ > > +int > > +open_cli_config( > > + char *cli_config_file, > > + char **fpath) > > +{ > > + int fd, len; > > + char *final_path = NULL; > > + char *relative_path= NULL; > > + unsigned int i; > > + > > + if (strlen(cli_config_file) > 2) { > > + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') > > + final_path = cli_config_file; > > + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') > > + final_path = cli_config_file; > > + else if (cli_config_file[0] == '/') > > + final_path = cli_config_file; > > + else > > + relative_path = cli_config_file; > > + } else if (strlen(cli_config_file) == 1) { > > + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { > > + errno = EINVAL; > > + return -1; > > + } else > > + relative_path = cli_config_file; > > + } > > This seems somewhat complex, and it doesn't check what the target of > the path is. > > fstatat(AT_FDCWD, fpath, &st, AT_SYMLINK_NOFOLLOW); > > will tell us if the file exists, it doesn't end in a symlink > and it will resolve both both relative and absolute paths correctly. > > If it succeeds, then we can check that the path points to a regular > file (and not, say, a block device) so that this sort of thing: > > [30/5/18 12:08] <sandeen> mkfs/mkfs.xfs -c /dev/sdb1 > [30/5/18 12:08] <sandeen> Invalid line /dev/sdb1:1 : XFSB > > Gives a sensible error before we get to parsing. > > If the file is found and isn't a regular file, then abort. > > If the file isn't found, then we need to ensure that we have been > passed a name without any directory component as we are going to > search the sysconf dir for that file. That's where basename(3) comes > in - if the name returned by basename(3) doesn't match fpath, then > there was a directory component in fpath, and we reject it and > abort. > > Then we can open the sysconf directory w/ O_PATH|O_DIRECTORY, then > do the same fstatat check and open it w/ openat(): > > fstatat(dirfd, fpath, &st, AT_SYMLINK_NOFOLLOW); > /* check return */ > openat(dirfd, fpath, O_NOFOLLOW, O_RDONLY); > > IOWs, I don't think we should be looking at just the first two > characters of the supplied filename to determine the action to take, > nor do i think we shoul dbe trying to resolve relative paths under > the sysconf dir. Alright, I'll give this a shot. > > +/* > > + * This is only called *iff* there is a configuration file which we know we > > + * *must* parse. > > + * > > + * If default_fd is set and is a valid file descriptor then the configuration > > + * file passed is the system default configuraiton file, and we already know > > + * it exists. If default_fd is not set we assume we've been passed a > > + * configuration file from the command line and must it must exist, otherwise > > + * we have to error out. > > + */ > > +int > > +parse_defaults_file( > > + struct mkfs_default_params *dft, > > + int default_fd, > > + char *config_file) > > +{ > > + char *fpath; > > + int fd; > > + FILE *fp; > > + int ret; > > + struct stat sp; > > + > > + if (strlen(config_file) > PATH_MAX) > > + return ENAMETOOLONG; > > + > > + fpath = malloc(PATH_MAX); > > + if (!fpath) > > + return ENOMEM; > > + memset(fpath, 0, PATH_MAX); > > + > > + if (default_fd < 0) { > > + fd = open_cli_config(config_file, &fpath); > > + if (fd < 0) { > > + free(fpath); > > + return errno; > > + } > > + } else { > > + fd = default_fd; > > + memcpy(fpath, config_file, strlen(config_file)); > > + } > > This is messy - opening the file shoul dbe split from parsing the > config file. I make more comments about the reason below. But I > think this should end up as: > > int > open_config_file( > const char *config_file, > char **source) > { > /* > * XXX: should asprintf the source string OK > so it's got the > * config_file opened in it. Not sure what you mean by this though, can you clarify? > */ > dft.source = _("Built in defaults"); This was already set, and dft is not a global, are you suggesting to remove the global setting you had and *always* make source now an allocated string? > dirfd = open(sysconfdir, O_PATH|O_NOFOLLOW); > > if (config_file) { > fd = open_cli_config(dirfd, config_file) > if (fd > 0) fd >= 0 > *source = _("CLI supplied file"); > goto out; > } > > fd = openat(dirfd, "default") > if (fd > 0) fd >= 0 > *source = ("Package default config file") I take it you mean to use asprintf(source, _("Package default config file") but this can also fail... and adds more error paths to check for... > out: > close(dirfd); > return fd; > } I think this asprintf() source approach add more complexity as well, which is why I had used an enum for it a while ago. Think about it and let me know. > and so be completely separated from the act of parsing the config > file. > > > + > > + /* > > + * At this point we know we have a valid file descriptor and have > > + * figured out the path to the file used on fpath. Get the file stream > > + * and do a bit of sanity checks before parsing the file. > > + */ > > + > > + fp = fdopen(fd, "r"); > > + if (!fp) { > > + perror(fpath); > > + ret = errno; > > + goto out_close_fd; > > + } > > + > > + ret = fstat(fd, &sp); > > + if (ret) { > > + ret = errno; > > + fprintf(stderr, _("Could not fstat() config file: %s: %s\n"), > > + fpath, strerror(errno)); > > + goto out; > > + } > > + > > + if (S_ISDIR(sp.st_mode)) { > > + ret = EBADF; > > if (!S_ISREG(sp.st_mode)) { > ret = -EINVAL; Amended. > > > + fprintf(stderr, _("Config file is a directory: %s\n"), fpath); > > + goto out; > > + } > > + > > + /* Anything beyond 1 MiB is kind of silly right now */ > > + if (sp.st_size > 1 * 1024 * 1024) { > > + ret = E2BIG; > > + goto out; > > + } > > As mentioned about, these checks should be done before we even open > the file. OK > > + > > + /* > > + * Pull config line options from command line > > */ > > + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { > > Why specify all the parameters? We're only interested in the "c:" > parameter here, so that's the only one we need to specify, right? Have you tried? I didn't work for me, its why I added the full set of parameters again. > Otherwise we've got two config option strings we need to keep i > check... Right, which is why I had a macro for it a while ago. > > + switch (c) { > > + case 'c': > > + if (cli_config_file) { > > + fprintf(stderr, _("respecification of configuration not allowed\n")); > > + exit(1); > > + } > > + cli_config_file = optarg; > > + dft.source = _("command line"); > > + break; > > + default: > > + continue; > > + } > > + } > > + > > + if (cli_config_file) > > + config_file = cli_config_file; > > + else { > > + default_fd = open(default_config, O_RDONLY); > > + if (default_fd >= 0) { > > + dft.source = _("system default configuration file"); > > + config_file = default_config; > > + } > > + } > > + > > + if (config_file) { > > + /* If default_fd is set it will be closed for us */ > > + ret = parse_defaults_file(&dft, default_fd, config_file); > > + if (ret) { > > + fprintf(stderr, _("Error parsing %s config file: %s : %s\n"), > > + dft.source, config_file, > > + strerror(ret)); > > + exit(1); > > + } > > + } > > It doesn't make sense to me to open the default config file here > before we know if it going to be needed. I would suggest that this > get split into two parts - one to find and open the config file, the > other to parse the opened file. i.e.: > > fd = open_config_file(cli_config_file, &dft.source); > if (fd >= 0) { > ret = parse_defaults_file(fd, &dft); > .... > close(fd); > } Let me know what you think of the other things first too. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 04:31:46PM -0700, Darrick J. Wong wrote: > On Tue, May 29, 2018 at 03:06:02PM -0700, Luis R. Rodriguez wrote: > > +int > > +parse_defaults_file( > > + struct mkfs_default_params *dft, > > + int default_fd, > > + char *config_file) > > +{ > > + char *fpath; > > + int fd; > > + FILE *fp; > > + int ret; > > + struct stat sp; > > + > > + if (strlen(config_file) > PATH_MAX) > > + return ENAMETOOLONG; > > + > > + fpath = malloc(PATH_MAX); > > + if (!fpath) > > + return ENOMEM; > > + memset(fpath, 0, PATH_MAX); > > + > > + if (default_fd < 0) { > > + fd = open_cli_config(config_file, &fpath); > > + if (fd < 0) { > > + free(fpath); > > + return errno; > > + } > > + } else { > > + fd = default_fd; > > + memcpy(fpath, config_file, strlen(config_file)); > > + } > > + > > + /* > > + * At this point we know we have a valid file descriptor and have > > + * figured out the path to the file used on fpath. Get the file stream > > + * and do a bit of sanity checks before parsing the file. > > + */ > > + > > + fp = fdopen(fd, "r"); > > + if (!fp) { > > + perror(fpath); > > It occurred to me just now (sorry...) that the caller of this function > prints out a string with strerror() contents, so the perror here is > unnecessary Alright. > since the caller will cough up an error anyway. Then all of > these constructions here become: > > fp = fdopen(...); > if (!fp) > return -1; > > ret = fstat(...); > if (ret) > goto out; > > if (S_ISDIR(...)) { > errno = EISDIR; > goto out; > } > ... > return 0; > out: > return -1; Sure. > > and the call site now becomes: > > if (parse_defaults_file(...)) { > fprintf(stderr, "%s: file is bad: %s\n", path, strerror(errno)); > ... > } Works with me. > Granted maybe we should just merge this and do all those cleanups > separately. ;) > > + if (S_ISDIR(sp.st_mode)) { > > + ret = EBADF; > > ret = EISDIR? Chinner asked we always make a regular file out of the config, so now a regular file check will suffice, and EISDIR would not be as descriptive. So I'll wait to hear back on some other minor things, hopefully next week we can wrap up this series for good.
diff --git a/configure.ac b/configure.ac index 508eefede073..94c5bda725f2 100644 --- a/configure.ac +++ b/configure.ac @@ -233,5 +233,9 @@ AC_CHECK_SIZEOF([char *]) AC_TYPE_UMODE_T AC_MANUAL_FORMAT -AC_CONFIG_FILES([include/builddefs]) +AC_CONFIG_FILES([ + include/builddefs + man/man5/mkfs.xfs.d.5 + man/man8/mkfs.xfs.8 +]) AC_OUTPUT diff --git a/include/builddefs.in b/include/builddefs.in index 8aac06cf90dc..e1ee9f7ba086 100644 --- a/include/builddefs.in +++ b/include/builddefs.in @@ -62,6 +62,7 @@ PKG_LIB_DIR = @libdir@@libdirsuffix@ PKG_INC_DIR = @includedir@/xfs DK_INC_DIR = @includedir@/disk PKG_MAN_DIR = @mandir@ +PKG_ETC_DIR = @sysconfdir@ PKG_DOC_DIR = @datadir@/doc/@pkg_name@ PKG_LOCALE_DIR = @datadir@/locale @@ -196,6 +197,7 @@ endif GCFLAGS = $(DEBUG) \ -DVERSION=\"$(PKG_VERSION)\" -DLOCALEDIR=\"$(PKG_LOCALE_DIR)\" \ + -DROOT_SYSCONFDIR=\"$(PKG_ETC_DIR)\" \ -DPACKAGE=\"$(PKG_NAME)\" -I$(TOPDIR)/include -I$(TOPDIR)/libxfs ifeq ($(ENABLE_GETTEXT),yes) diff --git a/man/man5/Makefile b/man/man5/Makefile index fe0aef6f016b..0b33122b064e 100644 --- a/man/man5/Makefile +++ b/man/man5/Makefile @@ -19,3 +19,5 @@ install : default $(INSTALL) -m 755 -d $(MAN_DEST) $(INSTALL_MAN) install-dev : + +LDIRT += mkfs.xfs.d.5 diff --git a/man/man5/mkfs.xfs.d.5.in b/man/man5/mkfs.xfs.d.5.in new file mode 100644 index 000000000000..287877ce029d --- /dev/null +++ b/man/man5/mkfs.xfs.d.5.in @@ -0,0 +1,151 @@ +.TH mkfs.xfs.d 5 +.SH NAME +mkfs.xfs.d \- mkfs.xfs configuration directory +.SH DESCRIPTION +.B mkfs.xfs (8) +uses a set of initial default parameters for configuration. + +The built-in mkfs defaults are decided by the XFS community. New features are +only enabled by default when the community consider them stable. One can +override these defaults on the +.B mkfs.xfs (8) +command line, but there are cases where it is desirable for the distro or the +system administrator to establish their own supported defaults in a uniform +manner, regardless of the version of +.B mkfs.xfs (8) +used. This may be desirable for example on systems with old kernels +where the built-in default +.B mkfs.xfs (8) +parameters create a filesystem that is not supported by the old kernel. +In such situations it would also be unclear what parameters are needed to +produce a compatible filesystem, having a configuration file present ensures +that if newer versions of +.B mkfs.xfs (8) +are deployed, creating a filesystem will remain compatible. Overriding +.B mkfs.xfs (8) +built-in defaults may also be desirable if you have a series of systems with +different kernels and want to be able to create filesystems which all systems +are able to support properly. +.PP +The XFS configuration directory +.B mkfs.xfs.d (5) +can be used as a home to define different configuration files which can be used +to override the built-in default parameters by +.B mkfs.xfs (8). +If the +.B -c +parameter is not used, the default configuration file: +.IP +.I @sysconfdir@/mkfs.xfs.d/default +.PP +will be looked for first and if present will be used to override +.B mkfs.xfs (8) +built-in default parameters. +.PP +You can override the default configuration file by specifying its name when +using +.B mkfs.xfs (8) +by using the +.B -c +parameter. +.PP +If the path does not start with a '.', the current working directory is +searched for the file. If the file is not found there, the +.B mkfs.xfs.d (5) +directory is searched for the file. +.PP +If +.B -c +is used with relative path with which has a leading '.' character, the given +path is used directly, so the configuration file will be relative to the +current working directory. +.PP +If the +.B -c +argument starts with a '/', it is considered an absolute path, and opened. +.PP +For example: +.IP +.B mkfs.xfs -c experimental -f /dev/sda1 +.PP +Will make +.B mkfs.xfs (8) +look for the following configuration files and use the first one it finds: +.IP +.B $PWD/experimental +.br +.B @sysconfdir@/mkfs.xfs.d/experimental +.PP +If you used an absolute path, for example: +.IP +.B mkfs.xfs -c /tmp/experimental -f /dev/sda1 +.PP +Then only the configuration file /tmp/experimental will be looked for and +used if present. +.PP +If you use the +.B -c +parameter the configuration file must be present and must parse correctly. +.PP +Parameters passed to the +.B mkfs.xfs (8) +command line always override any defaults set by the configuration file. +.PP +.B mkfs.xfs (8) +will always describe what configuration file was used, if any +was used at all. To verify which configuration file would be used prior to +execution of +.B mkfs.xfs (8) +you can use +.I mkfs.xfs -N. +.PP +.SH DEFAULT PARAMETERS +Default parameters for +.B mkfs.xfs (8) +consists of a small subset of the parameters one can set with on the command +line. Currently all default parameters can only be either enabled or disabled, +you can set their value to 1 to enable or 0 to disable. Below we list the +different supported default parameters which can be defined on configuration +files, along with the current built-in setting. +.PP +.BI [data] +.br +.BI noalign=0 +.PP +.BI [inode] +.br +.BI align=1 +.br +.BI projid32bit=1 +.br +.BI sparse=0 +.PP +.BI [log] +.br +.BI lazy-count=1 +.PP +.BI [metadata] +.br +.BI crc=1 +.br +.BI finobt=1 +.br +.BI rmapbt=0 +.br +.BI reflink=0 +.PP +.BI [naming] +.br +.BI ftype=1 +.PP +.BI [rtdev] +.br +.BI noalign=0 +.PP +.SH SEE ALSO +.BR mkfs.xfs (8), +.BR xfsctl (3), +.BR xfs_info (8), +.BR xfs_admin (8), +.BR xfsdump (8), +.BR xfsrestore (8). diff --git a/man/man8/Makefile b/man/man8/Makefile index 36620da172ae..2a6548079997 100644 --- a/man/man8/Makefile +++ b/man/man8/Makefile @@ -19,3 +19,5 @@ install : default $(INSTALL) -m 755 -d $(MAN_DEST) $(INSTALL_MAN) install-dev : + +LDIRT += mkfs.xfs.8 diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8.in similarity index 96% rename from man/man8/mkfs.xfs.8 rename to man/man8/mkfs.xfs.8.in index 4b8c78c37806..81e2753bd2b5 100644 --- a/man/man8/mkfs.xfs.8 +++ b/man/man8/mkfs.xfs.8.in @@ -83,6 +83,24 @@ and .B \-l internal \-l size=10m are equivalent. .PP +An optional XFS configuration file directory +.B mkfs.xfs.d (5) +exists to help fine tune different default parameters which can be used when +calling +.B mkfs.xfs (8). +If present, and if +.B -c +is not used, the default configuration file @sysconfigdir@/mkfs.xfs.d/default +will be used to override system build-in defaults. Refer to mkfs.xfs.d (5) +for a list of current defaults and further details. +Command line arguments directly passed to +.B mkfs.xfs (8) +will always override parameters set in the configuration file. +You can override the configuration file used by using the +.B -c +parameter, further explained below and in +.B mkfs.xfs.d (5) +.PP In the descriptions below, sizes are given in sectors, bytes, blocks, kilobytes, megabytes, gigabytes, etc. Sizes are treated as hexadecimal if prefixed by 0x or 0X, @@ -123,6 +141,14 @@ Many feature options allow an optional argument of 0 or 1, to explicitly disable or enable the functionality. .SH OPTIONS .TP +.BI \-c " configuration-file" +Override the configuration file used. If a relative path is given the search +path for the configuration file is your current directory and then the +.B mkfs.xfs.d (5) +directory. If an absolute path is given it is used directly. For more details +refer to +.B mkfs.xfs.d (5) +.TP .BI \-b " block_size_options" This option specifies the fundamental block size of the filesystem. The valid @@ -923,6 +949,7 @@ Prints the version number and exits. .SH SEE ALSO .BR xfs (5), .BR mkfs (8), +.BR mkfs.xfs.d (5), .BR mount (8), .BR xfs_info (8), .BR xfs_admin (8). diff --git a/mkfs/Makefile b/mkfs/Makefile index c84f9b6ae63b..c7815b3e106b 100644 --- a/mkfs/Makefile +++ b/mkfs/Makefile @@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs LTCOMMAND = mkfs.xfs HFILES = -CFILES = proto.c xfs_mkfs.c +CFILES = proto.c xfs_mkfs.c config.c LLDLIBS += $(LIBXFS) $(LIBXCMD) $(LIBFROG) $(LIBRT) $(LIBPTHREAD) $(LIBBLKID) \ $(LIBUUID) diff --git a/mkfs/config.c b/mkfs/config.c new file mode 100644 index 000000000000..0b4aebe51903 --- /dev/null +++ b/mkfs/config.c @@ -0,0 +1,645 @@ +/* + * Copyright (c) 2018 Luis R. Rodriguez <mcgrof@kernel.org> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#include <ctype.h> +#include <dirent.h> +#include <fcntl.h> + +#include "libxfs.h" +#include "config.h" + +/* + * Enums for each configuration option. All these currently match the CLI + * parameters for now but this may change later, so we keep all this code + * and definitions separate. The rules for configuration parameters may also + * differ. + * + * We only provide definitions for what we currently support parsing. + */ + +enum data_subopts { + D_NOALIGN = 0, +}; + +enum inode_subopts { + I_ALIGN = 0, + I_PROJID32BIT, + I_SPINODES, +}; + +enum log_subopts { + L_LAZYSBCNTR = 0, +}; + +enum metadata_subopts { + M_CRC = 0, + M_FINOBT, + M_RMAPBT, + M_REFLINK, +}; + +enum naming_subopts { + N_FTYPE = 0, +}; + +enum rtdev_subopts { + R_NOALIGN = 0, +}; + +/* Just define the max options array size manually right now */ +#define MAX_SUBOPTS 4 + +static int +data_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum data_subopts subopt = psubopt; + + switch (subopt) { + case D_NOALIGN: + dft->sb_feat.nodalign = value; + return 0; + } + return EINVAL; +} + +static int +inode_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum inode_subopts subopt = psubopt; + + switch (subopt) { + case I_ALIGN: + dft->sb_feat.inode_align = value; + return 0; + case I_PROJID32BIT: + dft->sb_feat.projid32bit = value; + return 0; + case I_SPINODES: + dft->sb_feat.spinodes = value; + return 0; + } + return EINVAL; +} + +static int +log_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum log_subopts subopt = psubopt; + + switch (subopt) { + case L_LAZYSBCNTR: + dft->sb_feat.lazy_sb_counters = value; + return 0; + } + return EINVAL; +} + +static int +metadata_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum metadata_subopts subopt = psubopt; + + switch (subopt) { + case M_CRC: + dft->sb_feat.crcs_enabled = value; + if (dft->sb_feat.crcs_enabled) + dft->sb_feat.dirftype = true; + return 0; + case M_FINOBT: + dft->sb_feat.finobt = value; + return 0; + case M_RMAPBT: + dft->sb_feat.rmapbt = value; + return 0; + case M_REFLINK: + dft->sb_feat.reflink = value; + return 0; + } + return EINVAL; +} + +static int +naming_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum naming_subopts subopt = psubopt; + + switch (subopt) { + case N_FTYPE: + dft->sb_feat.dirftype = value; + return 0; + } + return EINVAL; +} + +static int +rtdev_config_parser( + struct mkfs_default_params *dft, + int psubopt, + uint64_t value) +{ + enum rtdev_subopts subopt = psubopt; + + switch (subopt) { + case R_NOALIGN: + dft->sb_feat.nortalign = value; + return 0; + } + return EINVAL; +} + +struct confopts { + const char *name; + const char *subopts[MAX_SUBOPTS]; + int (*parser)(struct mkfs_default_params *dft, + int psubopt, uint64_t value); + bool seen; +} confopts_tab[] = { + { + .name = "data", + .subopts = { + [D_NOALIGN] = "noalign", + }, + .parser = data_config_parser, + }, + { + .name = "inode", + .subopts = { + [I_ALIGN] = "align", + [I_PROJID32BIT] = "projid32bit", + [I_SPINODES] = "sparse", + }, + .parser = inode_config_parser, + }, + { + .name = "log", + .subopts = { + [L_LAZYSBCNTR] = "lazy-count", + }, + .parser = log_config_parser, + }, + { + .name = "naming", + .subopts = { + [N_FTYPE] = "ftype", + }, + .parser = naming_config_parser, + }, + { + .name = "rtdev", + .subopts = { + [R_NOALIGN] = "noalign", + }, + .parser = rtdev_config_parser, + }, + { + .name = "metadata", + .subopts = { + [M_CRC] = "crc", + [M_FINOBT] = "finobt", + [M_RMAPBT] = "rmapbt", + [M_REFLINK] = "reflink", + }, + .parser = metadata_config_parser, + }, +}; + +static struct confopts * +get_confopts( + const char *section) +{ + unsigned int i; + struct confopts *opts; + + for (i=0; i < ARRAY_SIZE(confopts_tab); i++) { + opts = &confopts_tab[i]; + if (!opts) + return NULL; + if (strcmp(opts->name, section) == 0) { + return opts; + } + } + return NULL; +} + +enum parse_line_type { + PARSE_COMMENT = 0, + PARSE_EMPTY, + PARSE_SECTION, + PARSE_TAG_VALUE, + PARSE_INVALID, + PARSE_EOF, +}; + +static bool +isempty( + const char *line, + ssize_t linelen) +{ + ssize_t i = 0; + char p; + + while (i != linelen) { + p = line[i]; + i++; + + /* tab or space */ + if (isblank(p)) + continue; + else + return false; + } + + return true; +} + +static bool +iscomment( + const char *line, + ssize_t linelen) +{ + ssize_t i = 0; + char p; + + while (i != linelen) { + p = line[i]; + i++; + + /* tab or space */ + if (isblank(p)) + continue; + + if (p == '#') + return true; + + return false; + } + + return false; +} + +static int +parse_line_section( + const char *line, + char **tag) +{ + return sscanf(line, " [%m[^]]]", tag); +} + +static int +parse_line_tag_value( + const char *line, + char **tag, + uint64_t *value) +{ + return sscanf(line, " %m[^ \t=]" + " = " + "%lu ", + tag, value); +} + +static enum parse_line_type +parse_get_line_type( + const char *line, + ssize_t linelen, + char **tag, + uint64_t *value) +{ + int ret; + uint64_t u64_value; + + if (isempty(line, linelen)) + return PARSE_EMPTY; + + if (iscomment(line, linelen)) + return PARSE_COMMENT; + + ret = parse_line_section(line, tag); + if (ret == 1) + return PARSE_SECTION; + + if (ret == EOF) + return PARSE_EOF; + + ret = parse_line_tag_value(line, tag, &u64_value); + if (ret == 2) { + *value = u64_value; + + return PARSE_TAG_VALUE; + } + + if (ret == EOF) + return PARSE_EOF; + + return PARSE_INVALID; +} + +static int +parse_config_stream( + struct mkfs_default_params *dft, + const char *config_file, + FILE *fp) +{ + int ret; + char *line = NULL; + ssize_t linelen; + size_t len = 0, lineno = 0; + uint64_t value; + enum parse_line_type parse_type; + struct confopts *confopt = NULL; + int subopt; + + while ((linelen = getline(&line, &len, fp)) != -1) { + char *ignore_value; + char *p, *tag = NULL; + + lineno++; + + /* + * tag is allocated for us by scanf(), it must freed only on any + * successful parse of a section or tag-value pair. + */ + parse_type = parse_get_line_type(line, linelen, &tag, &value); + + switch (parse_type) { + case PARSE_EMPTY: + case PARSE_COMMENT: + /* Nothing tag to free for these */ + continue; + case PARSE_EOF: + break; + case PARSE_INVALID: + ret = EINVAL; + fprintf(stderr, _("Invalid line %s:%zu : %s\n"), + config_file, lineno, line); + goto out; + case PARSE_SECTION: + confopt = get_confopts(tag); + if (!confopt) { + ret = EINVAL; + fprintf(stderr, _("Invalid section on line %s:%zu : %s\n"), + config_file, lineno, tag); + free(tag); + goto out; + } + if (!confopt->subopts) { + ret = EINVAL; + fprintf(stderr, _("Section not yet supported on line %s:%zu : %s\n"), + config_file, lineno, tag); + free(tag); + goto out; + } + if (confopt->seen) { + ret = EINVAL; + fprintf(stderr, _("Section '%s' respecified\n"), + tag); + free(tag); + goto out; + } + confopt->seen = true; + free(tag); + break; + case PARSE_TAG_VALUE: + if (!confopt) { + ret = EINVAL; + fprintf(stderr, _("No section specified yet on line %s:%zu : %s\n"), + config_file, lineno, line); + free(tag); + goto out; + } + + /* + * We re-use the line buffer allocated by getline(), + * however line must be kept pointing to its original + * value to free it later. A separate pointer is needed + * as getsubopt() will otherwise muck with the value + * passed. + */ + p = line; + + /* + * Trims white spaces. getsubopt() does not grok + * white space, it would fail otherwise. + */ + snprintf(p, len, "%s=%lu", tag, value); + + /* Not needed anymore */ + free(tag); + + /* + * We only use getsubopt() to validate the possible + * subopt, we already parsed the value and its already + * in a more preferred data type. + */ + subopt = getsubopt(&p, (char **) confopt->subopts, + &ignore_value); + + ret = confopt->parser(dft, subopt, value); + if (ret) { + fprintf(stderr, _("Error parsine line %s:%zu : %s\n"), + config_file, lineno, line); + goto out; + } + + break; + } + } +out: + /* We must free even if getline() failed */ + free(line); + return ret; +} + +static const char *conf_paths[] = { + ".", + MKFS_XFS_CONF_DIR, +}; + +/* + * If the file is not found -1 is returned and errno set. Otherwise + * the file descriptor is returned. + */ +int +open_cli_config( + char *cli_config_file, + char **fpath) +{ + int fd, len; + char *final_path = NULL; + char *relative_path= NULL; + unsigned int i; + + if (strlen(cli_config_file) > 2) { + if (cli_config_file[0] == '.' && cli_config_file[1] == '/') + final_path = cli_config_file; + else if (cli_config_file[0] == '.' && cli_config_file[1] == '.') + final_path = cli_config_file; + else if (cli_config_file[0] == '/') + final_path = cli_config_file; + else + relative_path = cli_config_file; + } else if (strlen(cli_config_file) == 1) { + if (cli_config_file[0] == '.' || cli_config_file[0] == '/') { + errno = EINVAL; + return -1; + } else + relative_path = cli_config_file; + } + + if (final_path) { + fd = open(final_path, O_RDONLY); + if (fd >= 0) + memcpy(*fpath, final_path, strlen(final_path)); + return fd; + } + + /* We finally know the path is relative but just to be sure */ + if (!relative_path) { + errno = ENXIO; + return -1; + } + + for (i = 0; i < ARRAY_SIZE(conf_paths); i++) { + memset(*fpath, 0, PATH_MAX); + /* + * For current directory evaluation, skip concatenating the + * ./ from the file passed. We only concatenate for the other + * paths we look up on. + */ + if (i == 0) + memcpy(*fpath, relative_path, strlen(relative_path)); + else { + len = snprintf(*fpath, PATH_MAX, "%s/%s", conf_paths[i], + relative_path); + /* Indicates truncation */ + if (len >= PATH_MAX) { + errno = ENAMETOOLONG; + return -1; + } + } + fd = open(*fpath, O_RDONLY); + if (fd < 0) + continue; + return fd; + } + + errno = ENOENT; + return -1; +} + +/* + * This is only called *iff* there is a configuration file which we know we + * *must* parse. + * + * If default_fd is set and is a valid file descriptor then the configuration + * file passed is the system default configuraiton file, and we already know + * it exists. If default_fd is not set we assume we've been passed a + * configuration file from the command line and must it must exist, otherwise + * we have to error out. + */ +int +parse_defaults_file( + struct mkfs_default_params *dft, + int default_fd, + char *config_file) +{ + char *fpath; + int fd; + FILE *fp; + int ret; + struct stat sp; + + if (strlen(config_file) > PATH_MAX) + return ENAMETOOLONG; + + fpath = malloc(PATH_MAX); + if (!fpath) + return ENOMEM; + memset(fpath, 0, PATH_MAX); + + if (default_fd < 0) { + fd = open_cli_config(config_file, &fpath); + if (fd < 0) { + free(fpath); + return errno; + } + } else { + fd = default_fd; + memcpy(fpath, config_file, strlen(config_file)); + } + + /* + * At this point we know we have a valid file descriptor and have + * figured out the path to the file used on fpath. Get the file stream + * and do a bit of sanity checks before parsing the file. + */ + + fp = fdopen(fd, "r"); + if (!fp) { + perror(fpath); + ret = errno; + goto out_close_fd; + } + + ret = fstat(fd, &sp); + if (ret) { + ret = errno; + fprintf(stderr, _("Could not fstat() config file: %s: %s\n"), + fpath, strerror(errno)); + goto out; + } + + if (S_ISDIR(sp.st_mode)) { + ret = EBADF; + fprintf(stderr, _("Config file is a directory: %s\n"), fpath); + goto out; + } + + /* Anything beyond 1 MiB is kind of silly right now */ + if (sp.st_size > 1 * 1024 * 1024) { + ret = E2BIG; + goto out; + } + + ret = parse_config_stream(dft, config_file, fp); + if (ret) + goto out; + + printf(_("config-file=%s\n"), fpath); + +out: + fclose(fp); +out_close_fd: + close(fd); + free(fpath); + return ret; +} diff --git a/mkfs/config.h b/mkfs/config.h index e5ea968e2d65..0f429d9b7fd7 100644 --- a/mkfs/config.h +++ b/mkfs/config.h @@ -19,6 +19,8 @@ #ifndef _XFS_MKFS_CONFIG_H #define _XFS_MKFS_CONFIG_H +#define MKFS_XFS_CONF_DIR ROOT_SYSCONFDIR "/mkfs.xfs.d" + struct fsxattr; /* @@ -29,7 +31,7 @@ struct fsxattr; * source can overriding the later source: * * o built-in defaults - * o configuration file (XXX) + * o configuration file * o command line * * These values are not used directly - they are inputs into the mkfs geometry @@ -75,4 +77,10 @@ struct mkfs_default_params { struct fsxattr fsx; }; +int +parse_defaults_file( + struct mkfs_default_params *dft, + int default_fd, + char *config_file); + #endif /* _XFS_MKFS_CONFIG_H */ diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 4664e507afbf..81ef7ab584ed 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -3743,6 +3743,11 @@ main( .nortalign = false, }, }; + char *default_config = MKFS_XFS_CONF_DIR "/default"; + char *cli_config_file = NULL; + char *config_file = NULL; + int default_fd = -1; + int ret; platform_uuid_generate(&cli.uuid); progname = basename(argv[0]); @@ -3751,25 +3756,74 @@ main( textdomain(PACKAGE); /* - * TODO: Sourcing defaults from a config file - * * Before anything else, see if there's a config file with different - * defaults. If a file exists in <package location>, read in the new - * default values and overwrite them in the &dft structure. This way the - * new defaults will apply before we parse the CLI, and the CLI will - * still be able to override them. When more than one source is - * implemented, emit a message to indicate where the defaults being - * used came from. + * defaults. If the CLI specified a full path we use and require that. + * If a relative path was provided on the CLI we search the allowed + * search paths for the file. If no config file was specified on the + * CLI we will look for MKFS_XFS_CONF_DIR/default and use that if + * present, however this file is optional. * - * printf(_("Default configuration sourced from %s\n"), dft.source); + * If a configuration file is found we use it to help overwrite default + * values in the &dft structure. This way the new defaults will apply + * before we parse the CLI, and the user will still be able to override + * them through the CLI. + */ + + /* + * Pull config line options from command line */ + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { + switch (c) { + case 'c': + if (cli_config_file) { + fprintf(stderr, _("respecification of configuration not allowed\n")); + exit(1); + } + cli_config_file = optarg; + dft.source = _("command line"); + break; + default: + continue; + } + } + + if (cli_config_file) + config_file = cli_config_file; + else { + default_fd = open(default_config, O_RDONLY); + if (default_fd >= 0) { + dft.source = _("system default configuration file"); + config_file = default_config; + } + } + + if (config_file) { + /* If default_fd is set it will be closed for us */ + ret = parse_defaults_file(&dft, default_fd, config_file); + if (ret) { + fprintf(stderr, _("Error parsing %s config file: %s : %s\n"), + dft.source, config_file, + strerror(ret)); + exit(1); + } + } - /* copy new defaults into CLI parsing structure */ + printf(_("Default configuration sourced from %s\n"), dft.source); + + /* + * Done parsing defaults now, so memcpy defaults into CLI + * structure, reset getopt and start parsing CLI options + */ memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat)); memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx)); - while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { + platform_getoptreset(); + + while ((c = getopt(argc, argv, "b:c:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) { switch (c) { + case 'c': + /* already validated and parsed, ignore */ + break; case 'C': case 'f': force_overwrite = 1;
You may want to stick to specific set of configuration options when creating filesystems with mkfs.xfs -- sometimes due to pure technical reasons, but some other times to ensure systems remain compatible as new features are introduced with older kernels, or if you always want to take advantage of some new feature which would otherwise typically be disruptive. This adds support for parsing a configuration file to override defaults parameters to be used for mkfs.xfs. We define an XFS configuration directory, /etc/mkfs.xfs.d/ and allow for different configuration files, if none is specified we look for the default configuration file, /etc/mkfs.xfs.d/default. You can override with -c. For instance, if you specify: mkfs.xfs -c experimental -f /dev/loop0 The search path for the configuration file will be: 1) $PWD/experimental 2) /etc/mkfs.xfs.d/experimental Absolute paths are supported, in which case they will be used directly and the mkfs.xfs.d directory is ignored. To verify what configuration file is used on a system use the typical: mkfs.xfs -N There is only a subset of options allowed to be set on the configuration file. The default parameters you can override on a configuration file and their current built-in default settings are: [data] noalign=0 [inode] align=1 projid32bit=1 sparse=0 [log] lazy-count=1 [metadata] crc=1 finobt=1 rmapbt=0 reflink=0 [naming] ftype=1 [rtdev] noalign=0 Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org> --- configure.ac | 6 +- include/builddefs.in | 2 + man/man5/Makefile | 2 + man/man5/mkfs.xfs.d.5.in | 151 ++++++++ man/man8/Makefile | 2 + man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} | 27 ++ mkfs/Makefile | 2 +- mkfs/config.c | 645 +++++++++++++++++++++++++++++++++ mkfs/config.h | 10 +- mkfs/xfs_mkfs.c | 76 +++- 10 files changed, 909 insertions(+), 14 deletions(-) create mode 100644 man/man5/mkfs.xfs.d.5.in rename man/man8/{mkfs.xfs.8 => mkfs.xfs.8.in} (96%) create mode 100644 mkfs/config.c