Re: [PATCH] lib/string_helpers: fix string_get_size() unit promotion after rounding

[Shuvam Pandey]

Hi Andy,

Sorry, I should have kept the discussion on-list.

This is about the string_get_size() patch for outputs like "1000 kB" and
"1024 KiB", i.e. cases where the current code has already rounded to the
unit divisor but still prints the old unit.

> I'm trying to understand the current behaviour better and the logic of your
> change. But looking at the above I think your patch doesn't improve the case.
> And might even be another bug in the current code. I mean, do we have to round
> the numbers? 1048575 and lower are not equal 1.00 MiB strictly speaking.
> It's rather 0.999 MiB.

I took another look at the code and the old versions. I think the issue
here is narrower than whether values should cross the boundary at all.

For the base-10 values you asked about earlier, and for the binary
boundary case, the current code already rounds across the boundary.
With blk_size = 1, it prints:

  999999   -> 1000 kB
  1000000  -> 1.00 MB
  1000001  -> 1.00 MB

and

  1048575  -> 1024 KiB
  1048576  -> 1.00 MiB
  1048577  -> 1.00 MiB

So the patch does not introduce new cross-boundary rounding. It only changes
how those already-rounded results are displayed: it renormalizes 1000 kB to
1.00 MB and 1024 KiB to 1.00 MiB.

Around the first MB/MiB boundary with blk_size = 1, the display changes for:

  STRING_UNITS_10: 999500 .. 999999
  STRING_UNITS_2:  1048064 .. 1048575

with the adjacent values staying unchanged, e.g.:

  999499   -> 999 kB
  1000000  -> 1.00 MB
  1048063  -> 1023 KiB
  1048576  -> 1.00 MiB

That behavior comes from the arithmetic-rounding logic added by
564b026fbd0d ("string_helpers: fix precision loss for some inputs").
Before that change, 999999 printed 999 kB and 1048575 printed 1023 KiB,
so my original Fixes tag was wrong.

The v2 will carry:

  Fixes: 564b026fbd0d ("string_helpers: fix precision loss for some inputs")

If the intended policy is instead that values below 1.00 MB / MiB must
never round across the unit boundary, then I agree this is broader than a
renormalization bug and the rounding policy itself would need changing.

But with the current documented 3 significant figures behavior, leaving
the result as 1000 kB / 1024 KiB is inconsistent with that formatting,
because the existing rounding has already carried the value to the next
unit boundary.

Unless there is a reason to take a different approach, I'll send a v2 with
the corrected Fixes tag, an updated changelog framing this as post-rounding
renormalization, and threshold tests around the affected ranges.

Thanks,
Shuvam