From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGSGv-0002Sy-Fg for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGSGs-0000pz-Rd for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:36 -0500 Received: from mail-oi0-x22f.google.com ([2607:f8b0:4003:c06::22f]:33323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGSGs-0000pJ-Li for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:34 -0500 Received: by mail-oi0-x22f.google.com with SMTP id y66so274740634oig.0 for ; Tue, 05 Jan 2016 06:07:32 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1450717720-9627-12-git-send-email-eblake@redhat.com> References: <1450717720-9627-1-git-send-email-eblake@redhat.com> <1450717720-9627-12-git-send-email-eblake@redhat.com> Date: Tue, 5 Jan 2016 15:07:32 +0100 Message-ID: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small integer callbacks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Michael Roth , QEMU , Markus Armbruster Hi On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake wrote: > 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 > > --- > 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 | 22 +++--- > qapi/qapi-visit-core.c | 158 +++++++++++++++++---------------------= ------ > 2 files changed, 70 insertions(+), 110 deletions(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 70326e0..5ee2974 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 Red Hat, Inc. > * > * Author: Paolo Bonizni > * > @@ -36,6 +36,16 @@ struct Visitor > void (*get_next_type)(Visitor *v, QType *type, bool promote_int, > const char *name, Error **errp); > > + /* Must be set. */ > + void (*type_int64)(Visitor *v, int64_t *obj, const char *name, > + Error **errp); > + /* Must be set. */ > + void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, > + Error **errp); > + /* Optional; fallback is type_uint64(). */ > + void (*type_size)(Visitor *v, uint64_t *obj, const char *name, > + Error **errp); > + /* Must be set. */ > void (*type_bool)(Visitor *v, bool *obj, const char *name, Error **e= rrp); > void (*type_str)(Visitor *v, char **obj, const char *name, Error **e= rrp); > void (*type_number)(Visitor *v, double *obj, const char *name, > @@ -46,16 +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, Err= or **errp); > - void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Err= or **errp); > - void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name, Err= or **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); > - void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error= **errp); > - /* visit_type_size() falls back to (*type_uint64)() if type_size is = unset */ > - void (*type_size)(Visitor *v, uint64_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); > }; I think it would be a good idea to move this chunk in the previous patch. > 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, cons= t 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 **err= p) > +{ > + Error *err =3D NULL; > + uint64_t value =3D *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 =3D 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 =3D *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 =3D value; > - } > + uint64_t value =3D *obj; > + visit_type_uintN(v, &value, name, UINT8_MAX, "uint8_t", errp); > + *obj =3D value; > } > > -void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Erro= r **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 =3D *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 =3D value; > - } > + uint64_t value =3D *obj; > + visit_type_uintN(v, &value, name, UINT16_MAX, "uint16_t", errp); > + *obj =3D value; > } > > -void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Erro= r **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 =3D *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 =3D value; > - } > + uint64_t value =3D *obj; > + visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp); > + *obj =3D value; > } > > -void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Erro= r **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 =3D NULL; > + int64_t value =3D *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 =3D 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 =3D *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 =3D value; > - } > + int64_t value =3D *obj; > + visit_type_intN(v, &value, name, INT8_MIN, INT8_MAX, "int8_t", errp)= ; > + *obj =3D 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 =3D *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 =3D value; > - } > + int64_t value =3D *obj; > + visit_type_intN(v, &value, name, INT16_MIN, INT16_MAX, "int16_t", er= rp); > + *obj =3D 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 =3D *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 =3D value; > - } > + int64_t value =3D *obj; > + visit_type_intN(v, &value, name, INT32_MIN, INT32_MAX, "int32_t", er= rp); > + *obj =3D value; > } > > void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error = **errp) > -- > 2.4.3 > > Looks good otherwise. Reviewed-by: Marc-Andr=C3=A9 Lureau --=20 Marc-Andr=C3=A9 Lureau