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: [email protected] >> Signed-off-by: Andreas Färber <[email protected]> >> --- >> 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
