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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

On 01/20/2016 10:34 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---
>> 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
moot.]

>> +    } 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]