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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

IANAL, but I feel claiming 2012-2016 when some of the years in the
middle saw only changes of debatable copyrightability is a pardonable
sin.

> [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.

"Parameter 'null' expects int32_t" is not an acceptable error message.
It's better than crashing, but that's about it.

'null' is actively misleading.  The thing we choked on isn't named
'null'.  Even calling it a parameter is questionable.

A fix needs to identify the thing we choked on in a way that let's the
user find it.  '[[name]]' doesn't cut it, I'm afraid.

'expects int32_t' is below par, too.  Sure, a programmer will understand
it fine, but we need to explain things to the user in the user's term,
not in C programming jargon.