[Qemu-devel] [PATCH v4 0/7] Introduce strtosz and make use of it

[Qemu-devel] [PATCH v4 0/7] Introduce strtosz and make use of it

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

This patch introduces cutils.c: strtosz() and gets rid of the
multiple custom hacks for parsing byte sizes. In addition it adds
supports for specifying human style sizes such as 1.5G. Last it
eliminates the horrible abuse of a float to store the byte size for
migrate_set_speed in the monitor.

New in v3 I fixed the -1 case in the error handling pointed out by
Paolo and added a patch to clarify the default value for the monitor
command in the help message.

Jes

Jes Sorensen (7):
  Introduce strtosz() library function to convert a string to a byte
    count.
  Support human unit formats in strtosz, eg. 1.0G
  Add more error handling to strtosz()
  Add support for 'o' octet (bytes) format as monitor parameter.
  Switch migrate_set_speed() to take an 'o' argument rather than a
    float.
  Clarify default values in migration speed argument in monitor
  Remove obsolete 'f' double parameter type

 cutils.c        |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx |    5 ++-
 migration.c     |    4 +-
 monitor.c       |   46 ++++++++++++++++++++++++++---------------
 qemu-common.h   |    1 +
 vl.c            |   31 +++++++++-------------------
 6 files changed, 106 insertions(+), 42 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

strtosz() returns -1 on error.

v2 renamed from strtobytes() to strtosz() as suggested by Markus.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c      |   39 +++++++++++++++++++++++++++++++++++++++
 qemu-common.h |    1 +
 vl.c          |   31 ++++++++++---------------------
 3 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/cutils.c b/cutils.c
index 5883737..ee591c5 100644
--- a/cutils.c
+++ b/cutils.c
@@ -283,3 +283,42 @@ int fcntl_setfl(int fd, int flag)
 }
 #endif
 
+/*
+ * Convert string to bytes, allowing either K/k for KB, M/m for MB,
+ * G/g for GB or T/t for TB. Default without any postfix is MB.
+ * End pointer will be returned in *end, if end is valid.
+ * Return -1 on error.
+ */
+ssize_t strtosz(const char *nptr, char **end)
+{
+    int64_t value;
+    char *endptr;
+
+    value = strtoll(nptr, &endptr, 0);
+    switch (*endptr++) {
+    case 'K':
+    case 'k':
+        value <<= 10;
+        break;
+    case 0:
+    case 'M':
+    case 'm':
+        value <<= 20;
+        break;
+    case 'G':
+    case 'g':
+        value <<= 30;
+        break;
+    case 'T':
+    case 't':
+        value <<= 40;
+        break;
+    default:
+        value = -1;
+    }
+
+    if (end)
+        *end = endptr;
+
+    return value;
+}
diff --git a/qemu-common.h b/qemu-common.h
index 81aafa0..0a062d4 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
 int qemu_fls(int i);
 int qemu_fdatasync(int fd);
 int fcntl_setfl(int fd, int flag);
+ssize_t strtosz(const char *nptr, char **end);
 
 /* path.c */
 void init_paths(const char *prefix);
