| Message ID | 8c2a5a07-5d47-48a1-1593-e93eb0fb883a@sandeen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 13, 2018 at 02:37:01PM -0500, Eric Sandeen wrote: > The whole notion of a defaults "type" led to complications that > simply doesn't need to be there. If a configuration file was > used for defaults, simply state that fact and carry on. > > This also removes ambiguity for config files specified as > a relative path by calling realpath. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com> > > config.c | 20 ++++++++------------ > config.h | 31 ------------------------------- > xfs_mkfs.c | 8 +++----- > 3 files changed, 11 insertions(+), 48 deletions(-) > > diff --git a/mkfs/config.c b/mkfs/config.c > index 1a2cd35..3a389ad 100644 > --- a/mkfs/config.c > +++ b/mkfs/config.c > @@ -557,6 +557,7 @@ open_config_file( > { > int dirfd = -1, fd = -1, len, ret = 0; > struct stat st; > + bool cli_specified = false; > > *fpath = malloc(PATH_MAX); > if (!*fpath) > @@ -566,18 +567,19 @@ open_config_file( > > /* first try relative to pwd or absolute path to cli configfile */ > if (config_file) { > - dft->type = DEFAULTS_CLI_CONFIG; > + cli_specified = true; > if (strlen(config_file) > PATH_MAX) { > errno = ENAMETOOLONG; > goto out; > } > - memcpy(*fpath, config_file, strlen(config_file)); > + /* Get absolute path to this file */ > + realpath(config_file, *fpath); > fd = openat(AT_FDCWD, config_file, O_NOFOLLOW, O_RDONLY); > } > > /* on failure search for cli config or default file in sysconfdir */ > if (fd < 0) { > - if (!config_file) > + if (!cli_specified) > config_file = MKFS_XFS_DEFAULT_CONFIG; > len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR, > config_file); > @@ -592,8 +594,6 @@ open_config_file( > fd = openat(dirfd, config_file, O_NOFOLLOW, O_RDONLY); > if (fd < 0) > goto out; > - if (!strcmp(config_file, MKFS_XFS_DEFAULT_CONFIG)) > - dft->type = DEFAULTS_CONFIG; > } > > ret = fstat(fd, &st); > @@ -606,11 +606,9 @@ open_config_file( > out: > /* stat check is always fatal; missing is fatal only if cli-specified */ > if (ret || > - (fd < 0 && dft->type == DEFAULTS_CLI_CONFIG)) { > - fprintf(stderr, > -_("Unable to open %s config file: %s : %s\n"), > - default_type_str(dft->type), *fpath, > - strerror(errno)); > + (fd < 0 && cli_specified)) { > + fprintf(stderr, _("Unable to open config file: %s : %s\n"), > + *fpath, strerror(errno)); > free(*fpath); > exit(1); > } > @@ -645,7 +643,5 @@ parse_defaults_file( > return -1; > } > > - printf(_("config-file=%s\n"), config_file); > - > return 0; > } > diff --git a/mkfs/config.h b/mkfs/config.h > index db22ade..f4af2c7 100644 > --- a/mkfs/config.h > +++ b/mkfs/config.h > @@ -58,22 +58,6 @@ struct sb_feat_args { > }; > > /* > - * File configuration type settings > - * > - * These are the different possibilities by which you can end up parsing > - * default settings with. DEFAULTS_BUILTIN indicates there was no configuration > - * file parsed and we are using the built-in defaults on this code. > - * DEFAULTS_CONFIG means the default configuration file was found and used. > - * DEFAULTS_CLI_CONFIG means the user asked for a custom configuration type > - * through the command line interface and it was used. > - */ > -enum default_params_type { > - DEFAULTS_BUILTIN = 0, > - DEFAULTS_CONFIG, > - DEFAULTS_CLI_CONFIG, > -}; > - > -/* > * Default filesystem features and configuration values > * > * This structure contains the default mkfs values that are to be used when > @@ -82,8 +66,6 @@ enum default_params_type { > * calculations. > */ > struct mkfs_default_params { > - enum default_params_type type; /* where the defaults came from */ > - > int sectorsize; > int blocksize; > > @@ -94,19 +76,6 @@ struct mkfs_default_params { > struct fsxattr fsx; > }; > > -static inline const char *default_type_str(enum default_params_type type) > -{ > - switch (type) { > - case DEFAULTS_BUILTIN: > - return _("package built-in definitions"); > - case DEFAULTS_CONFIG: > - return _("package default config file"); > - case DEFAULTS_CLI_CONFIG: > - return _("CLI supplied file"); > - } > - return _("Unkown\n"); > -} > - > int > open_config_file( > const char *cli_config_file, > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index d32c2c8..56e5f03 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3722,7 +3722,6 @@ main( > > /* build time defaults */ > struct mkfs_default_params dft = { > - .type = DEFAULTS_BUILTIN, > .sectorsize = XFS_MIN_SECTORSIZE, > .blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG, > .sb_feat = { > @@ -3796,19 +3795,18 @@ _("respecification of configuration not allowed\n")); > ret = parse_defaults_file(fd, &dft, config_file); > if (ret) { > fprintf(stderr, > -_("Error parsing %s config file: %s : %s\n"), > - default_type_str(dft.type), > +_("Error parsing config file: %s : %s\n"), > config_file, strerror(errno)); > free(config_file); > close(fd); > exit(1); > } > + printf(_("Configuration file used for defaults: %s\n"), > + config_file); > free(config_file); > close(fd); > } > > - printf(_("Default configuration sourced from %s\n"), > - default_type_str(dft.type)); > > /* > * Done parsing defaults now, so memcpy defaults into CLI > > -- > 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, Jun 13, 2018 at 02:37:01PM -0500, Eric Sandeen wrote: > -static inline const char *default_type_str(enum default_params_type type) > -{ > - switch (type) { > - case DEFAULTS_BUILTIN: > - return _("package built-in definitions"); > - case DEFAULTS_CONFIG: > - return _("package default config file"); > - case DEFAULTS_CLI_CONFIG: > - return _("CLI supplied file"); > - } > - return _("Unkown\n"); > -} > > - printf(_("Default configuration sourced from %s\n"), > - default_type_str(dft.type)); The above is really the reason to the type stuff, and it was also why I added it, as otherwise we had to allocate a string for the old way of supplying that data. So really this is about the *source*, the type enum stuff is just a way to implement it without dealing with complexities on the string. So regardless of the type, the question is we are OK to loose the source of where the config file info came from. I think it is important information to get, but since it is not not information kept on the actual filesystem it is information lost, and so really only useful for deployment time. But since we are already printing the actual file used... it may suffice, and I do agree I this does simplify things more. Chinner had added that print line, I just tried to keep it around, if he's cool with the removal of the source stuff now, then sure. 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 6/14/18 11:19 AM, Luis R. Rodriguez wrote: > On Wed, Jun 13, 2018 at 02:37:01PM -0500, Eric Sandeen wrote: >> -static inline const char *default_type_str(enum default_params_type type) >> -{ >> - switch (type) { >> - case DEFAULTS_BUILTIN: >> - return _("package built-in definitions"); >> - case DEFAULTS_CONFIG: >> - return _("package default config file"); >> - case DEFAULTS_CLI_CONFIG: >> - return _("CLI supplied file"); >> - } >> - return _("Unkown\n"); >> -} > >> >> - printf(_("Default configuration sourced from %s\n"), >> - default_type_str(dft.type)); > > The above is really the reason to the type stuff, and it was also why > I added it, as otherwise we had to allocate a string for the old way of > supplying that data. So really this is about the *source*, the type enum > stuff is just a way to implement it without dealing with complexities > on the string. > > So regardless of the type, the question is we are OK to loose the source > of where the config file info came from. But we're not losing that; mkfs now says: # mkfs.xfs -f -c myconfig /dev/sdb1 Configuration file used for defaults: /etc/xfs/mkfs/myconfig ... or # mkfs.xfs -f /dev/sdb1 Configuration file used for defaults: /etc/xfs/mkfs/default ... so it explicitly tells the user what the source of defaults is. I didn't see the point to telling the user "you used a commandline option so we used your commandline option" after they typed a command line option. ;) > I think it is important information > to get, but since it is not not information kept on the actual filesystem it is > information lost, and so really only useful for deployment time. But since > we are already printing the actual file used... it may suffice, and I do > agree I this does simplify things more. > > Chinner had added that print line, I just tried to keep it around, if he's cool > with the removal of the source stuff now, then sure. ok. Thanks, -Eric > 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 Wed, Jun 13, 2018 at 02:37:01PM -0500, Eric Sandeen wrote: > The whole notion of a defaults "type" led to complications that > simply doesn't need to be there. If a configuration file was > used for defaults, simply state that fact and carry on. > > This also removes ambiguity for config files specified as > a relative path by calling realpath. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > config.c | 20 ++++++++------------ > config.h | 31 ------------------------------- > xfs_mkfs.c | 8 +++----- > 3 files changed, 11 insertions(+), 48 deletions(-) > > diff --git a/mkfs/config.c b/mkfs/config.c > index 1a2cd35..3a389ad 100644 > --- a/mkfs/config.c > +++ b/mkfs/config.c > @@ -557,6 +557,7 @@ open_config_file( > { > int dirfd = -1, fd = -1, len, ret = 0; > struct stat st; > + bool cli_specified = false; > > *fpath = malloc(PATH_MAX); > if (!*fpath) > @@ -566,18 +567,19 @@ open_config_file( > > /* first try relative to pwd or absolute path to cli configfile */ > if (config_file) { > - dft->type = DEFAULTS_CLI_CONFIG; > + cli_specified = true; > if (strlen(config_file) > PATH_MAX) { > errno = ENAMETOOLONG; > goto out; > } > - memcpy(*fpath, config_file, strlen(config_file)); > + /* Get absolute path to this file */ > + realpath(config_file, *fpath); > fd = openat(AT_FDCWD, config_file, O_NOFOLLOW, O_RDONLY); > } > > /* on failure search for cli config or default file in sysconfdir */ > if (fd < 0) { > - if (!config_file) > + if (!cli_specified) > config_file = MKFS_XFS_DEFAULT_CONFIG; > len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR, > config_file); > @@ -592,8 +594,6 @@ open_config_file( > fd = openat(dirfd, config_file, O_NOFOLLOW, O_RDONLY); > if (fd < 0) > goto out; > - if (!strcmp(config_file, MKFS_XFS_DEFAULT_CONFIG)) > - dft->type = DEFAULTS_CONFIG; > } > > ret = fstat(fd, &st); > @@ -606,11 +606,9 @@ open_config_file( > out: > /* stat check is always fatal; missing is fatal only if cli-specified */ > if (ret || > - (fd < 0 && dft->type == DEFAULTS_CLI_CONFIG)) { > - fprintf(stderr, > -_("Unable to open %s config file: %s : %s\n"), > - default_type_str(dft->type), *fpath, > - strerror(errno)); > + (fd < 0 && cli_specified)) { > + fprintf(stderr, _("Unable to open config file: %s : %s\n"), > + *fpath, strerror(errno)); > free(*fpath); > exit(1); > } > @@ -645,7 +643,5 @@ parse_defaults_file( > return -1; > } > > - printf(_("config-file=%s\n"), config_file); > - > return 0; > } > diff --git a/mkfs/config.h b/mkfs/config.h > index db22ade..f4af2c7 100644 > --- a/mkfs/config.h > +++ b/mkfs/config.h > @@ -58,22 +58,6 @@ struct sb_feat_args { > }; > > /* > - * File configuration type settings > - * > - * These are the different possibilities by which you can end up parsing > - * default settings with. DEFAULTS_BUILTIN indicates there was no configuration > - * file parsed and we are using the built-in defaults on this code. > - * DEFAULTS_CONFIG means the default configuration file was found and used. > - * DEFAULTS_CLI_CONFIG means the user asked for a custom configuration type > - * through the command line interface and it was used. > - */ > -enum default_params_type { > - DEFAULTS_BUILTIN = 0, > - DEFAULTS_CONFIG, > - DEFAULTS_CLI_CONFIG, > -}; > - > -/* > * Default filesystem features and configuration values > * > * This structure contains the default mkfs values that are to be used when > @@ -82,8 +66,6 @@ enum default_params_type { > * calculations. > */ > struct mkfs_default_params { > - enum default_params_type type; /* where the defaults came from */ > - > int sectorsize; > int blocksize; > > @@ -94,19 +76,6 @@ struct mkfs_default_params { > struct fsxattr fsx; > }; > > -static inline const char *default_type_str(enum default_params_type type) > -{ > - switch (type) { > - case DEFAULTS_BUILTIN: > - return _("package built-in definitions"); > - case DEFAULTS_CONFIG: > - return _("package default config file"); > - case DEFAULTS_CLI_CONFIG: > - return _("CLI supplied file"); > - } > - return _("Unkown\n"); > -} > - > int > open_config_file( > const char *cli_config_file, > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index d32c2c8..56e5f03 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3722,7 +3722,6 @@ main( > > /* build time defaults */ > struct mkfs_default_params dft = { > - .type = DEFAULTS_BUILTIN, > .sectorsize = XFS_MIN_SECTORSIZE, > .blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG, > .sb_feat = { > @@ -3796,19 +3795,18 @@ _("respecification of configuration not allowed\n")); > ret = parse_defaults_file(fd, &dft, config_file); > if (ret) { > fprintf(stderr, > -_("Error parsing %s config file: %s : %s\n"), > - default_type_str(dft.type), > +_("Error parsing config file: %s : %s\n"), > config_file, strerror(errno)); > free(config_file); > close(fd); > exit(1); > } > + printf(_("Configuration file used for defaults: %s\n"), _("EXPERIMENTAL configuration file used for defaults: %s\n") With that, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > + config_file); > free(config_file); > close(fd); > } > > - printf(_("Default configuration sourced from %s\n"), > - default_type_str(dft.type)); > > /* > * Done parsing defaults now, so memcpy defaults into CLI > > -- > 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 Thu, Jun 14, 2018 at 09:41:21AM -0700, Darrick J. Wong wrote: > On Wed, Jun 13, 2018 at 02:37:01PM -0500, Eric Sandeen wrote: > > @@ -3796,19 +3795,18 @@ _("respecification of configuration not allowed\n")); > > ret = parse_defaults_file(fd, &dft, config_file); > > if (ret) { > > fprintf(stderr, > > -_("Error parsing %s config file: %s : %s\n"), > > - default_type_str(dft.type), > > +_("Error parsing config file: %s : %s\n"), > > config_file, strerror(errno)); > > free(config_file); > > close(fd); > > exit(1); > > } > > + printf(_("Configuration file used for defaults: %s\n"), > > _("EXPERIMENTAL configuration file used for defaults: %s\n") I don't see a reason for this to be experimental - we're not screwing with on-disk formats and there's no potential for data corruption or loss, so why do we need to mark this as "don't use this because it might screw up your data"? I'd much prefer we spend the time now to get it right before before shipping it... Which then makes me ask: why don't we have a config file version identifier in the config file? Cheers, Dave.
On Thu, Jun 14, 2018 at 07:10:09PM -0700, Luis R. Rodriguez wrote: > Silly mobile gmail interface not letting me bottom-post... What if we treat > no version being present as version 0? We haven't released anything yet so we should put it in there from the start rather than having to work around the lack of a version field later. Cheers, Dave.
On 6/14/18 9:33 PM, Dave Chinner wrote: > On Thu, Jun 14, 2018 at 07:10:09PM -0700, Luis R. Rodriguez wrote: >> Silly mobile gmail interface not letting me bottom-post... What if we treat >> no version being present as version 0? > > We haven't released anything yet so we should put it in there from > the start rather than having to work around the lack of a version > field later. So, pretend I'm dumb ('cause I often am) and spell it out for me, what would we do with a version? If a config file contains a section or token that some version of mkfs doesn't understand, it'll fail. If we try to read a config file with a too-new version, we'd ... fail? What does this gain us? -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 Thu, Jun 14, 2018 at 09:45:49PM -0500, Eric Sandeen wrote: > > > On 6/14/18 9:33 PM, Dave Chinner wrote: > > On Thu, Jun 14, 2018 at 07:10:09PM -0700, Luis R. Rodriguez wrote: > >> Silly mobile gmail interface not letting me bottom-post... What if we treat > >> no version being present as version 0? > > > > We haven't released anything yet so we should put it in there from > > the start rather than having to work around the lack of a version > > field later. > > So, pretend I'm dumb ('cause I often am) and spell it out for me, what would > we do with a version? > > If a config file contains a section or token that some version of mkfs doesn't > understand, it'll fail. > > If we try to read a config file with a too-new version, we'd ... fail? Fail with a useful error message, rather than do something unexpected or incorrect. Let's face it - the config file is a persistent, on-disk structure that we have to handle in both forwards and backwards compatible manners for many, many years. It's no different to the on-disk format in that respect. Why wouldn't we apply the same guards for format changes we apply to syscalls, ioctls, on-disk formats, etc that all have the same long term compatibility requirements? Cheers, Dave.
t On 6/15/18 7:17 PM, Dave Chinner wrote: > On Thu, Jun 14, 2018 at 09:45:49PM -0500, Eric Sandeen wrote: >> >> >> On 6/14/18 9:33 PM, Dave Chinner wrote: >>> On Thu, Jun 14, 2018 at 07:10:09PM -0700, Luis R. Rodriguez wrote: >>>> Silly mobile gmail interface not letting me bottom-post... What if we treat >>>> no version being present as version 0? >>> >>> We haven't released anything yet so we should put it in there from >>> the start rather than having to work around the lack of a version >>> field later. >> >> So, pretend I'm dumb ('cause I often am) and spell it out for me, what would >> we do with a version? >> >> If a config file contains a section or token that some version of mkfs doesn't >> understand, it'll fail. >> >> If we try to read a config file with a too-new version, we'd ... fail? > > Fail with a useful error message, rather than do something > unexpected or incorrect. like "section 'foo' uknown" or "token 'bar' unknown?" We'd do that today... > Let's face it - the config file is a persistent, on-disk structure > that we have to handle in both forwards and backwards compatible > manners for many, many years. It's no different to the on-disk > format in that respect. Why wouldn't we apply the same guards for > format changes we apply to syscalls, ioctls, on-disk formats, etc > that all have the same long term compatibility requirements? Indeed, with on-disk formats we have compat, incompat, ro-compat .... Are you proposing forward and backward, compat/imcompat ... tokens? Pobably not. Disk formats have that because we may corrupt the things around us if we don't understand the existence of a new metadata type; others we can safely ignore. With ioctls we have a set size; the only time we have versions is if we have padding that should now be parsed in a newer version. I'm trying to find a concrete example of how an incompatible config file version would fail more gracefully than an unknown section/token already fails today. Maybe I lack imagination, but help me out? In the abstract it sounds good. In practice, I'm not yet seeing it. -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 Fri, Jun 15, 2018 at 07:55:02PM -0500, Eric Sandeen wrote: > t > > On 6/15/18 7:17 PM, Dave Chinner wrote: > > On Thu, Jun 14, 2018 at 09:45:49PM -0500, Eric Sandeen wrote: > >> > >> > >> On 6/14/18 9:33 PM, Dave Chinner wrote: > >>> On Thu, Jun 14, 2018 at 07:10:09PM -0700, Luis R. Rodriguez wrote: > >>>> Silly mobile gmail interface not letting me bottom-post... What if we treat > >>>> no version being present as version 0? > >>> > >>> We haven't released anything yet so we should put it in there from > >>> the start rather than having to work around the lack of a version > >>> field later. > >> > >> So, pretend I'm dumb ('cause I often am) and spell it out for me, what would > >> we do with a version? > >> > >> If a config file contains a section or token that some version of mkfs doesn't > >> understand, it'll fail. > >> > >> If we try to read a config file with a too-new version, we'd ... fail? > > > > Fail with a useful error message, rather than do something > > unexpected or incorrect. > > like "section 'foo' uknown" or "token 'bar' unknown?" > We'd do that today... User upgrades xfsprogs version (e.g. has built a git version to sort a problem with repair. Days later, runs mkfs, it throws "unknown foo" errors. User has no idea why, nor how to remedy the problem. As opposed to an error that says "Config file version too old - please update", which tells them exactly what the problem is, and it's easy to understand that it's a result of using the git version rather than the distro packaged version. > > Let's face it - the config file is a persistent, on-disk structure > > that we have to handle in both forwards and backwards compatible > > manners for many, many years. It's no different to the on-disk > > format in that respect. Why wouldn't we apply the same guards for > > format changes we apply to syscalls, ioctls, on-disk formats, etc > > that all have the same long term compatibility requirements? > > Indeed, with on-disk formats we have compat, incompat, ro-compat .... > > Are you proposing forward and backward, compat/imcompat ... tokens? > Pobably not. No. Just a version number. > Disk formats have that because we may corrupt the things around us if > we don't understand the existence of a new metadata type; others we can > safely ignore. > > With ioctls we have a set size; the only time we have versions is if we > have padding that should now be parsed in a newer version. ioctls and syscalls have flags fields in either the API or in the data stuctures that are passed in to indicate what the function and data structures support. This changes over time even when the ioctl/syscall definition and structures don't change size. And if we leave the version field in the same location across revisions of the format, then we have a mechanism for supporting both internal feature support addition/removal and entire file format changes... Cheers, Dave.
On 6/15/18 9:44 PM, Dave Chinner wrote: > On Fri, Jun 15, 2018 at 07:55:02PM -0500, Eric Sandeen wrote: >> t >> >> On 6/15/18 7:17 PM, Dave Chinner wrote: >>> On Thu, Jun 14, 2018 at 09:45:49PM -0500, Eric Sandeen wrote: >>>> ... >>>> So, pretend I'm dumb ('cause I often am) and spell it out for me, what would >>>> we do with a version? >>>> >>>> If a config file contains a section or token that some version of mkfs doesn't >>>> understand, it'll fail. >>>> >>>> If we try to read a config file with a too-new version, we'd ... fail? >>> >>> Fail with a useful error message, rather than do something >>> unexpected or incorrect. >> >> like "section 'foo' uknown" or "token 'bar' unknown?" >> We'd do that today... > > User upgrades xfsprogs version (e.g. has built a git version to sort > a problem with repair. Days later, runs mkfs, it throws "unknown > foo" errors. User has no idea why, nor how to remedy the problem. > As opposed to an error that says "Config file version too old - > please update", which tells them exactly what the problem is, and > it's easy to understand that it's a result of using the git version > rather than the distro packaged version. So the thing I'm still working out is what, exactly, would necessitate a newer version stamped into a config file. I'm being pedantic for two reasons; I don't want to add complexity we don't need, and I don't want to implement versioning that doesn't actually solve the future problem because we failed to clearly define what the versioning means. So bear with me, I guess, while I look at actual concrete examples to be sure we've really defined what versioning would actually mean, and make sure we're explicit about what would require a version bump. There are 2 things I can think of which could cause a config file to become incompatible in some way with a future or past mkfs version; I think these are the same two you alluded to. 1) We add or remove a section or token from mkfs's parsing abilities, within the current file format. tl;dr: This should not change the version. Read on if interested, or skip down to 2) Added/removed directives are indistinguishable, IMHO, from adding or removing a command line option or suboption. Say we remove -n ftype some day, because by now everyone should be using it, or whatever. Or maybe we add -m encryption=1 An older mkfs looking at a newer config file w/ new tokens would simply reject them in the same way as it would reject an unknown commandline option. I don't think we need to rev the version when we add new tokens, we just state that it's unknown, as we do on the commandline: # mkfs.xfs -m encryption=1/dev/blah unknown option -m encryption=1 and with: [metadata] encryption=1 # mkfs.xfs /dev/blah Unknown token at /etc/xfs/mkfs/default:5 : encryption=1 [*] seems equally clear. Same goes for any option that we deprecate and remove. And further, an older or newer config file may not even /contain/ the added or removed directive, so a file cannot be rejected based on version for that reason alone; it can only be accepted or rejected based on the tokens actually found within it - just like on the commandline. So I really don't think adding or removing tokens from the parser for the current cfg file format can possibly require a version change. 2) So maybe we'll change the config file format itself? If we change it to a completely new format like xml or json, we'd be validating the format, not testing a version. We can't just slap "cfg_file_version=2" at the top of an xml or json file and expect it to parse, because... that's not xml or json. So I don't think we can really drop a version directive in today's cfg file and hope it'd be parseable in some new future format's parser. (if we /did/ change to xml we'd have a dtd version...!) So I think the only case I see for bumping the version is if we change some internal parsing detail of the current home-grown file format. (INI is pretty standard and time tested so this seems unlikely, but ok...) Maybe we'll do nested sections, or something, if this has some use in your new subvolume work. Maybe we have a new hierarchy in some weird variant of an ini format? [[foo]] [bar] baz=1 [wut] urk=0 [[zap]] ... I guess this is the only case where I can really see the value. We'd still need to make assumptions about differing binaries being able parse out the version /itself/ (seem xml comment above). So we'd need to assume that we're still in an ini-like world, I guess, or at least not in some other new, well-defined format.. It seems like the safest way to do this would be to require a version number on the first line as a comment, because the "# comment" syntax seems unlikely to change? So something like: # mkfs-xfs-cfg-version-1 which is outside of the normal token parsing, because the whole /point/ of versioning in this case is that the parsing mechanism itself might change, right? So version /must/ be parsed independently from anything else. So in this case we'd reject any newer version than we understand, and if future mkfs ever drops old parsers, those would reject versions older than what it supports. So I've talked myself into needing a version, now to see if you agree with me about which circumstances would actually trigger a version change, and whether the version parsing itself needs to be separate from the standard parsing, because it indicates the required parsing mechanism required for the /rest/ of the file... Thanks, -Eric [*] It doesn't say this today, but we should fix it to distinguish parsing errors from unknown sections and tokens, then we can say "Unknown or unsupported token ..." etc. -- 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
diff --git a/mkfs/config.c b/mkfs/config.c index 1a2cd35..3a389ad 100644 --- a/mkfs/config.c +++ b/mkfs/config.c @@ -557,6 +557,7 @@ open_config_file( { int dirfd = -1, fd = -1, len, ret = 0; struct stat st; + bool cli_specified = false; *fpath = malloc(PATH_MAX); if (!*fpath) @@ -566,18 +567,19 @@ open_config_file( /* first try relative to pwd or absolute path to cli configfile */ if (config_file) { - dft->type = DEFAULTS_CLI_CONFIG; + cli_specified = true; if (strlen(config_file) > PATH_MAX) { errno = ENAMETOOLONG; goto out; } - memcpy(*fpath, config_file, strlen(config_file)); + /* Get absolute path to this file */ + realpath(config_file, *fpath); fd = openat(AT_FDCWD, config_file, O_NOFOLLOW, O_RDONLY); } /* on failure search for cli config or default file in sysconfdir */ if (fd < 0) { - if (!config_file) + if (!cli_specified) config_file = MKFS_XFS_DEFAULT_CONFIG; len = snprintf(*fpath, PATH_MAX, "%s/%s", MKFS_XFS_CONF_DIR, config_file); @@ -592,8 +594,6 @@ open_config_file( fd = openat(dirfd, config_file, O_NOFOLLOW, O_RDONLY); if (fd < 0) goto out; - if (!strcmp(config_file, MKFS_XFS_DEFAULT_CONFIG)) - dft->type = DEFAULTS_CONFIG; } ret = fstat(fd, &st); @@ -606,11 +606,9 @@ open_config_file( out: /* stat check is always fatal; missing is fatal only if cli-specified */ if (ret || - (fd < 0 && dft->type == DEFAULTS_CLI_CONFIG)) { - fprintf(stderr, -_("Unable to open %s config file: %s : %s\n"), - default_type_str(dft->type), *fpath, - strerror(errno)); + (fd < 0 && cli_specified)) { + fprintf(stderr, _("Unable to open config file: %s : %s\n"), + *fpath, strerror(errno)); free(*fpath); exit(1); } @@ -645,7 +643,5 @@ parse_defaults_file( return -1; } - printf(_("config-file=%s\n"), config_file); - return 0; } diff --git a/mkfs/config.h b/mkfs/config.h index db22ade..f4af2c7 100644 --- a/mkfs/config.h +++ b/mkfs/config.h @@ -58,22 +58,6 @@ struct sb_feat_args { }; /* - * File configuration type settings - * - * These are the different possibilities by which you can end up parsing - * default settings with. DEFAULTS_BUILTIN indicates there was no configuration - * file parsed and we are using the built-in defaults on this code. - * DEFAULTS_CONFIG means the default configuration file was found and used. - * DEFAULTS_CLI_CONFIG means the user asked for a custom configuration type - * through the command line interface and it was used. - */ -enum default_params_type { - DEFAULTS_BUILTIN = 0, - DEFAULTS_CONFIG, - DEFAULTS_CLI_CONFIG, -}; - -/* * Default filesystem features and configuration values * * This structure contains the default mkfs values that are to be used when @@ -82,8 +66,6 @@ enum default_params_type { * calculations. */ struct mkfs_default_params { - enum default_params_type type; /* where the defaults came from */ - int sectorsize; int blocksize; @@ -94,19 +76,6 @@ struct mkfs_default_params { struct fsxattr fsx; }; -static inline const char *default_type_str(enum default_params_type type) -{ - switch (type) { - case DEFAULTS_BUILTIN: - return _("package built-in definitions"); - case DEFAULTS_CONFIG: - return _("package default config file"); - case DEFAULTS_CLI_CONFIG: - return _("CLI supplied file"); - } - return _("Unkown\n"); -} - int open_config_file( const char *cli_config_file, diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index d32c2c8..56e5f03 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -3722,7 +3722,6 @@ main( /* build time defaults */ struct mkfs_default_params dft = { - .type = DEFAULTS_BUILTIN, .sectorsize = XFS_MIN_SECTORSIZE, .blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG, .sb_feat = { @@ -3796,19 +3795,18 @@ _("respecification of configuration not allowed\n")); ret = parse_defaults_file(fd, &dft, config_file); if (ret) { fprintf(stderr, -_("Error parsing %s config file: %s : %s\n"), - default_type_str(dft.type), +_("Error parsing config file: %s : %s\n"), config_file, strerror(errno)); free(config_file); close(fd); exit(1); } + printf(_("Configuration file used for defaults: %s\n"), + config_file); free(config_file); close(fd); } - printf(_("Default configuration sourced from %s\n"), - default_type_str(dft.type)); /* * Done parsing defaults now, so memcpy defaults into CLI
The whole notion of a defaults "type" led to complications that simply doesn't need to be there. If a configuration file was used for defaults, simply state that fact and carry on. This also removes ambiguity for config files specified as a relative path by calling realpath. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- config.c | 20 ++++++++------------ config.h | 31 ------------------------------- xfs_mkfs.c | 8 +++----- 3 files changed, 11 insertions(+), 48 deletions(-) -- 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