Re: [Qemu-devel] [PATCH 23/24] util/cutils: Change qemu_strtosz*() from int64_t to uint64_t

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> This will permit its use in parse_option_size().
>> 
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Cc: Eduardo Habkost <address@hidden> (maintainer:X86)
>> Cc: Kevin Wolf <address@hidden> (supporter:Block layer core)
>> Cc: Max Reitz <address@hidden> (supporter:Block layer core)
>> Cc: address@hidden (open list:Block layer core)
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hmp.c                 |  5 +++--
>>  hw/misc/ivshmem.c     |  2 +-
>>  include/qemu/cutils.h |  6 +++---
>>  monitor.c             |  4 ++--
>>  qapi/opts-visitor.c   |  6 ++----
>>  qemu-img.c            |  5 ++++-
>>  qemu-io-cmds.c        |  5 ++++-
>>  target/i386/cpu.c     |  4 ++--
>>  tests/test-cutils.c   | 40 ++++++++++++++++++++--------------------
>>  util/cutils.c         | 14 +++++++++-----
>>  10 files changed, 50 insertions(+), 41 deletions(-)
>> 
>> diff --git a/hmp.c b/hmp.c
>> index 9846fa4..5b9e461 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1338,7 +1338,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>> QDict *qdict)
>>  {
>>      const char *param = qdict_get_str(qdict, "parameter");
>>      const char *valuestr = qdict_get_str(qdict, "value");
>> -    int64_t valuebw = 0;
>> +    uint64_t valuebw = 0;
>>      long valueint = 0;
>>      Error *err = NULL;
>>      bool use_int_value = false;
>> @@ -1379,7 +1379,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>> QDict *qdict)
>>              case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>>                  p.has_max_bandwidth = true;
>>                  ret = qemu_strtosz_mebi(valuestr, NULL, &valuebw);
>> -                if (ret < 0 || (size_t)valuebw != valuebw) {
>> +                if (ret < 0 || valuebw > INT64_MAX
>> +                    || (size_t)valuebw != valuebw) {
>
> We should probably just turn all of the parameters into size_t's - although 
> that's
> more work and there's some int64_t's in qemu_file for no good reason.

I guess that's a comment on migration parameters, not on the strosz
family of functions.

>>                      error_setg(&err, "Invalid size %s", valuestr);
>>                      goto cleanup;
>>                  }
[...]
>> diff --git a/util/cutils.c b/util/cutils.c
>> index 08effe6..888c0fd 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -207,7 +207,7 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>   */
>>  static int do_strtosz(const char *nptr, char **end,
>>                        const char default_suffix, int64_t unit,
>> -                      int64_t *result)
>> +                      uint64_t *result)
>>  {
>>      int retval;
>>      char *endptr;
>> @@ -237,7 +237,11 @@ static int do_strtosz(const char *nptr, char **end,
>>          retval = -EINVAL;
>>          goto out;
>>      }
>> -    if ((val * mul >= INT64_MAX) || val < 0) {
>> +    /*
>> +     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
>> +     * through double (53 bits of precision).
>> +     */
>> +    if ((val * mul >= 0xfffffffffffffc00) || val < 0) {
>
> It would be great to get rid of the double's at some point.

Maybe.

53 bits of precision are fine in practice, if a bit awkward to document
and test once you bother to document and test (read: only now, 6 years
four months after its creation).  long double would give us 64 bits at
least on common hosts, but would be even more bothersome to document and
test.

Could try to parse both as integer and as floating-point and see which
one accepts more characters.

> Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks!

[...]