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: qemu-sta...@nongnu.org
>> Signed-off-by: Andreas Färber <afaer...@suse.de>
>> ---
>> 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
