[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small integer callbacks |
Date: |
Thu, 21 Jan 2016 10:05:14 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
- Re: [Qemu-devel] [PATCH v9 12/37] qapi: Don't cast Enum* to int*, (continued)
[Qemu-devel] [PATCH v9 10/37] qapi: Make all visitors supply uint64 callbacks, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small integer callbacks, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 08/37] qapi: Track all failures between visit_start/stop, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 09/37] qapi: Prefer type_int64 over type_int in visitors, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 20/37] qmp: Don't abuse stack to track qmp-output root, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 21/37] qapi: Document visitor interfaces, add assertions, Eric Blake, 2016/01/19