Re: [Qemu-stable] [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Andreas Färber]

Am 25.09.2015 um 16:49 schrieb Eric Blake:
> On 09/25/2015 06:39 AM, Andreas Färber wrote:
>> All integers would get parsed by strtoll(), not handling the case of
>> UINT64 properties with the most significient bit set.
>>
>> Implement a .type_uint64 visitor callback, reusing the existing
>> parse_str() code through a new argument, using strtoull().
>>
>> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
>>
>> Cc: address@hidden
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  qapi/string-input-visitor.c | 57 
>> +++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 52 insertions(+), 5 deletions(-)
>>
> 
>> @@ -50,7 +50,11 @@ static void parse_str(StringInputVisitor *siv, Error 
>> **errp)
>>  
>>      do {
>>          errno = 0;
>> -        start = strtoll(str, &endptr, 0);
>> +        if (u64) {
>> +            start = strtoull(str, &endptr, 0);
> 
> accepts the range [-ULLONG_MAX, ULLONG_MAX] (with 2s complement
> wraparound). Do you really want -1 being a synonym for ULLONG_MAX, or do
> you want to explicitly reject leading '-' when parsing unsigned
> (arguments can be made for both behaviors; in fact, libvirt has two
> separate wrappers for parsing uint64_t depending on which behavior is
> wanted)
> 
>> +        } else {
>> +            start = strtoll(str, &endptr, 0);
> 
> accepts the range [LLONG_MIN, LLONG_MAX] (that is, roughly half the
> range of the unsigned version)

No one has further commented on this, so I take it no further changes
are required here for now.

>> +        }
>>          if (errno == 0 && endptr > str) {
>>              if (*endptr == '\0') {
>>                  cur = g_malloc0(sizeof(*cur));
>> @@ -60,7 +64,7 @@ static void parse_str(StringInputVisitor *siv, Error 
>> **errp)
>>                                                            range_compare);
>>                  cur = NULL;
>>                  str = NULL;
>> -            } else if (*endptr == '-') {
>> +            } else if (*endptr == '-' && !u64) {
> 
> Why do you not want to handle ranges when using unsigned numbers?

For some reason I must've read this as handling negative numbers, which
we wouldn't have for unsigned numbers...

However, since there is only one .start_list() callback, which passes
!u64 to retain previous behavior, we would never actually run into this
code path today. I've reverted my change and duplicated the strtoull()
handling instead nonetheless.

>>  
>> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
>> +                              Error **errp)
>> +{
>> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
>> +
>> +    if (!siv->string) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> +                   "integer");
>> +        return;
>> +    }
> ...
> 
> That's a lot of copy-and-paste. Can't you make parse_type_int64() and
> parse_type_uint64() both call into a single helper method, that contains
> the guts of the existing parse_type_int64() and adds a single parameter
> for the one place where the two functions differ on their call to
> parse_str()?

I don't see how. They have different signatures, and there's a lot of
gotos that differ in the error message. I'm all for sharing code but it
seems more work refactoring that code for reuse than duplication saved.
If you have a concrete suggestion how to improve it, please share a diff
or let's do that as follow-up.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

signature.asc
Description: OpenPGP digital signature