[Libguestfs] [PATCH nbdkit 1/5] Add new public nbdkit_parse_probability function

[Laszlo Ersek]

On 5/16/23 22:22, Richard W.M. Jones wrote:
> On Tue, May 16, 2023 at 02:35:38PM -0500, Eric Blake wrote:

>> Our pre-existing error filter is already doing percentage calculations
>> via floating point, so on the grounds that this patch is consolidating
>> common code patterns to avoid reimplementation, we're good. But as you
>> say, representing things as a ratio between two integers may be more
>> maintainable than floating point where accuracy matters, or this may
>> be a case where we are explicit that use of the floating point
>> fractions as a percentage is intentionally liable to rounding issues.
>> Still, avoiding undefined behavior when the division can overflow
>> (whether that be the overflow to floating point infinity or even the
>> overflow beyond UINT64_MAX), does make it worth questioning if
>> floating point is our best representation, or at a minimum if we want
>> to be stricter in what we parse before passing a substring on to
>> strtod() so that we have more control over the values (part of my 19
>> patches to qemu was explicitly truncating any string at 'e' or 'E' so
>> that strtod() could not do an exponent parse that would inadverently
>> hit overflow to INFINITY).
> 
> I'm not really sure we reached a conclusion so far, but I did want to
> say that I changed the evil filter impl so that it now treats any
> 0 <= P < 1e-12 as effectively 0.  (P == evil-probability; P < 0 and P > 1
> are still rejected at config time as before).
> 
> With P == 1e-12:
> 
> nbdkit: debug: evil: mode: stuck-bits, P: 1e-12
> nbdkit: debug: evil: block_size: 17592186044416 (2**44)
> nbdkit: debug: evil: expected bits per block: 140.737
> 
> (The large block size isn't an issue here as nothing is allocated.
> However it would be a problem if the block_size calculation overflows,
> and I believe that limiting P to larger values avoids this.)

Right, this is about the direction I could agree with -- and again I
want to emphasize that you don't *need* my agreement to proceed with the
floating point approach!

I think enforcing a minimum for P could be useful too, for wherever we
divide by P.

> Also I'm not sure that handling probabilities as a numerator and
> denominator actually helps here?  Both still need to be stored as
> floats, so it just pushes the same problem to the plugin.

No, the point with N/D is that they're both positive integers (well, N
may be zero as well), and that they *together* give you a single
rational probability, namely N/D. They'd be stored as separate unsigned
integer fields everywhere, and the division would only be performed (in
unsigned integer) wherever directly needed, such as in (a * N) / D.

I don't know if this is compatible with existent code. If we already
have code that parses floats (and Eric's comments imply we do), then
that code couldn't be rebased on this N/D representation, compatibly.
But, I'd consider introducing a new (in a way, "divergent") N/D
representation a fair price, for not adding more floating point!
Refactoring (extracting common code) is only useful if we can fix the
extracted code later on, and that's a tough challenge with floating point.

Laszlo