| Message ID | 1453219845-30939-12-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > * > * Author: Paolo Bonizni <pbonzini@redhat.com> > * > @@ -56,12 +56,6 @@ struct Visitor > /* May be NULL; most useful for input visitors. */ > void (*optional)(Visitor *v, bool *present, const char *name); > > - void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp); > - void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp); > - void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp); > - void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp); > - void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp); > - void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp); > bool (*start_union)(Visitor *v, bool data_present, Error **errp); > void (*end_union)(Visitor *v, bool data_present, Error **errp); > }; > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 4a8ad43..a48fd4e 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -100,129 +100,89 @@ void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) > v->type_int64(v, obj, name, errp); > } > > +static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name, > + uint64_t max, const char *type, Error **errp) > +{ > + Error *err = NULL; > + uint64_t value = *obj; > + > + v->type_uint64(v, &value, name, &err); > + if (err) { > + error_propagate(errp, err); > + } 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. > + } else { > + *obj = value; > + } > +} [...]
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> writes: > 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. 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.
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. * * Author: Paolo Bonizni <pbonzini@redhat.com> * @@ -56,12 +56,6 @@ struct Visitor /* May be NULL; most useful for input visitors. */ void (*optional)(Visitor *v, bool *present, const char *name); - void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp); - void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp); - void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp); - void (*type_int8)(Visitor *v, int8_t *obj, const char *name, Error **errp); - void (*type_int16)(Visitor *v, int16_t *obj, const char *name, Error **errp); - void (*type_int32)(Visitor *v, int32_t *obj, const char *name, Error **errp); bool (*start_union)(Visitor *v, bool data_present, Error **errp); void (*end_union)(Visitor *v, bool data_present, Error **errp); }; diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 4a8ad43..a48fd4e 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -100,129 +100,89 @@ void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) v->type_int64(v, obj, name, errp); } +static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name, + uint64_t max, const char *type, Error **errp) +{ + Error *err = NULL; + uint64_t value = *obj; + + v->type_uint64(v, &value, name, &err); + if (err) { + error_propagate(errp, err); + } else if (value > max) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + name ? name : "null", type); + } else { + *obj = value; + } +} + void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp) { - uint64_t value; - - if (v->type_uint8) { - v->type_uint8(v, obj, name, errp); - } else { - value = *obj; - v->type_uint64(v, &value, name, errp); - if (value > UINT8_MAX) { - /* FIXME questionable reuse of errp if type_uint64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "uint8_t"); - return; - } - *obj = value; - } + uint64_t value = *obj; + visit_type_uintN(v, &value, name, UINT8_MAX, "uint8_t", errp); + *obj = value; } -void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp) +void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, + Error **errp) { - uint64_t value; - - if (v->type_uint16) { - v->type_uint16(v, obj, name, errp); - } else { - value = *obj; - v->type_uint64(v, &value, name, errp); - if (value > UINT16_MAX) { - /* FIXME questionable reuse of errp if type_uint64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "uint16_t"); - return; - } - *obj = value; - } + uint64_t value = *obj; + visit_type_uintN(v, &value, name, UINT16_MAX, "uint16_t", errp); + *obj = value; } -void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp) +void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, + Error **errp) { - uint64_t value; - - if (v->type_uint32) { - v->type_uint32(v, obj, name, errp); - } else { - value = *obj; - v->type_uint64(v, &value, name, errp); - if (value > UINT32_MAX) { - /* FIXME questionable reuse of errp if type_uint64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "uint32_t"); - return; - } - *obj = value; - } + uint64_t value = *obj; + visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp); + *obj = value; } -void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) +void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, + Error **errp) { v->type_uint64(v, obj, name, errp); } +static void visit_type_intN(Visitor *v, int64_t *obj, const char *name, + int64_t min, int64_t max, const char *type, + Error **errp) +{ + Error *err = NULL; + int64_t value = *obj; + + v->type_int64(v, &value, name, &err); + if (err) { + error_propagate(errp, err); + } else if (value < min || value > max) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + name ? name : "null", type); + } else { + *obj = value; + } +} + void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp) { - int64_t value; - - if (v->type_int8) { - v->type_int8(v, obj, name, errp); - } else { - value = *obj; - v->type_int64(v, &value, name, errp); - if (value < INT8_MIN || value > INT8_MAX) { - /* FIXME questionable reuse of errp if type_int64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "int8_t"); - return; - } - *obj = value; - } + int64_t value = *obj; + visit_type_intN(v, &value, name, INT8_MIN, INT8_MAX, "int8_t", errp); + *obj = value; } void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp) { - int64_t value; - - if (v->type_int16) { - v->type_int16(v, obj, name, errp); - } else { - value = *obj; - v->type_int64(v, &value, name, errp); - if (value < INT16_MIN || value > INT16_MAX) { - /* FIXME questionable reuse of errp if type_int64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "int16_t"); - return; - } - *obj = value; - } + int64_t value = *obj; + visit_type_intN(v, &value, name, INT16_MIN, INT16_MAX, "int16_t", errp); + *obj = value; } void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp) { - int64_t value; - - if (v->type_int32) { - v->type_int32(v, obj, name, errp); - } else { - value = *obj; - v->type_int64(v, &value, name, errp); - if (value < INT32_MIN || value > INT32_MAX) { - /* FIXME questionable reuse of errp if type_int64() changes - value on error */ - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, - name ? name : "null", "int32_t"); - return; - } - *obj = value; - } + int64_t value = *obj; + visit_type_intN(v, &value, name, INT32_MIN, INT32_MAX, "int32_t", errp); + *obj = value; } void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)