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