| Message ID | 20170423185503.31415-6-jtulak@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: > Add a new field int opt_params - value, Agreed. > which is filled with user input. It sounds to me its more than that, its the default value which will be used should not user input for the option be specified, and if user input value is provided it will be the passed user input value. If the final value for the respective option. ? > Signed-off-by: Jan Tulak <jtulak@redhat.com> > --- > mkfs/xfs_mkfs.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 513e106..c2ffd91 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -128,6 +128,12 @@ uint64_t sectorsize; > * Filled raw string from the user, so we never lose that information e.g. > * to print it back in case of an issue. > * > + * value OPTIONAL > + * The value that is to be used if the given subopt is not specified at all. It seems to be more than that ? > + * This is filled with user input This is a bit confusing, isn't it first the default value should no user input value be passed ? And finally if user input value is passed only then will this be that value ? > and anything you write here now is > + * overwritten if user specifies the subopt. (If the user input is a string > + * and not a number, this value is set to a positive non-zero number.) > + * > * !!! NOTE ================================================================== > * > * If you are adding a new option, or changing an existing one, > @@ -152,6 +158,7 @@ struct opt_params { > uint64_t maxval; > uint64_t flagval; > const char *raw_input; > + uint64_t value; > } subopt_params[MAX_SUBOPTS]; > } opts[MAX_OPTS] = { > #define OPT_B 0 It would seem rather unfair to define this this but not use it in the patch ? 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 Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: >> Add a new field int opt_params - value, > > Agreed. > >> which is filled with user input. > > It sounds to me its more than that, its the default value which will be used should > not user input for the option be specified, and if user input value is provided it > will be the passed user input value. If the final value for the respective option. > > ? Yes. I see I should make the description better, both here in commit message and in the code too. It is the same logic as it was for the old variables like agcount in main(). Initialize it with whatever you want as the default for pure "mkfs.xfs /some/dev", but if the users give us the specific option, we are going to overwrite it with their value. I don't see any use for remembering the defaults once it is clear that user wants to use custom values instead. And copying the defaults from some other field "if the user said nothing" sounds like a recipe for bugs... > >> Signed-off-by: Jan Tulak <jtulak@redhat.com> >> --- >> mkfs/xfs_mkfs.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c >> index 513e106..c2ffd91 100644 >> --- a/mkfs/xfs_mkfs.c >> +++ b/mkfs/xfs_mkfs.c >> @@ -128,6 +128,12 @@ uint64_t sectorsize; >> * Filled raw string from the user, so we never lose that information e.g. >> * to print it back in case of an issue. >> * >> + * value OPTIONAL >> + * The value that is to be used if the given subopt is not specified at all. > > It seems to be more than that ? > >> + * This is filled with user input > > This is a bit confusing, isn't it first the default value should no user input > value be passed ? And finally if user input value is passed only then will this > be that value ? > >> and anything you write here now is >> + * overwritten if user specifies the subopt. (If the user input is a string >> + * and not a number, this value is set to a positive non-zero number.) >> + * >> * !!! NOTE ================================================================== >> * >> * If you are adding a new option, or changing an existing one, >> @@ -152,6 +158,7 @@ struct opt_params { >> uint64_t maxval; >> uint64_t flagval; >> const char *raw_input; >> + uint64_t value; >> } subopt_params[MAX_SUBOPTS]; >> } opts[MAX_OPTS] = { >> #define OPT_B 0 > > It would seem rather unfair to define this this but not use it in the patch ? I'm still trying to find out when exactly should I make something two patches and when one, and it seems to shift case by case... I tried to separate the adding of new things and rewriting of the existing code, but do you think, in this case, the new thing is too trivial to have a standalone patch? Jan > > Luis
v On Tue, Apr 25, 2017 at 10:04 AM, Jan Tulak <jtulak@redhat.com> wrote: > On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >> On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: >>> Add a new field int opt_params - value, >> >> Agreed. >> >>> which is filled with user input. >> >> It sounds to me its more than that, its the default value which will be used should >> not user input for the option be specified, and if user input value is provided it >> will be the passed user input value. If the final value for the respective option. >> >> ? > > Yes. I see I should make the description better, both here in commit > message and in the code too. It is the same logic as it was for the > old variables like agcount in main(). Initialize it with whatever you > want as the default for pure "mkfs.xfs /some/dev", but if the users > give us the specific option, we are going to overwrite it with their > value. > > I don't see any use for remembering the defaults once it is clear that > user wants to use custom values instead. And copying the defaults from > some other field "if the user said nothing" sounds like a recipe for > bugs... > And rather than submitting repeatedly new versions of the whole patch... how about this? * * value OPTIONAL * The actual value used in computations and for creating the filesystem. * It is filled with user input and anything you write here now is * overwritten if the user specifies the subopt. But he does not, then whatever * you put there is used as the default. Can be omitted if the default * is 0. * (If the field is a string and not a number, this value is set to * a positive non-zero number on an user input.) And for the commit message, a bit shortened version: Add a new field int opt_params - value - which is the actual value used in computations and for creating the filesystem. It is filled with user input if the user specifies the subopt. But he does not, then whatever you put there is used as the default. Sounds better? >> >>> Signed-off-by: Jan Tulak <jtulak@redhat.com> >>> --- >>> mkfs/xfs_mkfs.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c >>> index 513e106..c2ffd91 100644 >>> --- a/mkfs/xfs_mkfs.c >>> +++ b/mkfs/xfs_mkfs.c >>> @@ -128,6 +128,12 @@ uint64_t sectorsize; >>> * Filled raw string from the user, so we never lose that information e.g. >>> * to print it back in case of an issue. >>> * >>> + * value OPTIONAL >>> + * The value that is to be used if the given subopt is not specified at all. >> >> It seems to be more than that ? >> >>> + * This is filled with user input >> >> This is a bit confusing, isn't it first the default value should no user input >> value be passed ? And finally if user input value is passed only then will this >> be that value ? >> >>> and anything you write here now is >>> + * overwritten if user specifies the subopt. (If the user input is a string >>> + * and not a number, this value is set to a positive non-zero number.) >>> + * >>> * !!! NOTE ================================================================== >>> * >>> * If you are adding a new option, or changing an existing one, >>> @@ -152,6 +158,7 @@ struct opt_params { >>> uint64_t maxval; >>> uint64_t flagval; >>> const char *raw_input; >>> + uint64_t value; >>> } subopt_params[MAX_SUBOPTS]; >>> } opts[MAX_OPTS] = { >>> #define OPT_B 0 >> >> It would seem rather unfair to define this this but not use it in the patch ? > > I'm still trying to find out when exactly should I make something two > patches and when one, and it seems to shift case by case... I tried to > separate the adding of new things and rewriting of the existing code, > but do you think, in this case, the new thing is too trivial to have a > standalone patch? > > Jan > >> >> Luis > > > > -- > Jan Tulak > jtulak@redhat.com / jan@tulak.me
On Tue, Apr 25, 2017 at 11:39:00AM +0200, Jan Tulak wrote: > And rather than submitting repeatedly new versions of the whole > patch... how about this? > > * > * value OPTIONAL > * The actual value used in computations and for creating the filesystem. > * It is filled with user input and anything you write here now is I think its easier to read if you are clear from the start user input is optional, and so a default value is used first, and use input overrides these initial system defaults. > * overwritten if the user specifies the subopt. But he does not, > then whatever "But he does not", I guess you meant, "But if he does not"... > * you put there is used as the default. Can be omitted if the default > * is 0. Hm, how about: The actual value used in computations for creating the filesystem. This is initialized with sensible default values, if initialized to 0 the value is considered disabled. User input can optionally override default values. If the field is a string and not a number, the value is set to a positive non-zero number when user input has been supplied. > * (If the field is a string and not a number, this value is set to > * a positive non-zero number on an user input.) > > And for the commit message, a bit shortened version: > > Add a new field int opt_params - value - which is the actual value > used in computations and for creating the filesystem. It is filled > with user input if the user specifies the subopt. But he does not, > then whatever you put there is used as the default. Modulo "But he does not"/ "but if no user input is not passed then the originally set defaults will be used" 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 Tue, Apr 25, 2017 at 10:04:39AM +0200, Jan Tulak wrote: > On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > > On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: > > > >> and anything you write here now is > >> + * overwritten if user specifies the subopt. (If the user input is a string > >> + * and not a number, this value is set to a positive non-zero number.) > >> + * > >> * !!! NOTE ================================================================== > >> * > >> * If you are adding a new option, or changing an existing one, > >> @@ -152,6 +158,7 @@ struct opt_params { > >> uint64_t maxval; > >> uint64_t flagval; > >> const char *raw_input; > >> + uint64_t value; > >> } subopt_params[MAX_SUBOPTS]; > >> } opts[MAX_OPTS] = { > >> #define OPT_B 0 > > > > It would seem rather unfair to define this this but not use it in the patch ? > > I'm still trying to find out when exactly should I make something two > patches and when one, and it seems to shift case by case... I tried to > separate the adding of new things and rewriting of the existing code, > but do you think, in this case, the new thing is too trivial to have a > standalone patch? Ah, I see, this was split out to be separate given a slew of subopts in turn later use it. The nasty alternative for review would be to have all subopts converted in one shot. I suppose a middle ground would be to have this patch squashed with just one of the subopt users, and then each other subopt ported atomically. That would make the addition of the value and definition in effect as soon as its added. Just my 0.02 CRC but its up to Eric as this is rather subjective and tree dependent. 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 4/25/17 8:10 PM, Luis R. Rodriguez wrote: > On Tue, Apr 25, 2017 at 10:04:39AM +0200, Jan Tulak wrote: >> On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >>> On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: >>> >>>> and anything you write here now is >>>> + * overwritten if user specifies the subopt. (If the user input is a string >>>> + * and not a number, this value is set to a positive non-zero number.) >>>> + * >>>> * !!! NOTE ================================================================== >>>> * >>>> * If you are adding a new option, or changing an existing one, >>>> @@ -152,6 +158,7 @@ struct opt_params { >>>> uint64_t maxval; >>>> uint64_t flagval; >>>> const char *raw_input; >>>> + uint64_t value; >>>> } subopt_params[MAX_SUBOPTS]; >>>> } opts[MAX_OPTS] = { >>>> #define OPT_B 0 >>> >>> It would seem rather unfair to define this this but not use it in the patch ? >> >> I'm still trying to find out when exactly should I make something two >> patches and when one, and it seems to shift case by case... I tried to >> separate the adding of new things and rewriting of the existing code, >> but do you think, in this case, the new thing is too trivial to have a >> standalone patch? > > Ah, I see, this was split out to be separate given a slew of subopts in turn > later use it. The nasty alternative for review would be to have all subopts > converted in one shot. I suppose a middle ground would be to have this patch > squashed with just one of the subopt users, and then each other subopt ported > atomically. That would make the addition of the value and definition in effect > as soon as its added. > > Just my 0.02 CRC but its up to Eric as this is rather subjective and tree dependent. I think the way Jan did this is fine. Ideally the commit would explain a bit more, i.e. "This patch simply adds the value field. Subsequent patches will utilize this new field." Then the reviewers know what to expect from the start, and we avoid the back and forth of figuring out why it was done as it was done. ;) However, I think 5 & 6 could easily be combined - add the field and the routines to manipulate it, /then/ start adding users of that new infrastructure. I haven't really reviewed from here on out yet, but from a "how should I break up patches" point of view, that's my ... $0.02. :) -Eric > > 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 > -- 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 Wed, Apr 26, 2017 at 3:04 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > On Tue, Apr 25, 2017 at 11:39:00AM +0200, Jan Tulak wrote: > Hm, how about: > > The actual value used in computations for creating the filesystem. > This is initialized with sensible default values, if initialized to 0 > the value is considered disabled. User input can optionally override > default values. If the field is a string and not a number, the value > is set to a positive non-zero number when user input has been supplied. > >> * (If the field is a string and not a number, this value is set to >> * a positive non-zero number on an user input.) >> >> And for the commit message, a bit shortened version: >> >> Add a new field int opt_params - value - which is the actual value >> used in computations and for creating the filesystem. It is filled >> with user input if the user specifies the subopt. But he does not, >> then whatever you put there is used as the default. > > Modulo "But he does not"/ "but if no user input is not passed then > the originally set defaults will be used" > > Luis Sounds good. I will use your version. Thanks, Jan
On Wed, Apr 26, 2017 at 4:50 AM, Eric Sandeen <sandeen@sandeen.net> wrote: > On 4/25/17 8:10 PM, Luis R. Rodriguez wrote: >> On Tue, Apr 25, 2017 at 10:04:39AM +0200, Jan Tulak wrote: >>> On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: >>>> On Sun, Apr 23, 2017 at 08:54:56PM +0200, Jan Tulak wrote: >>>> >>>>> and anything you write here now is >>>>> + * overwritten if user specifies the subopt. (If the user input is a string >>>>> + * and not a number, this value is set to a positive non-zero number.) >>>>> + * >>>>> * !!! NOTE ================================================================== >>>>> * >>>>> * If you are adding a new option, or changing an existing one, >>>>> @@ -152,6 +158,7 @@ struct opt_params { >>>>> uint64_t maxval; >>>>> uint64_t flagval; >>>>> const char *raw_input; >>>>> + uint64_t value; >>>>> } subopt_params[MAX_SUBOPTS]; >>>>> } opts[MAX_OPTS] = { >>>>> #define OPT_B 0 >>>> >>>> It would seem rather unfair to define this this but not use it in the patch ? >>> >>> I'm still trying to find out when exactly should I make something two >>> patches and when one, and it seems to shift case by case... I tried to >>> separate the adding of new things and rewriting of the existing code, >>> but do you think, in this case, the new thing is too trivial to have a >>> standalone patch? >> >> Ah, I see, this was split out to be separate given a slew of subopts in turn >> later use it. The nasty alternative for review would be to have all subopts >> converted in one shot. I suppose a middle ground would be to have this patch >> squashed with just one of the subopt users, and then each other subopt ported >> atomically. That would make the addition of the value and definition in effect >> as soon as its added. >> >> Just my 0.02 CRC but its up to Eric as this is rather subjective and tree dependent. > > I think the way Jan did this is fine. Ideally the commit would explain a bit more, > i.e. "This patch simply adds the value field. Subsequent patches will utilize > this new field." > > Then the reviewers know what to expect from the start, and we avoid the back and > forth of figuring out why it was done as it was done. ;) > > However, I think 5 & 6 could easily be combined - add the field and the routines > to manipulate it, /then/ start adding users of that new infrastructure. > > I haven't really reviewed from here on out yet, but from a "how should I break > up patches" point of view, that's my ... $0.02. :) > Yeah, that sounds reasonable, I'm squashing it for the next version (and adding the notice). Jan
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 513e106..c2ffd91 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -128,6 +128,12 @@ uint64_t sectorsize; * Filled raw string from the user, so we never lose that information e.g. * to print it back in case of an issue. * + * value OPTIONAL + * The value that is to be used if the given subopt is not specified at all. + * This is filled with user input and anything you write here now is + * overwritten if user specifies the subopt. (If the user input is a string + * and not a number, this value is set to a positive non-zero number.) + * * !!! NOTE ================================================================== * * If you are adding a new option, or changing an existing one, @@ -152,6 +158,7 @@ struct opt_params { uint64_t maxval; uint64_t flagval; const char *raw_input; + uint64_t value; } subopt_params[MAX_SUBOPTS]; } opts[MAX_OPTS] = { #define OPT_B 0
Add a new field int opt_params - value, which is filled with user input. Signed-off-by: Jan Tulak <jtulak@redhat.com> --- mkfs/xfs_mkfs.c | 7 +++++++ 1 file changed, 7 insertions(+)