[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small intege

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small integer callbacks
Date: Wed, 20 Jan 2016 11:17:31 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 01/20/2016 10:34 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>> Commit 4e27e819 introduced optional visitor callbacks for all
>> sorts of int types, but no visitor has supplied any of the
>> callbacks for sizes less than 64 bits.  In other words, the
>> generic implementation based on using type_[u]int64() followed
>> by bounds-checking works just fine. In the interest of
>> simplicity, it's easier to make the visitor callback interface
>> not have to worry about the other sizes.
>> Adding some helper functions minimizes the boilerplate required
>> to correct FIXMEs added earlier with regards to questionable
>> reuse of errp, particularly now that we can guarantee from a
>> single file audit that value is unchanged if an error is set.
>> Signed-off-by: Eric Blake <address@hidden>
>> Reviewed-by: Marc-André Lureau <address@hidden>
>> ---
>> v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
>> v8: no change
>> v7: further factor out helper functions that eliminate the
>> questionable errp reuse
>> v6: split off from v5 23/46
>> original version also appeared in v6-v9 of subset D
>> ---
>>  include/qapi/visitor-impl.h |   8 +--
>>  qapi/qapi-visit-core.c      | 158 
>> +++++++++++++++++---------------------------
>>  2 files changed, 60 insertions(+), 106 deletions(-)
> Nice diffstat.
>> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
>> index 45c1d3e..e6399d1 100644
>> --- a/include/qapi/visitor-impl.h
>> +++ b/include/qapi/visitor-impl.h
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Core Definitions for QAPI Visitor implementations
>>   *
>> - * Copyright (C) 2012 Red Hat, Inc.
>> + * Copyright (C) 2012, 2015-2016 Red Hat, Inc.
> git-log has authors @redhat.com in 2013 and 2014 as well.

I didn't bother to check whether those edits were complex enough to
warrant claiming copyright additions; but I'm also amenable to
shortening to '2012-2016' on a respin regardless of the sizes of those
edits.  As it is, I was not very careful about adding 2016 in the v9
spin, so I may be inconsistent on which years are claimed where, in
comparison to where I felt I was adding new copyrightable content rather
than just minor fixing of existing content.

[While it's nice that we DON'T have to assign copyright to a central
organization to contribute to qemu, sometimes it is much nicer working
on FSF code where a single copyright holder makes discussions like this

>> +    } else if (value > max) {
>> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                   name ? name : "null", type);
> We should clean up this name ? name : "null" nonsense some day.

It all stems from whether the visitor is locally visiting { 'name':value
} vs. [ value ]; in an array visit, there is no direct name.

Maybe we could go a step higher; if we have:

{ 'name': [ value ] }

then we could require callers to parse value by passing in "[name]" or
"name[0]" rather than NULL.  Then audit the code to always pass in a
sensible name at the top level of a parse.  It would even extend to 2-D
arrays, via "[[name]]" or "name[0][0]".  But not in this series.

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]