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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/14/2017 04:26 AM, Markus Armbruster wrote:
>> This will permit its use in parse_option_size().
>
> Progress!  (not that it matters - off_t is a signed value, so you are
> STILL limited to 2^63, not 2^64, as the maximum theoretical size storage
> that you can target - and it's not like we have that much storage even
> accessible)
>
>> 
>> 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>
>> ---
>
>> +++ 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) {
>
> I don't know if this looks any better as:
>
> if (ret < 0 || valuebw > MIN(INT64_MAX, SIZE_MAX))
>
> so don't bother changing it if you think that's ugly.

I think (size_t)valuebw != valuebw neatly expresses "valuebw not
representable as size_t".  Requires unsigned, though.

>> +++ b/qapi/opts-visitor.c
>> @@ -481,7 +481,6 @@ opts_type_size(Visitor *v, const char *name, uint64_t 
>> *obj, Error **errp)
>>  {
>>      OptsVisitor *ov = to_ov(v);
>>      const QemuOpt *opt;
>> -    int64_t val;
>>      int err;
>>  
>>      opt = lookup_scalar(ov, name, errp);
>> @@ -489,14 +488,13 @@ opts_type_size(Visitor *v, const char *name, uint64_t 
>> *obj, Error **errp)
>>          return;
>>      }
>>  
>> -    err = qemu_strtosz(opt->str ? opt->str : "", NULL, &val);
>> +    err = qemu_strtosz(opt->str ? opt->str : "", NULL, obj);
>>      if (err < 0) {
>>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>> -                   "a size value representible as a non-negative int64");
>
> Nice - you're killing the unusual variant spelling of representable.
>
>> +++ 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) {
>
> I guess there's not any simpler expression using INT64_MAX and
> operations (including casts to double and int64_t) that would reliably
> produce the same constant that you just open-coded here.

I think the literal plus the comment explaining it is easier to read.

> Reviewed-by: Eric Blake <address@hidden>

Thanks!