On Tue, Apr 25, 2017 at 5:13 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx / jan@xxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
