[v2,3/5] mkfs: replace defaults source with an enum
diff mbox

Message ID 20180517192700.23457-4-mcgrof@kernel.org
State Superseded
Headers show

Commit Message

Luis Chamberlain May 17, 2018, 7:26 p.m. UTC
Using an enum will let us later just use a switch statement to print
out the source, this makes sources easier to document, update and
manage.

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

Comments

Dave Chinner May 17, 2018, 10:48 p.m. UTC | #1
On Thu, May 17, 2018 at 12:26:58PM -0700, Luis R. Rodriguez wrote:
> Using an enum will let us later just use a switch statement to print
> out the source, this makes sources easier to document, update and
> manage.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

This is incomplete. :(

> ---
>  mkfs/xfs_mkfs.c        |  5 +++--
>  mkfs/xfs_mkfs_common.h | 13 ++++++++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index ac97039abc34..de0eab3f68e0 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3697,7 +3697,7 @@ main(
>  
>  	/* build time defaults */
>  	struct mkfs_default_params	dft = {
> -		.source = _("package build definitions"),
> +		.type = DEFAULTS_BUILTIN,
>  		.sectorsize = XFS_MIN_SECTORSIZE,
>  		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
>  		.sb_feat = {
> @@ -3737,7 +3737,8 @@ main(
>  	 * implemented, emit a message to indicate where the defaults being
>  	 * used came from.
>  	 *
> -	 * printf(_("Default configuration sourced from %s\n"), dft.source);
> +	 * printf(_("Default configuration sourced from %s\n"),
> +	 *	  default_type_str(dft.type));

This function does not exist in the patch. If you are going to add
functionality, first turn on that functionality so you can test the
patch actually works...

>  	 */
>  
>  	/* copy new defaults into CLI parsing structure */
> diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
> index 9b0f67b70cf1..d867ab377185 100644
> --- a/mkfs/xfs_mkfs_common.h
> +++ b/mkfs/xfs_mkfs_common.h
> @@ -40,6 +40,17 @@ struct sb_feat_args {
>  	bool	nortalign;
>  };
>  
> +/*
> + * File configuration type settings
> + *
> + * These are the different possibilities by which you can end up parsing
> + * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
> + * file parsed and we are using the built-in defaults on this code.
> + */
> +enum default_params_type {
> +	DEFAULTS_BUILTIN = 0,
> +};

Please add all the new types here, the functions to print the names,
etc.

>  /*
>   * Default filesystem features and configuration values
>   *
> @@ -49,7 +60,7 @@ struct sb_feat_args {
>   * calculations.
>   */
>  struct mkfs_default_params {
> -	char	*source;	/* where the defaults came from */
> +	enum default_params_type type; /* where the defaults came from */
>  
>  	int	sectorsize;
>  	int	blocksize;

As it is, I don't see why this change it necessary - you can just
store the appropriate string (as the code currently does) into the
structure once the source is known. Why do we need infrastructure to
abstract printing a string when we set it directly, anyway?

Cheers,

Dave.
Luis Chamberlain May 17, 2018, 11:09 p.m. UTC | #2
On Fri, May 18, 2018 at 08:48:49AM +1000, Dave Chinner wrote:
> On Thu, May 17, 2018 at 12:26:58PM -0700, Luis R. Rodriguez wrote:
> > Using an enum will let us later just use a switch statement to print
> > out the source, this makes sources easier to document, update and
> > manage.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> 
> This is incomplete. :(

It builds, the comment was intentional.

> > ---
> >  mkfs/xfs_mkfs.c        |  5 +++--
> >  mkfs/xfs_mkfs_common.h | 13 ++++++++++++-
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index ac97039abc34..de0eab3f68e0 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3697,7 +3697,7 @@ main(
> >  
> >  	/* build time defaults */
> >  	struct mkfs_default_params	dft = {
> > -		.source = _("package build definitions"),
> > +		.type = DEFAULTS_BUILTIN,
> >  		.sectorsize = XFS_MIN_SECTORSIZE,
> >  		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
> >  		.sb_feat = {
> > @@ -3737,7 +3737,8 @@ main(
> >  	 * implemented, emit a message to indicate where the defaults being
> >  	 * used came from.
> >  	 *
> > -	 * printf(_("Default configuration sourced from %s\n"), dft.source);
> > +	 * printf(_("Default configuration sourced from %s\n"),
> > +	 *	  default_type_str(dft.type));
> 
> This function does not exist in the patch. If you are going to add
> functionality, first turn on that functionality so you can test the
> patch actually works...

The function is in a comment though. I'll go ahead and add it along
with the other enums then.

> >  	 */
> >  
> >  	/* copy new defaults into CLI parsing structure */
> > diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
> > index 9b0f67b70cf1..d867ab377185 100644
> > --- a/mkfs/xfs_mkfs_common.h
> > +++ b/mkfs/xfs_mkfs_common.h
> > @@ -40,6 +40,17 @@ struct sb_feat_args {
> >  	bool	nortalign;
> >  };
> >  
> > +/*
> > + * File configuration type settings
> > + *
> > + * These are the different possibilities by which you can end up parsing
> > + * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
> > + * file parsed and we are using the built-in defaults on this code.
> > + */
> > +enum default_params_type {
> > +	DEFAULTS_BUILTIN = 0,
> > +};
> 
> Please add all the new types here, the functions to print the names,
> etc.

Ok.

> >  /*
> >   * Default filesystem features and configuration values
> >   *
> > @@ -49,7 +60,7 @@ struct sb_feat_args {
> >   * calculations.
> >   */
> >  struct mkfs_default_params {
> > -	char	*source;	/* where the defaults came from */
> > +	enum default_params_type type; /* where the defaults came from */
> >  
> >  	int	sectorsize;
> >  	int	blocksize;
> 
> As it is, I don't see why this change it necessary - you can just
> store the appropriate string (as the code currently does) into the
> structure once the source is known. Why do we need infrastructure to
> abstract printing a string when we set it directly, anyway?

Using an enum we get to document the different sources clearly, and we
also get to have an enum to compare against for conditionals later,
instead of having to strcmp().

  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
Dave Chinner May 18, 2018, 12:53 a.m. UTC | #3
On Thu, May 17, 2018 at 04:09:18PM -0700, Luis R. Rodriguez wrote:
> On Fri, May 18, 2018 at 08:48:49AM +1000, Dave Chinner wrote:
> > On Thu, May 17, 2018 at 12:26:58PM -0700, Luis R. Rodriguez wrote:
> > > Using an enum will let us later just use a switch statement to print
> > > out the source, this makes sources easier to document, update and
> > > manage.
> > > 
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
> > 
> > This is incomplete. :(
> 
> It builds, the comment was intentional.

That doesn't make it complete. Usually when introducing new code one
part at a time, the entire functionality of that part is separated
out so it can be reviewed whole, not split across multilple patches
and intermingled with other new functionality....

> > >  /*
> > >   * Default filesystem features and configuration values
> > >   *
> > > @@ -49,7 +60,7 @@ struct sb_feat_args {
> > >   * calculations.
> > >   */
> > >  struct mkfs_default_params {
> > > -	char	*source;	/* where the defaults came from */
> > > +	enum default_params_type type; /* where the defaults came from */
> > >  
> > >  	int	sectorsize;
> > >  	int	blocksize;
> > 
> > As it is, I don't see why this change it necessary - you can just
> > store the appropriate string (as the code currently does) into the
> > structure once the source is known. Why do we need infrastructure to
> > abstract printing a string when we set it directly, anyway?
> 
> Using an enum we get to document the different sources clearly, and we
> also get to have an enum to compare against for conditionals later,
> instead of having to strcmp().

See my comments in later patches, where I suggest all that gets
removed because i don't think it works. :P

Cheers,

Dave.

Patch
diff mbox

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index ac97039abc34..de0eab3f68e0 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3697,7 +3697,7 @@  main(
 
 	/* build time defaults */
 	struct mkfs_default_params	dft = {
-		.source = _("package build definitions"),
+		.type = DEFAULTS_BUILTIN,
 		.sectorsize = XFS_MIN_SECTORSIZE,
 		.blocksize = 1 << XFS_DFL_BLOCKSIZE_LOG,
 		.sb_feat = {
@@ -3737,7 +3737,8 @@  main(
 	 * implemented, emit a message to indicate where the defaults being
 	 * used came from.
 	 *
-	 * printf(_("Default configuration sourced from %s\n"), dft.source);
+	 * printf(_("Default configuration sourced from %s\n"),
+	 *	  default_type_str(dft.type));
 	 */
 
 	/* copy new defaults into CLI parsing structure */
diff --git a/mkfs/xfs_mkfs_common.h b/mkfs/xfs_mkfs_common.h
index 9b0f67b70cf1..d867ab377185 100644
--- a/mkfs/xfs_mkfs_common.h
+++ b/mkfs/xfs_mkfs_common.h
@@ -40,6 +40,17 @@  struct sb_feat_args {
 	bool	nortalign;
 };
 
+/*
+ * File configuration type settings
+ *
+ * These are the different possibilities by which you can end up parsing
+ * default settings with. DEFAULTS_BUILTIN indicates there was no configuration
+ * file parsed and we are using the built-in defaults on this code.
+ */
+enum default_params_type {
+	DEFAULTS_BUILTIN = 0,
+};
+
 /*
  * Default filesystem features and configuration values
  *
@@ -49,7 +60,7 @@  struct sb_feat_args {
  * calculations.
  */
 struct mkfs_default_params {
-	char	*source;	/* where the defaults came from */
+	enum default_params_type type; /* where the defaults came from */
 
 	int	sectorsize;
 	int	blocksize;