[v2] mkfs: add mkfs config version specifier for the config file
diff mbox

Message ID 20180618180520.22146-1-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain June 18, 2018, 6:05 p.m. UTC
We may opt later to bump this and add new features which we
don't want a v1 parser to use. So peg a version number. We add
a new [global] section dedicated for special configuration file
option specifiers, with the only subtopt supported being 'version'.

The only we support for now is version 1.

Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---

As suggested by Sandeed, added a [global] section which makes the
code easier to read and maintain.

 man/man8/mkfs.xfs.8.in |  9 ++++-
 mkfs/config.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 1 deletion(-)

Comments

Eric Sandeen June 19, 2018, 7:53 p.m. UTC | #1
On 6/18/18 1:05 PM, Luis R. Rodriguez wrote:
> We may opt later to bump this and add new features which we
> don't want a v1 parser to use. So peg a version number. We add
> a new [global] section dedicated for special configuration file
> option specifiers, with the only subtopt supported being 'version'.
> 
> The only we support for now is version 1.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> ---
> 
> As suggested by Sandeed, added a [global] section which makes the
> code easier to read and maintain.

Soo.... 

I'm really on the fence about versioning at all, because we have yet to
identify any concrete scenario in which we'd need to bump the version;
we haven't even defined what exactly the "version" implies.

Here's my high-level view.

1) The config file is a 1:1 mapping from [section] to option
([metadata]::"-m") and from token to suboption (crc=0::crc=0).
So it's really just an alternate representation of a command line.

2) We have no "version" on the command line - new options come, and rarely old
options sometimes go, and invalid command lines will issue informative errors.
The same should be expected from the config file as it gains or loses the
ability to parse any given section or token.  Directives coming or going
won't require a new version.

3) The config file is in INI format, which is trivial and well-established,
and exceedingly unlikely to change, so I can't foresee any situation where
we'd need to know the version in order to parse the file.

3a) If the config file format /did/ change, we have a chicken and egg problem
where we have to parse a potentially unsupported format in order to get the
version out.

So unless anyone has a concrete example of how a version would be needed,
functional, reliably-parsed, and well-defined I'm inclined to leave this out.

Thanks,
-Eric

