Re: [PATCH v2 3/5] mkfs: replace defaults source with an enum

["Luis R. Rodriguez" <mcgrof@kernel.org>]

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