| Message ID | 20180517192700.23457-4-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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.
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
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.
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;
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(-)