Re: [PATCH v2 09/19] test-cutils: Add coverage of qemu_strtod

[Eric Blake]

On Fri, May 19, 2023 at 05:05:20PM +0200, Hanna Czenczek wrote:
> On 12.05.23 04:10, Eric Blake wrote:
> > It's hard to tweak code for consistency if I can't prove what will or
> > won't break from those tweaks.  Time to add unit tests for
> > qemu_strtod() and qemu_strtod_finite().
> > 
> > Among other things, I wrote a check whether we have C99 semantics for
> > strtod("0x1") (which MUST parse hex numbers) rather than C89 (which
> > must stop parsing at 'x').  These days, I suspect that is okay; but if
> > it fails CI checks, knowing the difference will help us decide what we
> > want to do about it.  Note that C2x, while not final at the time of
> > this patch, has been considering whether to make strtol("0b1") parse
> > as 1 with no slop instead of the C17 parse of 0 with slop "b1"; that
> > decision may also bleed over to strtod().  But for now, I didn't think
> > it worth adding unit tests on that front (to strtol or strtod) as
> > things may still change.
> > 
> > Likewise, there are plenty more corner cases of strtod proper that I
> > don't explicitly test here, but there are enough unit tests added here
> > that it covers all the branches reached in our wrappers.  In
> > particular, it demonstrates the difference on when *value is left
> > uninitialized, which an upcoming patch will normalize.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > ---
> > 
> > v2: Added g_assert_false(signbit(res)) anywhere I used
> > g_assert_cmpfloat(res,==,0.0); add a test for strtod() hex parsing and
> > handling of junk after ERANGE, which is major enough that I dropped
> > R-b
> > ---
> >   tests/unit/test-cutils.c | 510 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 510 insertions(+)
> > 
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index d3076c3fec1..1763839a157 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
> 
> [...]
> 
> > +static void test_qemu_strtod_erange_junk(void)
> > +{
> > +    const char *str;
> > +    const char *endptr;
> > +    int err;
> > +    double res;
> > +
> > +    /* EINVAL has priority over ERANGE */
> 
> By being placed here, this comment confused me a bit, because the first case
> does return ERANGE.  So I’d prefer it above the second case, where we
> actually expect EINVAL, but understand that’s a personal preference.  (Same
> for the _finite_ variant)

The test is what happens when both conditions apply.  For
qemu_strtod("1e-999junk", &endptr), only ERANGE applies (because
"junk" is returned in endptr); it is not until
qemu_strtod("1e-999junk", NULL) where EINVAL is also possible
(trailing junk takes precedence over underflow).  For qemu_strtosz(),
I made it a bit more obvious by writing a helper function that shows
both errno values in a single line, rather than spreading out the
boilerplate over multiple lines.

Should I do a similar helper function for qemu_strtod[_finite] in v3?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org