qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Czenczek
Subject: Re: [PATCH v2 15/19] cutils: Set value in all qemu_strtosz* error paths
Date: Fri, 19 May 2023 17:29:55 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0

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>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]