Re: [Qemu-devel] [PATCH 22/24] util/cutils: Return qemu_strtosz*() error and value separately

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> This makes qemu_strtosz(), qemu_strtosz_mebi() and
> qemu_strtosz_metric() similar to qemu_strtoi64(), except negative
> values are rejected.
> 
> 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-bl...@nongnu.org (open list:Block layer core)
> Signed-off-by: Markus Armbruster 

Reviewed-by: Dr. David Alan Gilbert 

> ---
>  hmp.c |   6 +--
>  hw/misc/ivshmem.c |   7 ++-
>  include/qemu/cutils.h |   6 +--
>  monitor.c |   5 ++-
>  qapi/opts-visitor.c   |   5 ++-
>  qemu-img.c|  10 +++--
>  qemu-io-cmds.c|  10 +++--
>  target/i386/cpu.c |   5 ++-
>  tests/test-cutils.c   | 120 
> ++
>  util/cutils.c |  22 -
>  10 files changed, 119 insertions(+), 77 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 0eb5b6d..9846fa4 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1342,7 +1342,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> QDict *qdict)
>  long valueint = 0;
>  Error *err = NULL;
>  bool use_int_value = false;
> -int i;
> +int i, ret;
>  
>  for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
>  if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
> @@ -1378,8 +1378,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> QDict *qdict)
>  break;
>  case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>  p.has_max_bandwidth = true;
> -valuebw = qemu_strtosz_mebi(valuestr, NULL);
> -if (valuebw < 0 || (size_t)valuebw != valuebw) {
> +ret = qemu_strtosz_mebi(valuestr, NULL, );
> +if (ret < 0 || (size_t)valuebw != valuebw) {
>  error_setg(, "Invalid size %s", valuestr);
>  goto cleanup;
>  }
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index f00cd75..3dc04f4 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1267,8 +1267,11 @@ static void ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>  if (s->sizearg == NULL) {
>  s->legacy_size = 4 << 20; /* 4 MB default */
>  } else {
> -int64_t size = qemu_strtosz_mebi(s->sizearg, NULL);
> -if (size < 0 || (size_t)size != size || !is_power_of_2(size)) {
> +int ret;
> +int64_t size;
> +
> +ret = qemu_strtosz_mebi(s->sizearg, NULL, );
> +if (ret < 0 || (size_t)size != size || !is_power_of_2(size)) {
>  error_setg(errp, "Invalid size %s", s->sizearg);
>  return;
>  }
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index 4184851..c91649b 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -139,9 +139,9 @@ int parse_uint(const char *s, unsigned long long *value, 
> char **endptr,
> int base);
>  int parse_uint_full(const char *s, unsigned long long *value, int base);
>  
> -int64_t qemu_strtosz(const char *nptr, char **end);
> -int64_t qemu_strtosz_mebi(const char *nptr, char **end);
> -int64_t qemu_strtosz_metric(const char *nptr, char **end);
> +int qemu_strtosz(const char *nptr, char **end, int64_t *result);
> +int qemu_strtosz_mebi(const char *nptr, char **end, int64_t *result);
> +int qemu_strtosz_metric(const char *nptr, char **end, int64_t *result);
>  
>  #define K_BYTE (1ULL << 10)
>  #define M_BYTE (1ULL << 20)
> diff --git a/monitor.c b/monitor.c
> index 1f8c031..85b1b61 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2773,6 +2773,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>  break;
>  case 'o':
>  {
> +int ret;
>  int64_t val;
>  char *end;
>  
> @@ -2785,8 +2786,8 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>  break;
>  }
>  }
> -val = qemu_strtosz_mebi(p, );
> -if (val < 0) {
> +ret = qemu_strtosz_mebi(p, , );
> +if (ret < 0) {
>  monitor_printf(mon, "invalid size\n");
>  goto fail;
>  }
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 911a0ee..aac2e09 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -482,14 +482,15 @@ 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);
>  if (!opt) {
>  return;
>  }
>  
> -val = qemu_strtosz(opt->str ? opt->str : "", NULL);
> -if (val < 0) {
> +err = 

Re: [Qemu-devel] [PATCH 22/24] util/cutils: Return qemu_strtosz*() error and value separately

[Markus Armbruster]

Eric Blake  writes:

> On 02/14/2017 04:26 AM, Markus Armbruster wrote:
>> This makes qemu_strtosz(), qemu_strtosz_mebi() and
>> qemu_strtosz_metric() similar to qemu_strtoi64(), except negative
>> values are rejected.
>
> Yay. It also opens the door to allowing them to return an unsigned 64
> bit value ;)
>
>> 
>> 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-bl...@nongnu.org (open list:Block layer core)
>> Signed-off-by: Markus Armbruster 
>> ---
>> @@ -1378,8 +1378,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
>> QDict *qdict)
>>  break;
>>  case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>>  p.has_max_bandwidth = true;
>> -valuebw = qemu_strtosz_mebi(valuestr, NULL);
>> -if (valuebw < 0 || (size_t)valuebw != valuebw) {
>> +ret = qemu_strtosz_mebi(valuestr, NULL, );
>> +if (ret < 0 || (size_t)valuebw != valuebw) {
>
> Question: do we want a wrapper that takes a maximum, as in:
>
> ret = qemu_strtosz_capped(valuestr, NULL, 'M', SIZE_MAX, );
>
> so that the caller doesn't have to check (size_t)valuebw != valuebw ?

Maybe.

It's a more interesting question for qemu_strtou64(), actually.  The
obvious

err = qemu_strtou64(str, NULL, 0, );
if (value > limit) {
err = -ERANGE;
}

is subtly weird.  With str = "-1", you get value = UINT64_MAX.  With str
= "-18446744073709551615", you get value = 1.  Rejecting the former but
accepting the latter is weird.

> But not essential, so I can live with what you did here.
>
>
>> +++ b/qemu-img.c
>> @@ -370,10 +370,14 @@ static int add_old_style_options(const char *fmt, 
>> QemuOpts *opts,
>>  
>>  static int64_t cvtnum(const char *s)
>
>> +++ b/qemu-io-cmds.c
>> @@ -137,10 +137,14 @@ static char **breakline(char *input, int *count)
>>  
>>  static int64_t cvtnum(const char *s)
>
> May be some fallout based on rebase churn from earlier patch reviews,
> but nothing too serious to prevent you from adding:
>
> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH 22/24] util/cutils: Return qemu_strtosz*() error and value separately

[Eric Blake]

On 02/14/2017 04:26 AM, Markus Armbruster wrote:
> This makes qemu_strtosz(), qemu_strtosz_mebi() and
> qemu_strtosz_metric() similar to qemu_strtoi64(), except negative
> values are rejected.

Yay. It also opens the door to allowing them to return an unsigned 64
bit value ;)

> 
> 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-bl...@nongnu.org (open list:Block layer core)
> Signed-off-by: Markus Armbruster 
> ---
> @@ -1378,8 +1378,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const 
> QDict *qdict)
>  break;
>  case MIGRATION_PARAMETER_MAX_BANDWIDTH:
>  p.has_max_bandwidth = true;
> -valuebw = qemu_strtosz_mebi(valuestr, NULL);
> -if (valuebw < 0 || (size_t)valuebw != valuebw) {
> +ret = qemu_strtosz_mebi(valuestr, NULL, );
> +if (ret < 0 || (size_t)valuebw != valuebw) {

Question: do we want a wrapper that takes a maximum, as in:

ret = qemu_strtosz_capped(valuestr, NULL, 'M', SIZE_MAX, );

so that the caller doesn't have to check (size_t)valuebw != valuebw ?

But not essential, so I can live with what you did here.


> +++ b/qemu-img.c
> @@ -370,10 +370,14 @@ static int add_old_style_options(const char *fmt, 
> QemuOpts *opts,
>  
>  static int64_t cvtnum(const char *s)

> +++ b/qemu-io-cmds.c
> @@ -137,10 +137,14 @@ static char **breakline(char *input, int *count)
>  
>  static int64_t cvtnum(const char *s)

May be some fallout based on rebase churn from earlier patch reviews,
but nothing too serious to prevent you from adding:

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

[Qemu-devel] [PATCH 22/24] util/cutils: Return qemu_strtosz*() error and value separately

[Markus Armbruster]

This makes qemu_strtosz(), qemu_strtosz_mebi() and
qemu_strtosz_metric() similar to qemu_strtoi64(), except negative
values are rejected.

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-bl...@nongnu.org (open list:Block layer core)
Signed-off-by: Markus Armbruster 
---
 hmp.c |   6 +--
 hw/misc/ivshmem.c |   7 ++-
 include/qemu/cutils.h |   6 +--
 monitor.c |   5 ++-
 qapi/opts-visitor.c   |   5 ++-
 qemu-img.c|  10 +++--
 qemu-io-cmds.c|  10 +++--
 target/i386/cpu.c |   5 ++-
 tests/test-cutils.c   | 120 ++
 util/cutils.c |  22 -
 10 files changed, 119 insertions(+), 77 deletions(-)

diff --git a/hmp.c b/hmp.c
index 0eb5b6d..9846fa4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1342,7 +1342,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
*qdict)
 long valueint = 0;
 Error *err = NULL;
 bool use_int_value = false;
-int i;
+int i, ret;
 
 for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
 if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
@@ -1378,8 +1378,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
*qdict)
 break;
 case MIGRATION_PARAMETER_MAX_BANDWIDTH:
 p.has_max_bandwidth = true;
-valuebw = qemu_strtosz_mebi(valuestr, NULL);
-if (valuebw < 0 || (size_t)valuebw != valuebw) {
+ret = qemu_strtosz_mebi(valuestr, NULL, );
+if (ret < 0 || (size_t)valuebw != valuebw) {
 error_setg(, "Invalid size %s", valuestr);
 goto cleanup;
 }
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index f00cd75..3dc04f4 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1267,8 +1267,11 @@ static void ivshmem_realize(PCIDevice *dev, Error **errp)
 if (s->sizearg == NULL) {
 s->legacy_size = 4 << 20; /* 4 MB default */
 } else {
-int64_t size = qemu_strtosz_mebi(s->sizearg, NULL);
-if (size < 0 || (size_t)size != size || !is_power_of_2(size)) {
+int ret;
+int64_t size;
+
+ret = qemu_strtosz_mebi(s->sizearg, NULL, );
+if (ret < 0 || (size_t)size != size || !is_power_of_2(size)) {
 error_setg(errp, "Invalid size %s", s->sizearg);
 return;
 }
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 4184851..c91649b 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -139,9 +139,9 @@ int parse_uint(const char *s, unsigned long long *value, 
char **endptr,
int base);
 int parse_uint_full(const char *s, unsigned long long *value, int base);
 
-int64_t qemu_strtosz(const char *nptr, char **end);
-int64_t qemu_strtosz_mebi(const char *nptr, char **end);
-int64_t qemu_strtosz_metric(const char *nptr, char **end);
+int qemu_strtosz(const char *nptr, char **end, int64_t *result);
+int qemu_strtosz_mebi(const char *nptr, char **end, int64_t *result);
+int qemu_strtosz_metric(const char *nptr, char **end, int64_t *result);
 
 #define K_BYTE (1ULL << 10)
 #define M_BYTE (1ULL << 20)
diff --git a/monitor.c b/monitor.c
index 1f8c031..85b1b61 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2773,6 +2773,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 break;
 case 'o':
 {
+int ret;
 int64_t val;
 char *end;
 
@@ -2785,8 +2786,8 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 break;
 }
 }
-val = qemu_strtosz_mebi(p, );
-if (val < 0) {
+ret = qemu_strtosz_mebi(p, , );
+if (ret < 0) {
 monitor_printf(mon, "invalid size\n");
 goto fail;
 }
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 911a0ee..aac2e09 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -482,14 +482,15 @@ 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);
 if (!opt) {
 return;
 }
 
-val = qemu_strtosz(opt->str ? opt->str : "", NULL);
-if (val < 0) {
+err = qemu_strtosz(opt->str ? opt->str : "", NULL, );
+if (err < 0) {
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
"a size value representible as a non-negative int64");
 return;
diff --git a/qemu-img.c b/qemu-img.c
index adcb511..89ced5a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -370,10 +370,14 @@ static int