>  man/man8/mkfs.xfs.8.in |  9 ++++-
>  mkfs/config.c          | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 99 insertions(+), 1 deletion(-)
> 
> diff --git a/man/man8/mkfs.xfs.8.in b/man/man8/mkfs.xfs.8.in
> index cf4bdf827d9c..c86d559ba3e1 100644
> --- a/man/man8/mkfs.xfs.8.in
> +++ b/man/man8/mkfs.xfs.8.in
> @@ -965,12 +965,19 @@ option follow a simple ini-style format as shown below.
>  Available options consist of a small subset of the parameters available
>  via the
>  .BR mkfs.xfs (8)
> -command line.
> +command line. All configuration files must start with an annotation of
> +the configuration file format, specified in the global section with the keyword
> +.B version
> +and the only supported version format is 1.
>  Currently all default parameters can only be either enabled or disabled,
>  with a value of 1 to enable or 0 to disable.
>  See below for a list of all supported configuration parameters and their
>  current built-in default settings.
>  .PP
> +.BI [global]
> +.br
> +.BI version=1
> +.PP
>  .BI [data]
>  .br
>  .BI noalign=0
> diff --git a/mkfs/config.c b/mkfs/config.c
> index 835adc45f02d..4bb371d6b1e8 100644
> --- a/mkfs/config.c
> +++ b/mkfs/config.c
> @@ -32,6 +32,8 @@
>   * We only provide definitions for what we currently support parsing.
>   */
>  
> +static uint64_t mkfs_config_version;
> +
>  enum data_subopts {
>  	D_NOALIGN = 0,
>  };
> @@ -42,6 +44,14 @@ enum inode_subopts {
>  	I_SPINODES,
>  };
>  
> +/*
> + * These are not part of the defaults but rather global generic configuration
> + * file options.
> + */
> +enum global_subopts {
> +	G_VERSION = 0,
> +};
> +
>  enum log_subopts {
>  	L_LAZYSBCNTR = 0,
>  };
> @@ -122,6 +132,24 @@ inode_config_parser(
>  	return -1;
>  }
>  
> +static int
> +global_config_parser(
> +	struct mkfs_default_params	*dft,
> +	int				psubopt,
> +	uint64_t			value)
> +{
> +	enum global_subopts		subopt = psubopt;
> +
> +	switch (subopt) {
> +	case G_VERSION:
> +		if (value != 1)
> +			return -1;
> +		mkfs_config_version = value;
> +		return 0;
> +	}
> +	return -1;
> +}
> +
>  static int
>  log_config_parser(
>  	struct mkfs_default_params	*dft,
> @@ -234,6 +262,14 @@ struct confopts {
>  		},
>  		.parser = inode_config_parser,
>  	},
> +	{
> +		.name = "global",
> +		.subopts = {
> +			[G_VERSION] = "version",
> +			NULL
> +		},
> +		.parser = global_config_parser,
> +	},
>  	{
>  		.name = "log",
>  		.subopts = {
> @@ -287,6 +323,24 @@ get_confopts(
>  	return NULL;
>  }
>  
> +static bool config_version_set(void)
> +{
> +	/* cannot fail */
> +	struct confopts *opts = get_confopts("global");
> +
> +	return opts->seen;
> +}
> +
> +static uint64_t config_version(void)
> +{
> +	/* cannot fail */
> +	struct confopts *opts = get_confopts("global");
> +
> +	if (!opts->seen)
> +		return 0;
> +	return mkfs_config_version;
> +}
> +
>  enum parse_line_type {
>  	PARSE_COMMENT = 0,
>  	PARSE_EMPTY,
> @@ -379,6 +433,32 @@ parse_get_line_type(
>  	return PARSE_INVALID;
>  }
>  
> +static bool
> +check_version_supported(
> +		void)
> +{
> +	uint64_t version = config_version();
> +
> +	if (!version) {
> +		fprintf(stderr,
> +_("Version for configuration file not specified\n"));
> +		goto out;
> +	}
> +
> +	if (version != 1) {
> +		errno = EOPNOTSUPP;
> +		fprintf(stderr,
> +_("Only version 1 supported, you specified version %lu\n"), version);
> +		goto out;
> +	}
> +
> +	return true;
> +
> +out:
> +	errno = EOPNOTSUPP;
> +	return false;
> +}
> +
>  static int
>  parse_config_stream(
>  	struct mkfs_default_params	*dft,
> @@ -420,6 +500,7 @@ parse_config_stream(
>  					  config_file, lineno, line);
>  			goto out;
>  		case PARSE_SECTION:
> +
>  			confopt = get_confopts(tag);
>  			if (!confopt) {
>  				fprintf(stderr,
> @@ -451,6 +532,13 @@ _("No section specified yet on line %s:%zu : %s\n"),
>  				goto out_free_tag;
>  			}
>  
> +			if (!config_version_set() &&
> +			    strcmp(confopt->name, "global") != 0) {
> +				fprintf(stderr,
> +_("A global section with a version set was not detected and must be set first\n"));
> +				goto out_free_tag;
> +			}
> +
>  			/*
>  			 * We re-use the line buffer allocated by getline(),
>  			 * however line must be kept pointing to its original
> @@ -486,6 +574,9 @@ _("Error parsing line %s:%zu : %s\n"),
>  				goto out;
>  			}
>  
> +			if (!check_version_supported())
> +				goto out;
> +
>  			break;
>  		}
>  		free(line);
> 
--
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
Luis Chamberlain June 19, 2018, 8:57 p.m. UTC | #2
On Tue, Jun 19, 2018 at 02:53:28PM -0500, Eric Sandeen wrote:
> Soo.... 
> 
> I'm really on the fence about versioning at all, because we have yet to
> identify any concrete scenario in which we'd need to bump the version;
> we haven't even defined what exactly the "version" implies.

Right.

> So unless anyone has a concrete example of how a version would be needed,
> functional, reliably-parsed, and well-defined I'm inclined to leave this out.

Dave asked for it, so I would imagine he could make a better case for it.

But from a generic file format point of view, a version typically does
make sense for a slew of reasons, but it is typically the not known, the
unexpected, for which it can be a life saver.

Things that I can mention other than what you mentioned (whether or not
they are good, this is just a list of crazy things):

 1) We move from using our own parser to e2fsprogs profile parser (recall
    that my studies revealed it was the smallest) or lib_iniconfig (more
    robust, and widely used, however a kitchen sink compared to e2fsprogs
    profile parser)

    Both however support the INI format we are using.

    The version however may help to make the old parser not barf if
    say we added some magic in the future which would make the old
    parser barf.

 2) Data type change for an existing token. Say crc=2 was invented
    (again just an example), it would barf on the old parser, but with
    the version check it would immediately tell you a more meaningful
    message.

Perhaps the biggest advantage I can think of then then is a better error
message for a set of supported features, starting with what we have today
reflected as parsed as version 1. This does differ from what we do with
the CLI, however as with the respecification simplifcation done on
config.c in contrast to the CLI respecification checks, I would see
this as a win for users, and yet keeps things simple.

If this seems acceptable and reasonable, then version would be a reflection
of supporting the format and layout of the file, as well was indicative
of supporting only a specific feature set.

Thoughts?

  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
Eric Sandeen June 19, 2018, 9:21 p.m. UTC | #3
On 6/19/18 3:57 PM, Luis R. Rodriguez wrote:
> On Tue, Jun 19, 2018 at 02:53:28PM -0500, Eric Sandeen wrote:
>> Soo.... 
>>
>> I'm really on the fence about versioning at all, because we have yet to
>> identify any concrete scenario in which we'd need to bump the version;
>> we haven't even defined what exactly the "version" implies.
> 
> Right.
> 
>> So unless anyone has a concrete example of how a version would be needed,
>> functional, reliably-parsed, and well-defined I'm inclined to leave this out.
> 
> Dave asked for it, so I would imagine he could make a better case for it.

Yup; well, I spoke with Dave and he was kind of leaving it as an exercise
for the reader, an exercise in which we are now engaged.  :)

> But from a generic file format point of view, a version typically does
> make sense for a slew of reasons, but it is typically the not known, the
> unexpected, for which it can be a life saver.
> 
> Things that I can mention other than what you mentioned (whether or not
> they are good, this is just a list of crazy things):
> 
>  1) We move from using our own parser to e2fsprogs profile parser (recall
>     that my studies revealed it was the smallest) or lib_iniconfig (more
>     robust, and widely used, however a kitchen sink compared to e2fsprogs
>     profile parser)
> 
>     Both however support the INI format we are using.
> 
>     The version however may help to make the old parser not barf if
>     say we added some magic in the future which would make the old
>     parser barf.

... if the file format does not change, a parser change doesn't matter, and
a version change would be pointless.

>  2) Data type change for an existing token. Say crc=2 was invented
>     (again just an example), it would barf on the old parser, but with
>     the version check it would immediately tell you a more meaningful
>     message.

But how is that any different from what it would do on the command line for
a similar change?

"Invalid value 2 for token crc=2 on line XYZ" is completely reasonable, and
does not require any version information.

In fact, what you are suggesting here means we'd bump the version if and only
if the config file contained "crc=2" which makes little sense.

> Perhaps the biggest advantage I can think of then then is a better error
> message for a set of supported features, starting with what we have today
> reflected as parsed as version 1. This does differ from what we do with
> the CLI, however as with the respecification simplifcation done on
> config.c in contrast to the CLI respecification checks, I would see
> this as a win for users, and yet keeps things simple.

See my earlier email for why versioning cannot depend on the presence or
absence of any parseable tokens ... stating "token XYZ not supported"
should suffice IMHO.

(now, it's true that today we don't distinguish between unknown vs.
unsupported tokens, I don't know if I should try to fix that before 4.17
or not; again, though, that is on the parser side, not the config file side.)

> If this seems acceptable and reasonable, then version would be a reflection
> of supporting the format and layout of the file, as well was indicative
> of supporting only a specific feature set.

What is or is not a supported token is wholly a function of the mkfs/parser
version, not the config file version, right?

So, not to be argumentative ... but I remain unconvinced of the need for
config file versioning.

Thanks,
-Eric


> Thoughts?
> 
>   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
Luis Chamberlain June 19, 2018, 11:52 p.m. UTC | #4
On Tue, Jun 19, 2018 at 04:21:25PM -0500, Eric Sandeen wrote:
> On 6/19/18 3:57 PM, Luis R. Rodriguez wrote:
> > But from a generic file format point of view, a version typically does
> > make sense for a slew of reasons, but it is typically the not known, the
> > unexpected, for which it can be a life saver.
> > 
> > Things that I can mention other than what you mentioned (whether or not
> > they are good, this is just a list of crazy things):
> > 
> >  1) We move from using our own parser to e2fsprogs profile parser (recall
> >     that my studies revealed it was the smallest) or lib_iniconfig (more
> >     robust, and widely used, however a kitchen sink compared to e2fsprogs
> >     profile parser)
> > 
> >     Both however support the INI format we are using.
> > 
> >     The version however may help to make the old parser not barf if
> >     say we added some magic in the future which would make the old
> >     parser barf.
> 
> ... if the file format does not change, a parser change doesn't matter, and
> a version change would be pointless.

It can. Consider support for different comment styles.

> >  2) Data type change for an existing token. Say crc=2 was invented
> >     (again just an example), it would barf on the old parser, but with
> >     the version check it would immediately tell you a more meaningful
> >     message.
> 
> But how is that any different from what it would do on the command line for
> a similar change?

So long as we remain forward compatible its a non-issue. If we say decide
to make a unit not compatible with old values then surely that's a format
change which would break old configs.

We should keep the config file parser ompatible with old config files produced.
That means anything and everything we allow today should keep working in the
future, and one way to sanely break that compatibility would be a version bump.

> "Invalid value 2 for token crc=2 on line XYZ" is completely reasonable, and
> does not require any version information.
> 
> In fact, what you are suggesting here means we'd bump the version if and only
> if the config file contained "crc=2" which makes little sense.

For example, a config file in the future with format 2 with crc=2 would not
work with and older xfsprog which only supports format version 1. Saying
that your xfsprogs does not support version 2 is IMHO clearer what the
issue may be.

> > message for a set of supported features, starting with what we have today
> > reflected as parsed as version 1. This does differ from what we do with
> > the CLI, however as with the respecification simplifcation done on
> > config.c in contrast to the CLI respecification checks, I would see
> > this as a win for users, and yet keeps things simple.
> 
> See my earlier email for why versioning cannot depend on the presence or
> absence of any parseable tokens ... stating "token XYZ not supported"
> should suffice IMHO.

It can. And ultimately its up to you to pick and choose.

> (now, it's true that today we don't distinguish between unknown vs.
> unsupported tokens, I don't know if I should try to fix that before 4.17
> or not; again, though,

I don't think its worth to say something is unsupported vs unknown given
we don't know what options will be supported, as some config file entries
are expected to differ from CLI.

> that is on the parser side, not the config file side.)
> 
> > If this seems acceptable and reasonable, then version would be a reflection
> > of supporting the format and layout of the file, as well was indicative
> > of supporting only a specific feature set.
> 
> What is or is not a supported token is wholly a function of the mkfs/parser
> version, not the config file version, right?

Yes.

> So, not to be argumentative ... but I remain unconvinced of the need for
> config file versioning.

Your call :)

  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
Jan Tulak June 20, 2018, 1:11 p.m. UTC | #5
On Wed, Jun 20, 2018 at 1:52 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Tue, Jun 19, 2018 at 04:21:25PM -0500, Eric Sandeen wrote:
>> On 6/19/18 3:57 PM, Luis R. Rodriguez wrote:
>> > But from a generic file format point of view, a version typically does
>> > make sense for a slew of reasons, but it is typically the not known, the
>> > unexpected, for which it can be a life saver.
>> >
>> > Things that I can mention other than what you mentioned (whether or not
>> > they are good, this is just a list of crazy things):
>> >
>> >  1) We move from using our own parser to e2fsprogs profile parser (recall
>> >     that my studies revealed it was the smallest) or lib_iniconfig (more
>> >     robust, and widely used, however a kitchen sink compared to e2fsprogs
>> >     profile parser)
>> >
>> >     Both however support the INI format we are using.
>> >
>> >     The version however may help to make the old parser not barf if
>> >     say we added some magic in the future which would make the old
>> >     parser barf.
>>
>> ... if the file format does not change, a parser change doesn't matter, and
>> a version change would be pointless.
>
> It can. Consider support for different comment styles.

IMO comments are part of the format and if they change, then the
format itself was changed too. So I find it difficult to see any
practical use for a config version, but all arguments about it were
already said. Still, if we end up with something like this, let's make
the section to be optional.

If we ever happen to find ourselves in such situation and we need the
version check as an emergency brake, then we can specify that the v2
format has to begin with version=2. The old parser will understand it
and abort, but if it is not present, it is equal to version=1 and we
will not pollute the configs in the meantime.

Cheers,
Jan
--
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
Dave Chinner June 20, 2018, 11:18 p.m. UTC | #6
On Tue, Jun 19, 2018 at 10:57:10PM +0200, Luis R. Rodriguez wrote:
> On Tue, Jun 19, 2018 at 02:53:28PM -0500, Eric Sandeen wrote:
> > Soo.... 
> > 
> > I'm really on the fence about versioning at all, because we have yet to
> > identify any concrete scenario in which we'd need to bump the version;
> > we haven't even defined what exactly the "version" implies.
> 
> Right.
> 
> > So unless anyone has a concrete example of how a version would be needed,
> > functional, reliably-parsed, and well-defined I'm inclined to leave this out.
> 
> Dave asked for it, so I would imagine he could make a better case for it.

I ask for things so that people think about the problem, not
necessarily because I want something to be done. Please don't
conflate "Dave says X" with "we must implement X" because I am often
wrong and/or playing the Devil's Advocate role.

I try to look outside the box for potential issues, so if I see a
potential issue I'll say /something/ just to get people to discuss
that aspect of the problem and come up with a reasonable solution. I
don't have the time to solve every problem we have, I don't
/want/ to solve every problem, but I want to make sure we solve
the problems we have in the right way the first time.

In reality, I don't care either way if we have a version symbol in
the config file, and if it is decided that it is a good thing how it
will be implemented. What I care about is that we've thought about
supporting config files over their entire lifetime, not just
considered what is needed in the near term to solve the immediate
problem.

IOWs, my comments about treating a config file like any other
long term persistent structure were intended to shift the focus of
the discussion from implementation minutae to the bigger, long term
picture details that needed to be thrashed out. Work out the big
picture first, the small details will fall naturally from that...

Cheers,

Dave.

Patch
diff mbox

diff --git a/man/man8/mkfs.xfs.8.in b/man/man8/mkfs.xfs.8.in
index cf4bdf827d9c..c86d559ba3e1 100644
--- a/man/man8/mkfs.xfs.8.in
+++ b/man/man8/mkfs.xfs.8.in
@@ -965,12 +965,19 @@  option follow a simple ini-style format as shown below.
 Available options consist of a small subset of the parameters available
 via the
 .BR mkfs.xfs (8)
-command line.
+command line. All configuration files must start with an annotation of
+the configuration file format, specified in the global section with the keyword
+.B version
+and the only supported version format is 1.
 Currently all default parameters can only be either enabled or disabled,
 with a value of 1 to enable or 0 to disable.
 See below for a list of all supported configuration parameters and their
 current built-in default settings.
 .PP
+.BI [global]
+.br
+.BI version=1
+.PP
 .BI [data]
 .br
 .BI noalign=0
diff --git a/mkfs/config.c b/mkfs/config.c
index 835adc45f02d..4bb371d6b1e8 100644
--- a/mkfs/config.c
+++ b/mkfs/config.c
@@ -32,6 +32,8 @@ 
  * We only provide definitions for what we currently support parsing.
  */
 
+static uint64_t mkfs_config_version;
+
 enum data_subopts {
 	D_NOALIGN = 0,
 };
@@ -42,6 +44,14 @@  enum inode_subopts {
 	I_SPINODES,
 };
 
+/*
+ * These are not part of the defaults but rather global generic configuration
+ * file options.
+ */
+enum global_subopts {
+	G_VERSION = 0,
+};
+
 enum log_subopts {
 	L_LAZYSBCNTR = 0,
 };
@@ -122,6 +132,24 @@  inode_config_parser(
 	return -1;
 }
 
+static int
+global_config_parser(
+	struct mkfs_default_params	*dft,
+	int				psubopt,
+	uint64_t			value)
+{
+	enum global_subopts		subopt = psubopt;
+
+	switch (subopt) {
+	case G_VERSION:
+		if (value != 1)
+			return -1;
+		mkfs_config_version = value;
+		return 0;
+	}
+	return -1;
+}
+
 static int
 log_config_parser(
 	struct mkfs_default_params	*dft,
@@ -234,6 +262,14 @@  struct confopts {
 		},
 		.parser = inode_config_parser,
 	},
+	{
+		.name = "global",
+		.subopts = {
+			[G_VERSION] = "version",
+			NULL
+		},
+		.parser = global_config_parser,
+	},
 	{
 		.name = "log",
 		.subopts = {
@@ -287,6 +323,24 @@  get_confopts(
 	return NULL;
 }
 
+static bool config_version_set(void)
+{
+	/* cannot fail */
+	struct confopts *opts = get_confopts("global");
+
+	return opts->seen;
+}
+
+static uint64_t config_version(void)
+{
+	/* cannot fail */
+	struct confopts *opts = get_confopts("global");
+
+	if (!opts->seen)
+		return 0;
+	return mkfs_config_version;
+}
+
 enum parse_line_type {
 	PARSE_COMMENT = 0,
 	PARSE_EMPTY,
@@ -379,6 +433,32 @@  parse_get_line_type(
 	return PARSE_INVALID;
 }
 
+static bool
+check_version_supported(
+		void)
+{
+	uint64_t version = config_version();
+
+	if (!version) {
+		fprintf(stderr,
+_("Version for configuration file not specified\n"));
+		goto out;
+	}
+
+	if (version != 1) {
+		errno = EOPNOTSUPP;
+		fprintf(stderr,
+_("Only version 1 supported, you specified version %lu\n"), version);
+		goto out;
+	}
+
+	return true;
+
+out:
+	errno = EOPNOTSUPP;
+	return false;
+}
+
 static int
 parse_config_stream(
 	struct mkfs_default_params	*dft,
@@ -420,6 +500,7 @@  parse_config_stream(
 					  config_file, lineno, line);
 			goto out;
 		case PARSE_SECTION:
+
 			confopt = get_confopts(tag);
 			if (!confopt) {
 				fprintf(stderr,
@@ -451,6 +532,13 @@  _("No section specified yet on line %s:%zu : %s\n"),
 				goto out_free_tag;
 			}
 
+			if (!config_version_set() &&
+			    strcmp(confopt->name, "global") != 0) {
+				fprintf(stderr,
+_("A global section with a version set was not detected and must be set first\n"));
+				goto out_free_tag;
+			}
+
 			/*
 			 * We re-use the line buffer allocated by getline(),
 			 * however line must be kept pointing to its original
@@ -486,6 +574,9 @@  _("Error parsing line %s:%zu : %s\n"),
 				goto out;
 			}
 
+			if (!check_version_supported())
+				goto out;
+
 			break;
 		}
 		free(line);