Re: [PATCH 01/22] mkfs: remove intermediate getstr followed by getnum

[Eric Sandeen <sandeen@sandeen.net>]

On 4/5/17 8:00 AM, Jan Tulak wrote:
> On Thu, Mar 16, 2017 at 11:59 PM, Eric Sandeen <sandeen@sandeen.net> wrote:
>> The only downside I see to this is that we used to echo back the same
>> form that was specified, i.e. -
>>
>> # mkfs.xfs -f -d size=4e fsfile
>> size 4e specified for data subvolume is too large, maximum is 262144 blocks
>> # mkfs.xfs -f -d size=4611686018427387904 fsfile
>> size 4611686018427387904 specified for data subvolume is too large, maximum is 262144 blocks
>> # mkfs.xfs -f -d size=1125899906842624b fsfile
>> size 1125899906842624b specified for data subvolume is too large, maximum is 262144 blocks
>>
>> now we always get back the raw byte value:
>>
>> # mkfs/mkfs.xfs -f -d size=4e fsfile
>> size 4611686018427387904 specified for data subvolume is too large, maximum is 262144 blocks
>>
>> Anything that fails the getnum() checks echo back the original
>> form on the cmdline; this case is different because getnum() passes,
>> but by the time we do value checking all we have is the bytes.  Is
>> that intentional/desirable?
>>
>> So it's a change, not necessarily incorrect or unacceptable, but
>> I wanted to highlight it and check.  It wouldn't be too hard to
>> convert it right away, but keep the string around for error printing
>> purposes, I suppose.
>>
> 
> It goes back into the original reporting style later in the set. I
> tried to change the patch to avoid this temporary change, but the
> issue is, I loose the old raw strings before I have access to the
> necessary indexes for the option table to read the raw string from it,
> or before a code shuffling brings the reporting closer to where the
> raw string still exists.
> 
> Given that it is only a temporary change of style and it seems to be
> too deeply depending on other changes, I'm keeping it as a TODO, but
> not a blocker. Maybe if the patch can be split and one of the parts
> moved after other commits...

Even simply noting in the changelog that it's temporary, and the reason
for the change, would be helpful.  It's a big enough patchset that it's
hard for the reviewer to keep everything in mind across 20+ patches.  ;)

Thanks,
-Eric