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

[Jan Tulak <jtulak@redhat.com>]

On Wed, Jul 26, 2017 at 10:49 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
> 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 "---"
>

Yes, I'm sorry about that. :( I forgot to remove the Reviewed-by after
rebasing it without the uint64 change. :(

Jan

> Thanks,
> -Eric
>
> --
> 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



-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me