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

[Dave Chinner <david@fromorbit.com>]

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.

-- 
Dave Chinner
david@fromorbit.com