diff --git a/vl.c b/vl.c
index df414ef..6043fa2 100644
--- a/vl.c
+++ b/vl.c
@@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
         if (get_param_value(option, 128, "mem", optarg) == 0) {
             node_mem[nodenr] = 0;
         } else {
-            value = strtoull(option, &endptr, 0);
-            switch (*endptr) {
-            case 0: case 'M': case 'm':
-                value <<= 20;
-                break;
-            case 'G': case 'g':
-                value <<= 30;
-                break;
+            ssize_t sval;
+            sval = strtosz(option, NULL);
+            if (sval < 0) {
+                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
+                exit(1);
             }
-            node_mem[nodenr] = value;
+            node_mem[nodenr] = sval;
         }
         if (get_param_value(option, 128, "cpus", optarg) == 0) {
             node_cpumask[nodenr] = 0;
@@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
                 exit(0);
                 break;
             case QEMU_OPTION_m: {
-                uint64_t value;
-                char *ptr;
+                ssize_t value;
 
-                value = strtoul(optarg, &ptr, 10);
-                switch (*ptr) {
-                case 0: case 'M': case 'm':
-                    value <<= 20;
-                    break;
-                case 'G': case 'g':
-                    value <<= 30;
-                    break;
-                default:
+                value = strtosz(optarg, NULL);
+                if (value < 0) {
                     fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
                     exit(1);
                 }
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count.

[Markus Armbruster]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> strtosz() returns -1 on error.
>
> v2 renamed from strtobytes() to strtosz() as suggested by Markus.
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  cutils.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  qemu-common.h |    1 +
>  vl.c          |   31 ++++++++++---------------------
>  3 files changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 5883737..ee591c5 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -283,3 +283,42 @@ int fcntl_setfl(int fd, int flag)
>  }
>  #endif
>  
> +/*
> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
> + * G/g for GB or T/t for TB. Default without any postfix is MB.
> + * End pointer will be returned in *end, if end is valid.
> + * Return -1 on error.
> + */
> +ssize_t strtosz(const char *nptr, char **end)
> +{
> +    int64_t value;

long long, please, because that's what strtoll() returns.

> +    char *endptr;
> +
> +    value = strtoll(nptr, &endptr, 0);
> +    switch (*endptr++) {
> +    case 'K':
> +    case 'k':
> +        value <<= 10;
> +        break;
> +    case 0:
> +    case 'M':
> +    case 'm':
> +        value <<= 20;
> +        break;
> +    case 'G':
> +    case 'g':
> +        value <<= 30;
> +        break;
> +    case 'T':
> +    case 't':
> +        value <<= 40;
> +        break;
> +    default:
> +        value = -1;
> +    }
> +
> +    if (end)
> +        *end = endptr;
> +
> +    return value;

Casts value to ssize_t, which might truncate.

> +}

Sloppy use of strtoll().

Both tolerable as long as the patch doesn't make things worse.  Let's
see:

> diff --git a/qemu-common.h b/qemu-common.h
> index 81aafa0..0a062d4 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>  int qemu_fls(int i);
>  int qemu_fdatasync(int fd);
>  int fcntl_setfl(int fd, int flag);
> +ssize_t strtosz(const char *nptr, char **end);
>  
>  /* path.c */
>  void init_paths(const char *prefix);
> diff --git a/vl.c b/vl.c
> index df414ef..6043fa2 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>          if (get_param_value(option, 128, "mem", optarg) == 0) {
>              node_mem[nodenr] = 0;
>          } else {
> -            value = strtoull(option, &endptr, 0);
> -            switch (*endptr) {
> -            case 0: case 'M': case 'm':
> -                value <<= 20;
> -                break;
> -            case 'G': case 'g':
> -                value <<= 30;
> -                break;
> +            ssize_t sval;
> +            sval = strtosz(option, NULL);
> +            if (sval < 0) {
> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
> +                exit(1);

                        Before                          After
Invalid number          silently interpreted as zero    no change
Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
                                                        trunc ssize_t
Invalid size suffix     silently ignored                rejected

>              }
> -            node_mem[nodenr] = value;
> +            node_mem[nodenr] = sval;
>          }
>          if (get_param_value(option, 128, "cpus", optarg) == 0) {
>              node_cpumask[nodenr] = 0;
> @@ -2163,18 +2160,10 @@ int main(int argc, char **argv, char **envp)
>                  exit(0);
>                  break;
>              case QEMU_OPTION_m: {
> -                uint64_t value;
> -                char *ptr;
> +                ssize_t value;
>  
> -                value = strtoul(optarg, &ptr, 10);
> -                switch (*ptr) {
> -                case 0: case 'M': case 'm':
> -                    value <<= 20;
> -                    break;
> -                case 'G': case 'g':
> -                    value <<= 30;
> -                    break;
> -                default:
> +                value = strtosz(optarg, NULL);
> +                if (value < 0) {
>                      fprintf(stderr, "qemu: invalid ram size: %s\n", optarg);
>                      exit(1);
>                  }

                        Before                          After
Invalid number          silently interpreted as zero    no change
Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
                                                        trunc ssize_t
Invalid size suffix     rejected                        no change

A bit more context:


                   /* On 32-bit hosts, QEMU is limited by virtual address space */
                   if (value > (2047 << 20) && HOST_LONG_BITS == 32) {
                       fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
                       exit(1);
                   }
                   if (value != (uint64_t)(ram_addr_t)value) {
                       fprintf(stderr, "qemu: ram size too large\n");
                       exit(1);
                   }
                   ram_size = value;
                   break;

I'm afraid you break both conditionals for 32 bit hosts.

On such hosts, ssize_t is 32 bits wide.  strtosz() parses 64 bits
internally, but truncates to 32 bits silently.

The old code reliably rejects values larger than 2047MiB.  Your
truncation can change a value exceeding the limit to one within the
limit.  First conditional becomes unreliable.

The second conditional becomes useless: it sign-extends a non-negative
32 bit integer value to 64 bit, then truncates back, and checks the
value stays the same.  It trivially does.


I strongly recommend to make strtosz() sane from the start, not in a
later patch: proper error checking, including proper handling of
overflow.

Perhaps squashing 1-3/7 would get us there, or at least closer.

Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count.

[Jes Sorensen]

On 10/11/10 10:51, Markus Armbruster wrote:
> Jes.Sorensen@redhat.com writes:
>> +/*
>> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
>> + * G/g for GB or T/t for TB. Default without any postfix is MB.
>> + * End pointer will be returned in *end, if end is valid.
>> + * Return -1 on error.
>> + */
>> +ssize_t strtosz(const char *nptr, char **end)
>> +{
>> +    int64_t value;
> 
> long long, please, because that's what strtoll() returns.

I merged patches 1-3 as you suggested, so comments based on the combined
patch.

This is longer an issue as it is switched to strtod().

>> +    char *endptr;
>> +
>> +    value = strtoll(nptr, &endptr, 0);
>> +    switch (*endptr++) {
>> +    case 'K':
>> +    case 'k':
>> +        value <<= 10;
>> +        break;
>> +    case 0:
>> +    case 'M':
>> +    case 'm':
>> +        value <<= 20;
>> +        break;
>> +    case 'G':
>> +    case 'g':
>> +        value <<= 30;
>> +        break;
>> +    case 'T':
>> +    case 't':
>> +        value <<= 40;
>> +        break;
>> +    default:
>> +        value = -1;
>> +    }
>> +
>> +    if (end)
>> +        *end = endptr;
>> +
>> +    return value;
> 
> Casts value to ssize_t, which might truncate.

The new patch does:

    int64_t tmpval;
    tmpval = (val * mul);
    if (tmpval >= ~(size_t)0)
        goto fail;

so anything that is out of bounds is checked and caught before returning
a possibly truncated valued.

> Sloppy use of strtoll().    tmpval = (val * mul);
    if (tmpval >= ~(size_t)0)
        goto fail;
> 
> Both tolerable as long as the patch doesn't make things worse.  Let's
> see:
> 
>> diff --git a/qemu-common.h b/qemu-common.h
>> index 81aafa0..0a062d4 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>>  int qemu_fls(int i);
>>  int qemu_fdatasync(int fd);
>>  int fcntl_setfl(int fd, int flag);
>> +ssize_t strtosz(const char *nptr, char **end);
>>  
>>  /* path.c */
>>  void init_paths(const char *prefix);
>> diff --git a/vl.c b/vl.c
>> index df414ef..6043fa2 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>>          if (get_param_value(option, 128, "mem", optarg) == 0) {
>>              node_mem[nodenr] = 0;
>>          } else {
>> -            value = strtoull(option, &endptr, 0);
>> -            switch (*endptr) {
>> -            case 0: case 'M': case 'm':
>> -                value <<= 20;
>> -                break;
>> -            case 'G': case 'g':
>> -                value <<= 30;
>> -                break;
>> +            ssize_t sval;
>> +            sval = strtosz(option, NULL);
>> +            if (sval < 0) {
>> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
>> +                exit(1);
> 
>                         Before                          After
> Invalid number          silently interpreted as zero    no change
> Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
>                                                         trunc ssize_t
> Invalid size suffix     silently ignored                rejected

What do you mean by invalid number here?

LLONG_MAX seems a reasonable limit, ie. 31 bit on 32 bit hosts or 63
bits on 64 bit hosts. strtosz() is meant to return a size, so IMHO thats
a fair limitation. We could use size_t instead of ssize_t but it would
require ugly casts in any function calling the function to test for error.

>                         Before                          After
> Invalid number          silently interpreted as zero    no change
> Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
>                                                         trunc ssize_t
> Invalid size suffix     rejected                        no change
> 
> A bit more context:
> 
> 
>                    /* On 32-bit hosts, QEMU is limited by virtual address space */
>                    if (value > (2047 << 20) && HOST_LONG_BITS == 32) {
>                        fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
>                        exit(1);
>                    }
>                    if (value != (uint64_t)(ram_addr_t)value) {
>                        fprintf(stderr, "qemu: ram size too large\n");
>                        exit(1);
>                    }
>                    ram_size = value;
>                    break;
> 
> I'm afraid you break both conditionals for 32 bit hosts.
> 
> On such hosts, ssize_t is 32 bits wide.  strtosz() parses 64 bits
> internally, but truncates to 32 bits silently.

I believe the combined patch is taking care of this fine with the
>= ~(size_t)0 comparison. If the value is above that, it returns an
error. For 32 bit hosts that means we should be able to specify 2047MB
RAM fine.

The only place where I see the latter being a potential problem is on
P64 systems such as win64, since ram_addr_t is defined to be unsigned
long, but afaik we don't support win64 anyway. On both 32 bit and LP64
systems sizeof(ram_addr_t) == sizeof(ssize_t), so it should be fine.


> The old code reliably rejects values larger than 2047MiB.  Your
> truncation can change a value exceeding the limit to one within the
> limit.  First conditional becomes unreliable.
> 
> The second conditional becomes useless: it sign-extends a non-negative
> 32 bit integer value to 64 bit, then truncates back, and checks the
> value stays the same.  It trivially does.
> 
> I strongly recommend to make strtosz() sane from the start, not in a
> later patch: proper error checking, including proper handling of
> overflow.
> 
> Perhaps squashing 1-3/7 would get us there, or at least closer.

I have squashed 1-3, but I don't think 7 should be squashed. Adding a
new feature and taking away the old one in the same patch isn't right IMHO.

Cheers,
Jes

Re: [Qemu-devel] [PATCH 1/7] Introduce strtosz() library function to convert a string to a byte count.

[Markus Armbruster]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:

> On 10/11/10 10:51, Markus Armbruster wrote:
>> Jes.Sorensen@redhat.com writes:
>>> +/*
>>> + * Convert string to bytes, allowing either K/k for KB, M/m for MB,
>>> + * G/g for GB or T/t for TB. Default without any postfix is MB.
>>> + * End pointer will be returned in *end, if end is valid.
>>> + * Return -1 on error.
>>> + */
>>> +ssize_t strtosz(const char *nptr, char **end)
>>> +{
>>> +    int64_t value;
>> 
>> long long, please, because that's what strtoll() returns.
>
> I merged patches 1-3 as you suggested, so comments based on the combined
> patch.
>
> This is longer an issue as it is switched to strtod().

Okay, I'll review it.

>>> +    char *endptr;
>>> +
>>> +    value = strtoll(nptr, &endptr, 0);
>>> +    switch (*endptr++) {
>>> +    case 'K':
>>> +    case 'k':
>>> +        value <<= 10;
>>> +        break;
>>> +    case 0:
>>> +    case 'M':
>>> +    case 'm':
>>> +        value <<= 20;
>>> +        break;
>>> +    case 'G':
>>> +    case 'g':
>>> +        value <<= 30;
>>> +        break;
>>> +    case 'T':
>>> +    case 't':
>>> +        value <<= 40;
>>> +        break;
>>> +    default:
>>> +        value = -1;
>>> +    }
>>> +
>>> +    if (end)
>>> +        *end = endptr;
>>> +
>>> +    return value;
>> 
>> Casts value to ssize_t, which might truncate.
>
> The new patch does:
>
>     int64_t tmpval;
>     tmpval = (val * mul);
>     if (tmpval >= ~(size_t)0)
>         goto fail;
>
> so anything that is out of bounds is checked and caught before returning
> a possibly truncated valued.
>
>> Sloppy use of strtoll().    tmpval = (val * mul);
>     if (tmpval >= ~(size_t)0)
>         goto fail;
>> 
>> Both tolerable as long as the patch doesn't make things worse.  Let's
>> see:
>> 
>>> diff --git a/qemu-common.h b/qemu-common.h
>>> index 81aafa0..0a062d4 100644
>>> --- a/qemu-common.h
>>> +++ b/qemu-common.h
>>> @@ -153,6 +153,7 @@ time_t mktimegm(struct tm *tm);
>>>  int qemu_fls(int i);
>>>  int qemu_fdatasync(int fd);
>>>  int fcntl_setfl(int fd, int flag);
>>> +ssize_t strtosz(const char *nptr, char **end);
>>>  
>>>  /* path.c */
>>>  void init_paths(const char *prefix);
>>> diff --git a/vl.c b/vl.c
>>> index df414ef..6043fa2 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -734,16 +734,13 @@ static void numa_add(const char *optarg)
>>>          if (get_param_value(option, 128, "mem", optarg) == 0) {
>>>              node_mem[nodenr] = 0;
>>>          } else {
>>> -            value = strtoull(option, &endptr, 0);
>>> -            switch (*endptr) {
>>> -            case 0: case 'M': case 'm':
>>> -                value <<= 20;
>>> -                break;
>>> -            case 'G': case 'g':
>>> -                value <<= 30;
>>> -                break;
>>> +            ssize_t sval;
>>> +            sval = strtosz(option, NULL);
>>> +            if (sval < 0) {
>>> +                fprintf(stderr, "qemu: invalid numa mem size: %s\n", optarg);
>>> +                exit(1);
>> 
>>                         Before                          After
>> Invalid number          silently interpreted as zero    no change
>> Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
>>                                                         trunc ssize_t
>> Invalid size suffix     silently ignored                rejected
>
> What do you mean by invalid number here?

strtoul(nptr, &eptr, base) & friends skip whitespace, eat sign + digits,
stop at first unrecognized character.

What if they can't find any digits?  They store nptr in eptr and return
0.

> LLONG_MAX seems a reasonable limit, ie. 31 bit on 32 bit hosts or 63
> bits on 64 bit hosts. strtosz() is meant to return a size, so IMHO thats
> a fair limitation. We could use size_t instead of ssize_t but it would
> require ugly casts in any function calling the function to test for error.

64 bit hosts are fine; 2^63 should remain an acceptable implementation
limit for a while.

The use in main() is fine on 32 bit: the limit is 2047MiB, which fits
into a 32 bit ssize_t.  Wonder why the limit is 2047, not 2048MiB.  Oh
well.

As far as I can see, the use in numa_add() is also fine, because a
node's memory can't exceed total memory.

>>                         Before                          After
>> Invalid number          silently interpreted as zero    no change
>> Overflow                silently capped to ULLONG_MAX   LLONG_MAX, then
>>                                                         trunc ssize_t
>> Invalid size suffix     rejected                        no change
>> 
>> A bit more context:
>> 
>> 
>>                    /* On 32-bit hosts, QEMU is limited by virtual address space */
>>                    if (value > (2047 << 20) && HOST_LONG_BITS == 32) {
>>                        fprintf(stderr, "qemu: at most 2047 MB RAM can be simulated\n");
>>                        exit(1);
>>                    }
>>                    if (value != (uint64_t)(ram_addr_t)value) {
>>                        fprintf(stderr, "qemu: ram size too large\n");
>>                        exit(1);
>>                    }
>>                    ram_size = value;
>>                    break;
>> 
>> I'm afraid you break both conditionals for 32 bit hosts.
>> 
>> On such hosts, ssize_t is 32 bits wide.  strtosz() parses 64 bits
>> internally, but truncates to 32 bits silently.
>
> I believe the combined patch is taking care of this fine with the
>>= ~(size_t)0 comparison. If the value is above that, it returns an
> error. For 32 bit hosts that means we should be able to specify 2047MB
> RAM fine.
>
> The only place where I see the latter being a potential problem is on
> P64 systems such as win64, since ram_addr_t is defined to be unsigned
> long, but afaik we don't support win64 anyway. On both 32 bit and LP64
> systems sizeof(ram_addr_t) == sizeof(ssize_t), so it should be fine.
>
>
>> The old code reliably rejects values larger than 2047MiB.  Your
>> truncation can change a value exceeding the limit to one within the
>> limit.  First conditional becomes unreliable.
>> 
>> The second conditional becomes useless: it sign-extends a non-negative
>> 32 bit integer value to 64 bit, then truncates back, and checks the
>> value stays the same.  It trivially does.
>> 
>> I strongly recommend to make strtosz() sane from the start, not in a
>> later patch: proper error checking, including proper handling of
>> overflow.
>> 
>> Perhaps squashing 1-3/7 would get us there, or at least closer.
>
> I have squashed 1-3, but I don't think 7 should be squashed. Adding a
> new feature and taking away the old one in the same patch isn't right IMHO.

Misunderstanding?  I suggested to squash 1-3 *of* 7, not (1-3 and 7) of 7.

[Qemu-devel] [PATCH 2/7] Support human unit formats in strtosz, eg. 1.0G

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/cutils.c b/cutils.c
index ee591c5..0782032 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,34 +291,52 @@ int fcntl_setfl(int fd, int flag)
  */
 ssize_t strtosz(const char *nptr, char **end)
 {
-    int64_t value;
+    ssize_t retval = -1;
     char *endptr;
+    int mul_required = 0;
+    double val, mul = 1;
+
+    endptr = (char *)nptr + strspn(nptr, " 0123456789");
+    if (*endptr == '.') {
+        mul_required = 1;
+    }
+
+    val = strtod(nptr, &endptr);
+
+    if (val < 0)
+        goto fail;
 
-    value = strtoll(nptr, &endptr, 0);
     switch (*endptr++) {
     case 'K':
     case 'k':
-        value <<= 10;
+        mul = 1 << 10;
         break;
     case 0:
+    case ' ':
+        if (mul_required) {
+            goto fail;
+        }
     case 'M':
     case 'm':
-        value <<= 20;
+        mul = 1ULL << 20;
         break;
     case 'G':
     case 'g':
-        value <<= 30;
+        mul = 1ULL << 30;
         break;
     case 'T':
     case 't':
-        value <<= 40;
+        mul = 1ULL << 40;
         break;
     default:
-        value = -1;
+        goto fail;
     }
 
+    retval = (ssize_t)(val * mul);
+
     if (end)
         *end = endptr;
 
-    return value;
+fail:
+    return retval;
 }
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 2/7] Support human unit formats in strtosz, eg. 1.0G

[Stefan Weil]

Am 08.10.2010 11:15, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   cutils.c |   34 ++++++++++++++++++++++++++--------
>   1 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index ee591c5..0782032 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -291,34 +291,52 @@ int fcntl_setfl(int fd, int flag)
>    */
>   ssize_t strtosz(const char *nptr, char **end)
>   {
> -    int64_t value;
> +    ssize_t retval = -1;
>       char *endptr;
> +    int mul_required = 0;
> +    double val, mul = 1;
> +
> +    endptr = (char *)nptr + strspn(nptr, " 0123456789");
> +    if (*endptr == '.') {
> +        mul_required = 1;
> +    }
> +
> +    val = strtod(nptr,&endptr);
> +
> +    if (val<  0)
> +        goto fail;
>    

See CODING_STYLE.

>
> -    value = strtoll(nptr,&endptr, 0);
>       switch (*endptr++) {
>       case 'K':
>       case 'k':
> -        value<<= 10;
> +        mul = 1<<  10;
>           break;
>       case 0:
> +    case ' ':
> +        if (mul_required) {
> +            goto fail;
> +        }
>       case 'M':
>       case 'm':
> -        value<<= 20;
> +        mul = 1ULL<<  20;
>           break;
>       case 'G':
>       case 'g':
> -        value<<= 30;
> +        mul = 1ULL<<  30;
>           break;
>       case 'T':
>       case 't':
> -        value<<= 40;
> +        mul = 1ULL<<  40;
>           break;
>       default:
> -        value = -1;
> +        goto fail;
>       }
>
> +    retval = (ssize_t)(val * mul);
> +
>       if (end)
>           *end = endptr;
>
> -    return value;
> +fail:
> +    return retval;
>   }
>    

[Qemu-devel] [PATCH 3/7] Add more error handling to strtosz()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 cutils.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cutils.c b/cutils.c
index 0782032..e5a135e 100644
--- a/cutils.c
+++ b/cutils.c
@@ -292,6 +292,7 @@ int fcntl_setfl(int fd, int flag)
 ssize_t strtosz(const char *nptr, char **end)
 {
     ssize_t retval = -1;
+    int64_t tmpval;
     char *endptr;
     int mul_required = 0;
     double val, mul = 1;
@@ -301,9 +302,9 @@ ssize_t strtosz(const char *nptr, char **end)
         mul_required = 1;
     }
 
+    errno = 0;
     val = strtod(nptr, &endptr);
-
-    if (val < 0)
+    if (endptr == nptr || errno != 0 || val < 0)
         goto fail;
 
     switch (*endptr++) {
@@ -332,7 +333,10 @@ ssize_t strtosz(const char *nptr, char **end)
         goto fail;
     }
 
-    retval = (ssize_t)(val * mul);
+    tmpval = (val * mul);
+    if (tmpval >= ~(size_t)0)
+        goto fail;
+    retval = tmpval;
 
     if (end)
         *end = endptr;
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 3/7] Add more error handling to strtosz()

[Stefan Weil]

Am 08.10.2010 11:15, schrieb Jes.Sorensen@redhat.com:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   cutils.c |   10 +++++++---
>   1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/cutils.c b/cutils.c
> index 0782032..e5a135e 100644
> --- a/cutils.c
> +++ b/cutils.c
> @@ -292,6 +292,7 @@ int fcntl_setfl(int fd, int flag)
>   ssize_t strtosz(const char *nptr, char **end)
>   {
>       ssize_t retval = -1;
> +    int64_t tmpval;
>       char *endptr;
>       int mul_required = 0;
>       double val, mul = 1;
> @@ -301,9 +302,9 @@ ssize_t strtosz(const char *nptr, char **end)
>           mul_required = 1;
>       }
>
> +    errno = 0;
>       val = strtod(nptr,&endptr);
> -
> -    if (val<  0)
> +    if (endptr == nptr || errno != 0 || val<  0)
>           goto fail;
>    
See CODING_STYLE.
>
>       switch (*endptr++) {
> @@ -332,7 +333,10 @@ ssize_t strtosz(const char *nptr, char **end)
>           goto fail;
>       }
>
> -    retval = (ssize_t)(val * mul);
> +    tmpval = (val * mul);
> +    if (tmpval>= ~(size_t)0)
> +        goto fail;
>    
See CODING_STYLE.
> +    retval = tmpval;
>
>       if (end)
>           *end = endptr;
>    

[Qemu-devel] [PATCH 4/7] Add support for 'o' octet (bytes) format as monitor parameter.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Octet format relies on strtosz which supports K/k, M/m, G/g, T/t
suffixes and unit support for humans, like 1.3G

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 monitor.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index fbb678d..6dd1926 100644
--- a/monitor.c
+++ b/monitor.c
@@ -78,6 +78,11 @@
  * 'l'          target long (32 or 64 bit)
  * 'M'          just like 'l', except in user mode the value is
  *              multiplied by 2^20 (think Mebibyte)
+ * 'o'          octets (aka bytes)
+ *              user mode accepts an optional T, t, G, g, M, m, K, k
+ *              suffix, which multiplies the value by 2^40 for
+ *              suffixes T and t, 2^30 for suffixes G and g, 2^20 for
+ *              M and m, 2^10 for K and k
  * 'f'          double
  *              user mode accepts an optional G, g, M, m, K, k suffix,
  *              which multiplies the value by 2^30 for suffixes G and
@@ -3699,6 +3704,28 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 qdict_put(qdict, key, qint_from_int(val));
             }
             break;
+        case 'o':
+            {
+                ssize_t val;
+                char *end;
+
+                while (qemu_isspace(*p))
+                    p++;
+                if (*typestr == '?') {
+                    typestr++;
+                    if (*p == '\0') {
+                        break;
+                    }
+                }
+                val = strtosz(p, &end);
+                if (val < 0) {
+                    monitor_printf(mon, "invalid size\n");
+                    goto fail;
+                }
+                qdict_put(qdict, key, qint_from_int(val));
+                p = end;
+            }
+            break;
         case 'f':
         case 'T':
             {
@@ -4196,6 +4223,7 @@ static int check_client_args_type(const QDict *client_args,
         case 'i':
         case 'l':
         case 'M':
+        case 'o':
             if (qobject_type(client_arg) != QTYPE_QINT) {
                 qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
                               "int");
-- 
1.7.2.3

[Qemu-devel] [PATCH 5/7] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hmp-commands.hx |    2 +-
 migration.c     |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 81999aa..95bdb91 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -754,7 +754,7 @@ ETEXI
 
     {
         .name       = "migrate_set_speed",
-        .args_type  = "value:f",
+        .args_type  = "value:o",
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .user_print = monitor_user_noop,
diff --git a/migration.c b/migration.c
index 468d517..9ee8b17 100644
--- a/migration.c
+++ b/migration.c
@@ -132,10 +132,10 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    double d;
+    int64_t d;
     FdMigrationState *s;
 
-    d = qdict_get_double(qdict, "value");
+    d = qdict_get_int(qdict, "value");
     d = MAX(0, MIN(UINT32_MAX, d));
     max_throttle = d;
 
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 5/7] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Markus Armbruster]

[cc: Anthony, please review the proposed incompatible change of the
human monitor]

Jes.Sorensen@redhat.com writes:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  hmp-commands.hx |    2 +-
>  migration.c     |    4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81999aa..95bdb91 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -754,7 +754,7 @@ ETEXI
>  
>      {
>          .name       = "migrate_set_speed",
> -        .args_type  = "value:f",
> +        .args_type  = "value:o",
>          .params     = "value",
>          .help       = "set maximum speed (in bytes) for migrations",
>          .user_print = monitor_user_noop,
> diff --git a/migration.c b/migration.c
> index 468d517..9ee8b17 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -132,10 +132,10 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  
>  int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> -    double d;
> +    int64_t d;
>      FdMigrationState *s;
>  
> -    d = qdict_get_double(qdict, "value");
> +    d = qdict_get_int(qdict, "value");
>      d = MAX(0, MIN(UINT32_MAX, d));
>      max_throttle = d;

As noted before, this is an incompatible change of the human monitor
command: unit now defaults to 'M'.  This must be noted *prominently* in
the commit message.  Best in the subject.

Incompatible changes can break tools.  Quick grep of libvirt:

src/qemu/qemu_monitor_json.c:    cmd = qemuMonitorJSONMakeCommand("migrate_set_speed",
src/qemu/qemu_monitor_text.c:    if (virAsprintf(&cmd, "migrate_set_speed %lum", bandwidth) < 0) {

Looks like we're safe here.

Anthony, is this incompatibility okay?

Help should be updated in the same patch; please squash 6/7 into 5/7.

Re: [Qemu-devel] [PATCH 5/7] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Paolo Bonzini]

On 10/11/2010 11:03 AM, Markus Armbruster wrote:
> As noted before, this is an incompatible change of the human monitor
> command: unit now defaults to 'M'.  This must be noted*prominently*  in
> the commit message.  Best in the subject.
>
> Incompatible changes can break tools.  Quick grep of libvirt:
>
> src/qemu/qemu_monitor_json.c:    cmd = qemuMonitorJSONMakeCommand("migrate_set_speed",
> src/qemu/qemu_monitor_text.c:    if (virAsprintf(&cmd, "migrate_set_speed %lum", bandwidth)<  0) {
>
> Looks like we're safe here.
>
> Anthony, is this incompatibility okay?
>
> Help should be updated in the same patch; please squash 6/7 into 5/7.

Personally, I'd rather see the patch I sent incorporated, so that there 
is no change in the monitor at all.

Not going to make a fuss of it as long as libvirt is okay, though.

Paolo

Re: [Qemu-devel] [PATCH 5/7] Switch migrate_set_speed() to take an 'o' argument rather than a float.

[Jes Sorensen]

On 10/11/10 11:03, Markus Armbruster wrote:
> 
> As noted before, this is an incompatible change of the human monitor
> command: unit now defaults to 'M'.  This must be noted *prominently* in
> the commit message.  Best in the subject.
> 
> Incompatible changes can break tools.  Quick grep of libvirt:
> 
> src/qemu/qemu_monitor_json.c:    cmd = qemuMonitorJSONMakeCommand("migrate_set_speed",
> src/qemu/qemu_monitor_text.c:    if (virAsprintf(&cmd, "migrate_set_speed %lum", bandwidth) < 0) {
> 
> Looks like we're safe here.
> 
> Anthony, is this incompatibility okay?

I agree it is incompatible, however it was a conscious decision to get
rid of the inconsistency we had around various places for these types of
arguments. If there is strong opposition to this change, then I can
mangle the interface to allow for the old, but IMHO bad, default of the
monitor.

> Help should be updated in the same patch; please squash 6/7 into 5/7.

Ok will do.

Cheers,
Jes

[Qemu-devel] [PATCH 6/7] Clarify default values in migration speed argument in monitor

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 hmp-commands.hx |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 95bdb91..f138a76 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -756,7 +756,8 @@ ETEXI
         .name       = "migrate_set_speed",
         .args_type  = "value:o",
         .params     = "value",
-        .help       = "set maximum speed (in bytes) for migrations",
+        .help       = "set maximum speed (in bytes) for migrations. "
+	"Defaults to KB if no size suffix is specified, ie. K/M/G/T",
         .user_print = monitor_user_noop,
         .mhandler.cmd_new = do_migrate_set_speed,
     },
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 6/7] Clarify default values in migration speed argument in monitor

[Paolo Bonzini]

On 10/08/2010 11:15 AM, Jes.Sorensen@redhat.com wrote:
> From: Jes Sorensen<Jes.Sorensen@redhat.com>
>
> Signed-off-by: Jes Sorensen<Jes.Sorensen@redhat.com>
> ---
>   hmp-commands.hx |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 95bdb91..f138a76 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -756,7 +756,8 @@ ETEXI
>           .name       = "migrate_set_speed",
>           .args_type  = "value:o",
>           .params     = "value",
> -        .help       = "set maximum speed (in bytes) for migrations",
> +        .help       = "set maximum speed (in bytes) for migrations. "
> +	"Defaults to KB if no size suffix is specified, ie. K/M/G/T",
                      ^^

To MB, no?

Paolo

[Qemu-devel] Re: [PATCH 6/7] Clarify default values in migration speed argument in monitor

[Jes Sorensen]

On 10/08/10 18:21, Paolo Bonzini wrote:
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 95bdb91..f138a76 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -756,7 +756,8 @@ ETEXI
>>           .name       = "migrate_set_speed",
>>           .args_type  = "value:o",
>>           .params     = "value",
>> -        .help       = "set maximum speed (in bytes) for migrations",
>> +        .help       = "set maximum speed (in bytes) for migrations. "
>> +    "Defaults to KB if no size suffix is specified, ie. K/M/G/T",
>                      ^^
> 
> To MB, no?

DOH, you're right. Re-spin coming up shortly unless I receive anymore
comments.

Cheers,
Jes

[Qemu-devel] [PATCH 7/7] Remove obsolete 'f' double parameter type

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

'f' double is no longer used, and we should be using floating point
variables to store byte sizes. Remove it.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 monitor.c |   18 +-----------------
 1 files changed, 1 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6dd1926..6678fb5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -83,10 +83,6 @@
  *              suffix, which multiplies the value by 2^40 for
  *              suffixes T and t, 2^30 for suffixes G and g, 2^20 for
  *              M and m, 2^10 for K and k
- * 'f'          double
- *              user mode accepts an optional G, g, M, m, K, k suffix,
- *              which multiplies the value by 2^30 for suffixes G and
- *              g, 2^20 for M and m, 2^10 for K and k
  * 'T'          double
  *              user mode accepts an optional ms, us, ns suffix,
  *              which divides the value by 1e3, 1e6, 1e9, respectively
@@ -3726,7 +3722,6 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 p = end;
             }
             break;
-        case 'f':
         case 'T':
             {
                 double val;
@@ -3742,17 +3737,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                 if (get_double(mon, &val, &p) < 0) {
                     goto fail;
                 }
-                if (c == 'f' && *p) {
-                    switch (*p) {
-                    case 'K': case 'k':
-                        val *= 1 << 10; p++; break;
-                    case 'M': case 'm':
-                        val *= 1 << 20; p++; break;
-                    case 'G': case 'g':
-                        val *= 1 << 30; p++; break;
-                    }
-                }
-                if (c == 'T' && p[0] && p[1] == 's') {
+                if (p[0] && p[1] == 's') {
                     switch (*p) {
                     case 'm':
                         val /= 1e3; p += 2; break;
@@ -4230,7 +4215,6 @@ static int check_client_args_type(const QDict *client_args,
                 return -1; 
             }
             break;
-        case 'f':
         case 'T':
             if (qobject_type(client_arg) != QTYPE_QINT &&
                 qobject_type(client_arg) != QTYPE_QFLOAT) {
-- 
1.7.2.3