Re: [Qemu-devel] [PATCH v2 6/8] monitor: New argument type 'T'

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 20 Jan 2010 17:08:20 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> This is a double value with optional suffixes ms, us, ns.  We'll need
>> this to get migrate_set_downtime() QMP-ready.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c |   16 +++++++++++++++-
>>  1 files changed, 15 insertions(+), 1 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index ce97e7b..6dafe0b 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -75,6 +75,9 @@
>>   *              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
>>   * '/'          optional gdb-like print format (like "/10x")
>>   *
>>   * '?'          optional type (for all types, except '/')
>> @@ -3544,6 +3547,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>              }
>>              break;
>>          case 'b':
>> +        case 'T':
>>              {
>>                  double val;
>>  
>> @@ -3558,7 +3562,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                  if (get_double(mon, &val, &p) < 0) {
>>                      goto fail;
>>                  }
>> -                if (*p) {
>> +                if (c == 'b' && *p) {
>>                      switch (*p) {
>>                      case 'K': case 'k':
>>                          val *= 1 << 10; p++; break;
>> @@ -3568,6 +3572,16 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>>                          val *= 1 << 30; p++; break;
>>                      }
>>                  }
>> +                if (c == 'T' && p[0] && p[1] == 's') {
>
>  Is this indexing of 'p' really correct? What if the value you're interested
> is at the of the string? Like:
>
> .args_type = "str:s,value:b"

p points right behind the floating-point number.  If no characters
follow, it points to the string's terminating zero.  If exactly one
character follows, it points to that one, and the next one is the
terminating zero.  If more than one follow, p points to the first one.

"p[0] && p[1] == 's'" is safe, because p[0] is safe, and if p[0] != 0,
then p[1] is also safe.

Convinced?

>> +                    switch (*p) {
>> +                    case 'm':
>> +                        val /= 1e3; p += 2; break;
>> +                    case 'u':
>> +                        val /= 1e6; p += 2; break;
>> +                    case 'n':
>> +                        val /= 1e9; p += 2; break;
>> +                    }
>> +                }
>>                  if (*p && !qemu_isspace(*p)) {
>>                      monitor_printf(mon, "Unknown unit suffix\n");
>>                      goto fail;