Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand <david@redhat.com>]

On 31.10.18 15:40, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> The qemu api claims to be easier to use, and the resulting code seems to
>> agree.
> 
> Ah, an opportunity to nitpick spelling!  "The QEMU API", and "qapi: Use
> qemu_strtoi64() ..."

Whatever floats your boat ;) Will change.

> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  qapi/string-input-visitor.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b3fdd0827d..c1454f999f 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -20,6 +20,7 @@
>>  #include "qemu/option.h"
>>  #include "qemu/queue.h"
>>  #include "qemu/range.h"
>> +#include "qemu/cutils.h"
>>  
>>  
>>  struct StringInputVisitor
>> @@ -46,10 +47,10 @@ static void free_range(void *range, void *dummy)
>>  
>>  static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>  {
>> -    char *str = (char *) siv->string;
>> -    long long start, end;
>> +    const char *str = (char *) siv->string;
> 
> And an opportunity to nitpick whitespace!  Drop the space between (char
> *) and siv->string while there.

Shouldn't checkpatch complain, too? Will fix.

> 
>> +    const char *endptr;
>> +    int64_t start, end;
>>      Range *cur;
>> -    char *endptr;
>>  
>>      if (siv->ranges) {
>>          return 0;
>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>      }
>>  
>>      do {
>> -        errno = 0;
>> -        start = strtoll(str, &endptr, 0);
>> -        if (errno == 0 && endptr > str) {
>> +        if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>              if (*endptr == '\0') {
>>                  cur = g_malloc0(sizeof(*cur));
>>                  range_set_bounds(cur, start, start);
>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
>>                  str = NULL;
>>              } else if (*endptr == '-') {
>>                  str = endptr + 1;
>> -                errno = 0;
>> -                end = strtoll(str, &endptr, 0);
>> -                if (errno == 0 && endptr > str && start <= end &&
>> -                    (start > INT64_MAX - 65536 ||
>> -                     end < start + 65536)) {
>> +                if (!qemu_strtoi64(str, &endptr, 0, &end) && start < end) {
> 
> You deleted (start > INT64_MAX - 65536 || end < start + 65536).  Can you
> explain that to me?  I'm feeling particularly dense today...

qemu_strtoi64 performs all different kinds of error handling completely
internally. This old code here was an attempt to filter out -EWHATEVER
from the response. No longer needed as errors and the actual value are
reported via different ways.

Thanks!

> 
>>                      if (*endptr == '\0') {
>>                          cur = g_malloc0(sizeof(*cur));
>>                          range_set_bounds(cur, start, end);


-- 

Thanks,

David / dhildenb