Re: [PATCH 3/7] mkfs: remove intermediate getstr followed by getnum

Eric Sandeen
2017-07-26T20:49:05Z

On 7/20/17 10:54 AM, Darrick J. Wong wrote:
> On Thu, Jul 20, 2017 at 11:29:28AM +0200, Jan Tulak wrote:
>> Some options loaded a number as a string with getstr and converted it to
>> number with getnum later in the code, without any reason for this
>> approach. (They were probably forgotten in some past cleaning.)
>>
>> This patch changes them to skip the string and use getnum directly in
>> the main option-parsing loop, as do all the other numerical options.
>> And as we now have two variables of the same type for the same value,
>> merge them together. (e.g. former string dsize and number dbytes).
>>
>> Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>> Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
>>
>> ---
>> In reply to Eric's comment, so it doesn't pop up again:
>> This patch has to be applied after "mkfs: Save raw user input ...",
>> because otherwise we would temporary lose the access to strings
>> with user-given values and thus wouldn't be able to report them in
>> case of any issue.
>> ---
>> mkfs/xfs_mkfs.c | 90 ++++++++++++++++++++++++++-------------------------------
>> 1 file changed, 41 insertions(+), 49 deletions(-)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index f2f6468e..127f14c3 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -1345,6 +1345,7 @@ getnum(
>> long long c;
>>
>> check_opt(opts, index, false);
>> + set_conf_raw(opts, index, str);
>> /* empty strings might just return a default value */
>> if (!str || *str == '\0') {
>> if (sp->flagval == SUBOPT_NEEDS_VAL)
>> @@ -1432,12 +1433,12 @@ main(
>> char *dfile;
>> int dirblocklog;
>> int dirblocksize;
>> - char *dsize;
>> + int dbytes;

<snip>

>> @@ -2179,10 +2180,7 @@ _("rmapbt not supported with realtime devices\n"));
>> }
>>
>>
>> - if (dsize) {
>> - __uint64_t dbytes;
>> -
>> - dbytes = getnum(dsize, &dopts, D_SIZE);
>> + if (dbytes) {
>
> Why has dbytes been demoted from uint64_t to int? This eliminates the
> ability to -d size=8G, right?

That wasn't a problem in the version I reviewed.... pls don't keep reviewed-by's
on patches that change from the reviewed version. Further, best to indicate
explicitly what has changed since the last version, under the "---"

Thanks,
-Eric