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

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> This will permit its use in parse_option_size().
>> 
>> Cc: Dr. David Alan Gilbert 
>> Cc: Eduardo Habkost  (maintainer:X86)
>> Cc: Kevin Wolf  (supporter:Block layer core)
>> Cc: Max Reitz  (supporter:Block layer core)
>> Cc: qemu-block@nongnu.org (open list:Block layer core)
>> Signed-off-by: Markus Armbruster 
>> ---
>>  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, );
>> -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(, "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 >= 0xfc00 overflow uint64_t after their trip
>> + * through double (53 bits of precision).
>> + */
>> +if ((val * mul >= 0xfc00) || 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 

Thanks!

[...]

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

[Markus Armbruster]

Eric Blake  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 
>> Cc: Eduardo Habkost  (maintainer:X86)
>> Cc: Kevin Wolf  (supporter:Block layer core)
>> Cc: Max Reitz  (supporter:Block layer core)
>> Cc: qemu-block@nongnu.org (open list:Block layer core)
>> Signed-off-by: Markus Armbruster 
>> ---
>
>> +++ 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, );
>> -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, );
>> +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 >= 0xfc00 overflow uint64_t after their trip
>> + * through double (53 bits of precision).
>> + */
>> +if ((val * mul >= 0xfc00) || 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 

Thanks!

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

[Eric Blake]

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 
> Cc: Eduardo Habkost  (maintainer:X86)
> Cc: Kevin Wolf  (supporter:Block layer core)
> Cc: Max Reitz  (supporter:Block layer core)
> Cc: qemu-block@nongnu.org (open list:Block layer core)
> Signed-off-by: Markus Armbruster 
> ---

> +++ 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, );
> -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.

> +++ 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, );
> +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 >= 0xfc00 overflow uint64_t after their trip
> + * through double (53 bits of precision).
> + */
> +if ((val * mul >= 0xfc00) || 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.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature