Re: [PATCH v2 15/19] cutils: Set value in all qemu_strtosz* error paths

[Hanna Czenczek <hreitz@redhat.com>]

On 12.05.23 04:10, Eric Blake wrote:
> Making callers determine whether or not *value was populated on error
> is not nice for usability.  Pre-patch, we have unit tests that check
> that *result is left unchanged on most EINVAL errors and set to 0 on
> many ERANGE errors.  This is subtly different from libc strtoumax()
> behavior which returns UINT64_MAX on ERANGE errors, as well as
> different from our parse_uint() which slams to 0 on EINVAL on the
> grounds that we want our functions to be harder to mis-use than
> strtoumax().
>
> Let's audit callers:
>
> - hw/core/numa.c:parse_numa() fixed in the previous patch to check for
>    errors
>
> - migration/migration-hmp-cmds.c:hmp_migrate_set_parameter(),
>    monitor/hmp.c:monitor_parse_arguments(),
>    qapi/opts-visitor.c:opts_type_size(),
>    qapi/qobject-input-visitor.c:qobject_input_type_size_keyval(),
>    qemu-img.c:cvtnum_full(), qemu-io-cmds.c:cvtnum(),
>    target/i386/cpu.c:x86_cpu_parse_featurestr(), and
>    util/qemu-option.c:parse_option_size() appear to reject all failures
>    (although some with distinct messages for ERANGE as opposed to
>    EINVAL), so it doesn't matter what is in the value parameter on
>    error.
>
> - All remaining callers are in the testsuite, where we can tweak our
>    expectations to match our new desired behavior.
>
> Advancing to the end of the string parsed on overflow (ERANGE), while
> still returning 0, makes sense (UINT64_MAX as a size is unlikely to be
> useful); likewise, our size parsing code is complex enough that it's
> easier to always return 0 when endptr is NULL but trailing garbage was
> found, rather than trying to return the value of the prefix actually
> parsed (no current caller cared about the value of the prefix).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
>
> v2: cutils.c unchanged, but rebasing test suite is significant enough
> that I doropped Hanna's R-b
> ---
>   tests/unit/test-cutils.c | 106 +++++++++++++++++++--------------------
>   util/cutils.c            |  17 +++++--
>   2 files changed, 63 insertions(+), 60 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>