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

On Tue, May 16, 2023 at 07:20:16PM +0200, Laszlo Ersek wrote: I'll start with the good, before focusing on the questionable. You make a compelling argument about wanting to reuse already-existing code for parsing a percentage. It is indeed useful to have a single utility function, to be reused by multiple plugins and filters, so that we have consistent grammars for what we are willing to accept (rather than multiple ad hoc parsers being subtly different), and where we can fix any bugs or add new supported parses in just one location. So that is a strong argument in favor of this patch if we can justify why having a floating point error representation in the first place is worthwhile. The latter, of course, is harder to argue; so just because this patch is good at eliminating redundancy does not necessarily mean it is a good idea to use floating point. Would it be helpful if the utility function included bounds that the result must lie between? As in: if (nbdkit_parse_probability ("my function", str, 0.0, 1.0, &value) == -1) return -1; where code that wants to allow larger than 100% can do so by passing different bounds? This accepts LOTS of strings. Maybe that is intentional, but still, it accepts more than you documented. For example, it accepts "-inf:nan(0x0)", but only on some platforms (since expanded nan(...) sequences are implementation defined), even though I can't possibly see how that makes for a valid percentage. So your intent is to allow bypassing 0/0 to instead behave identically to 0%. No offense taken. And while you are correct that my v2 series for qemu clocked in at 19 patches, it doesn't necessarily translate to 19 bugs fixed (although it was definitely more than one), nor were they all in floating point (one of the bugs was in qemu_strtoui() which is integer only). In short, my biggest time sink there was fixing technical debt on needing a LOT more unit tests to make sure I wasn't inadvertently breaking some corner case (and with strtod() or scanf("%g"), there are a LOT of those). Still, being able to write 1.5G rather than 1610612736 explains why qemu goes to the bother of attempting to allow certain floating point strings. So inspired by your concern, I just took another look at nbdkit_parse_size(), which currently has rather terse public documentation of: | Use the C<nbdkit_parse_size> utility function to parse human-readable | size strings such as "100M" into the size in bytes. | | int64_t nbdkit_parse_size (const char *str); | | C<str> can be a string in a number of common formats. The function | returns the size in bytes. If there was an error, it returns C<-1>. but where you are better reading server/public.c to see what we actually parse, along with server/test-public.c:test_nbdkit_parse_size() for what we promise not to break (currently, no strings with '.' are in our explicitly supported set). The former has: /* XXX Should we also parse things like '1.5M'? */ /* XXX Should we allow hex? If so, hex cannot use scaling suffixes, * because some of them are valid hex digits */ which are some of the very corner cases I was just dealing with in qemu's qemu_strtosz(); and the latter has its own shortfalls (for example, classifying "8E" as a bogus string, instead of in the section of strings leading to overflow - the real reason we reject 8E is because it doesn't fit as a positive value in off_t). Alas, since qemu has a stricter license, I can't just copy my work there over to nbdkit without also exercising my right as author to dual-license it more permissively. But the way our docs are currently written, we do seem to reserve the right to make strings that currently fail to parse start validly parsing in the future (such as 1.5M). C is a lot looser here than other languages. For example, Java took pains to define that division by 0 or infinity produces platform-independent reliable results (at the expense of someone writing a Java virtual machine having to do slower implementations which special case anything that the hardware does not natively do in the same manner). At any rate, I agree with you that floating point division, when you have not validated that the unbounded-precision result won't overflow/underflow under rounding scenarios, is not trivial to think about. Oh, so my assumption (in my reply to patch 5, before replying to this email) that (uint64_t)INFINITY produces UINT64_MAX is invalid (again, maybe something I'm remembering from Java taking pains to explicitly specify that). It may happen to do so, but other values are also possible, and the compiler can also use this explicit promise of UB to optimize things differently than we would like. Yeah, this really is a bear-trap. I challenge you to find a system where 'double' can hold the value UINT64_MAX + 0.5 with sufficient precision (IEEE double only requires 51 bits of precision). In fact, some of the back-story of my work on qemu's floating point parser was an earlier bug report, raised by nbdkit's unit testing, when we discovered that qemu was rounding large values (because of its trip through 'double') rather than matching an integral parse (found when qemu was unable to produce large sparse images as large as nbdkit because of the lost precision). Semi-related, just last week, I also fixed a bug in glib where it was assuming that the round trip (uint64_t)(long double)(uint64_t)value would not lose precision, which happens to be the case on x86_64 but is not true on other hardware; but here we're using sscanf with 'double' not 'long double'. 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). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org