From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50322) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGSGx-0002ae-Kl for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGSGv-0000qv-LW for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:39 -0500 Received: from mail-oi0-x231.google.com ([2607:f8b0:4003:c06::231]:35663) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGSGv-0000qj-G6 for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:07:37 -0500 Received: by mail-oi0-x231.google.com with SMTP id l9so246646355oia.2 for ; Tue, 05 Jan 2016 06:07:37 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1450717720-9627-10-git-send-email-eblake@redhat.com> References: <1450717720-9627-1-git-send-email-eblake@redhat.com> <1450717720-9627-10-git-send-email-eblake@redhat.com> Date: Tue, 5 Jan 2016 15:07:37 +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 09/35] qapi: Prefer type_int64 over type_int in visitors 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: > The qapi builtin type 'int' is basically shorthand for the type > 'int64'. In fact, since no visitor was providing the optional > type_int64() callback, visit_type_int64() was just always falling > back to type_int(), cementing the equivalence between the types. > > However, some visitors are providing a type_uint64() callback. > For purposes of code consistency, it is nicer if all visitors > use the paired type_int64/type_uint64 names rather than the > mismatched type_int/type_uint64. So this patch just renames > the signed int callbacks in place, dropping the type_int() > callback as redundant, and a later patch will focus on the > unsigned int callbacks. > > Add some FIXMEs to questionable reuse of errp in code touched > by the rename, while at it (the reuse works as long as the > callbacks don't modify value when setting an error, but it's not > a good example to set). Hmm, that potentially could cause assert() in error_setv() (that could possibly trigger in so many untested ways I would rather see a warning, but ok). Rather than a FIXME, I would see a patch to check for errp before doing the further check and assert. I later realized that a following patch fixes it, I guess you could update commit message to mention it. > > No change in functionality here, although further cleanups are > in the pipeline. > > Signed-off-by: Eric Blake > > --- > v8: no change > v7: split off of 1/23 and 2/23 for easier-to-read diffs > --- > include/qapi/visitor-impl.h | 1 - > qapi/opts-visitor.c | 4 ++-- > qapi/qapi-dealloc-visitor.c | 6 +++--- > qapi/qapi-visit-core.c | 36 ++++++++++++++++++++++-------------- > qapi/qmp-input-visitor.c | 6 +++--- > qapi/qmp-output-visitor.c | 6 +++--- > qapi/string-input-visitor.c | 6 +++--- > qapi/string-output-visitor.c | 6 +++--- > 8 files changed, 39 insertions(+), 32 deletions(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 44a21b7..70326e0 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -36,7 +36,6 @@ struct Visitor > void (*get_next_type)(Visitor *v, QType *type, bool promote_int, > const char *name, Error **errp); > > - void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error *= *errp); > 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, > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index dd4094c..56c798f 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -360,7 +360,7 @@ opts_type_bool(Visitor *v, bool *obj, const char *nam= e, Error **errp) > > > static void > -opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) > +opts_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp= ) > { > OptsVisitor *ov =3D to_ov(v); > const QemuOpt *opt; > @@ -528,7 +528,7 @@ opts_visitor_new(const QemuOpts *opts) > */ > ov->visitor.type_enum =3D &input_type_enum; > > - ov->visitor.type_int =3D &opts_type_int; > + ov->visitor.type_int64 =3D &opts_type_int64; > ov->visitor.type_uint64 =3D &opts_type_uint64; > ov->visitor.type_size =3D &opts_type_size; > ov->visitor.type_bool =3D &opts_type_bool; > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 204de8f..e9b9f3f 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -135,8 +135,8 @@ static void qapi_dealloc_type_str(Visitor *v, char **= obj, const char *name, > } > } > > -static void qapi_dealloc_type_int(Visitor *v, int64_t *obj, const char *= name, > - Error **errp) > +static void qapi_dealloc_type_int64(Visitor *v, int64_t *obj, const char= *name, > + Error **errp) > { > } > > @@ -219,7 +219,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) > v->visitor.next_list =3D qapi_dealloc_next_list; > v->visitor.end_list =3D qapi_dealloc_end_list; > v->visitor.type_enum =3D qapi_dealloc_type_enum; > - v->visitor.type_int =3D qapi_dealloc_type_int; > + v->visitor.type_int64 =3D qapi_dealloc_type_int64; > v->visitor.type_bool =3D qapi_dealloc_type_bool; > v->visitor.type_str =3D qapi_dealloc_type_str; > v->visitor.type_number =3D qapi_dealloc_type_number; > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 6d63e40..6295fa8 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -97,7 +97,7 @@ void visit_type_enum(Visitor *v, int *obj, const char *= const strings[], > > void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **= errp) > { > - v->type_int(v, obj, name, errp); > + v->type_int64(v, obj, name, errp); > } > > void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error = **errp) > @@ -108,8 +108,10 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, cons= t char *name, Error **errp) > v->type_uint8(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + v->type_int64(v, &value, name, errp); > if (value < 0 || value > UINT8_MAX) { > + /* FIXME questionable reuse of errp if type_int64() changes > + value on error */ > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > name ? name : "null", "uint8_t"); > return; > @@ -126,8 +128,10 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, co= nst char *name, Error **errp > v->type_uint16(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + v->type_int64(v, &value, name, errp); > if (value < 0 || value > UINT16_MAX) { > + /* FIXME questionable reuse of errp if type_int64() changes > + value on error */ > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > name ? name : "null", "uint16_t"); > return; > @@ -144,8 +148,10 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, co= nst char *name, Error **errp > v->type_uint32(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + v->type_int64(v, &value, name, errp); > if (value < 0 || value > UINT32_MAX) { > + /* FIXME questionable reuse of errp if type_int64() changes > + value on error */ > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > name ? name : "null", "uint32_t"); > return; > @@ -162,7 +168,7 @@ void visit_type_uint64(Visitor *v, uint64_t *obj, con= st char *name, Error **errp > v->type_uint64(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + v->type_int64(v, &value, name, errp); > *obj =3D value; > } > } > @@ -175,8 +181,10 @@ void visit_type_int8(Visitor *v, int8_t *obj, const = char *name, Error **errp) > v->type_int8(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + 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; > @@ -193,8 +201,10 @@ void visit_type_int16(Visitor *v, int16_t *obj, cons= t char *name, Error **errp) > v->type_int16(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + 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; > @@ -211,8 +221,10 @@ void visit_type_int32(Visitor *v, int32_t *obj, cons= t char *name, Error **errp) > v->type_int32(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + 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; > @@ -223,11 +235,7 @@ void visit_type_int32(Visitor *v, int32_t *obj, cons= t char *name, Error **errp) > > void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error = **errp) > { > - if (v->type_int64) { > - v->type_int64(v, obj, name, errp); > - } else { > - v->type_int(v, obj, name, errp); > - } > + v->type_int64(v, obj, name, errp); > } > > void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error = **errp) > @@ -240,7 +248,7 @@ void visit_type_size(Visitor *v, uint64_t *obj, const= char *name, Error **errp) > v->type_uint64(v, obj, name, errp); > } else { > value =3D *obj; > - v->type_int(v, &value, name, errp); > + v->type_int64(v, &value, name, errp); > *obj =3D value; > } > } > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 932b5f3..0d8a3c3 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -224,8 +224,8 @@ static void qmp_input_get_next_type(Visitor *v, QType= *type, bool promote_int, > } > } > > -static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *nam= e, > - Error **errp) > +static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *n= ame, > + Error **errp) > { > QmpInputVisitor *qiv =3D to_qiv(v); > QInt *qint =3D qobject_to_qint(qmp_input_get_object(qiv, name, true)= ); > @@ -341,7 +341,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj) > v->visitor.next_list =3D qmp_input_next_list; > v->visitor.end_list =3D qmp_input_end_list; > v->visitor.type_enum =3D input_type_enum; > - v->visitor.type_int =3D qmp_input_type_int; > + v->visitor.type_int64 =3D qmp_input_type_int64; > v->visitor.type_bool =3D qmp_input_type_bool; > v->visitor.type_str =3D qmp_input_type_str; > v->visitor.type_number =3D qmp_input_type_number; > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index 29899ac..3984011 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -158,8 +158,8 @@ static void qmp_output_end_list(Visitor *v, Error **e= rrp) > qmp_output_pop(qov); > } > > -static void qmp_output_type_int(Visitor *v, int64_t *obj, const char *na= me, > - Error **errp) > +static void qmp_output_type_int64(Visitor *v, int64_t *obj, const char *= name, > + Error **errp) > { > QmpOutputVisitor *qov =3D to_qov(v); > qmp_output_add(qov, name, qint_from_int(*obj)); > @@ -241,7 +241,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void) > v->visitor.next_list =3D qmp_output_next_list; > v->visitor.end_list =3D qmp_output_end_list; > v->visitor.type_enum =3D output_type_enum; > - v->visitor.type_int =3D qmp_output_type_int; > + v->visitor.type_int64 =3D qmp_output_type_int64; > v->visitor.type_bool =3D qmp_output_type_bool; > v->visitor.type_str =3D qmp_output_type_str; > v->visitor.type_number =3D qmp_output_type_number; > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 7f5645b..2f422f0 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -184,8 +184,8 @@ end_list(Visitor *v, Error **errp) > siv->head =3D true; > } > > -static void parse_type_int(Visitor *v, int64_t *obj, const char *name, > - Error **errp) > +static void parse_type_int64(Visitor *v, int64_t *obj, const char *name, > + Error **errp) > { > StringInputVisitor *siv =3D to_siv(v); > > @@ -335,7 +335,7 @@ StringInputVisitor *string_input_visitor_new(const ch= ar *str) > v =3D g_malloc0(sizeof(*v)); > > v->visitor.type_enum =3D input_type_enum; > - v->visitor.type_int =3D parse_type_int; > + v->visitor.type_int64 =3D parse_type_int64; > v->visitor.type_size =3D parse_type_size; > v->visitor.type_bool =3D parse_type_bool; > v->visitor.type_str =3D parse_type_str; > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index 202764c..c0a9331 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -121,8 +121,8 @@ static void format_string(StringOutputVisitor *sov, R= ange *r, bool next, > } > } > > -static void print_type_int(Visitor *v, int64_t *obj, const char *name, > - Error **errp) > +static void print_type_int64(Visitor *v, int64_t *obj, const char *name, > + Error **errp) > { > StringOutputVisitor *sov =3D to_sov(v); > GList *l; > @@ -345,7 +345,7 @@ StringOutputVisitor *string_output_visitor_new(bool h= uman) > v->string =3D g_string_new(NULL); > v->human =3D human; > v->visitor.type_enum =3D output_type_enum; > - v->visitor.type_int =3D print_type_int; > + v->visitor.type_int64 =3D print_type_int64; > v->visitor.type_size =3D print_type_size; > v->visitor.type_bool =3D print_type_bool; > v->visitor.type_str =3D print_type_str; > -- > 2.4.3 > > Anyhow, this patch is a welcome cleanup to me: Reviewed-by: Marc-Andr=C3=A9 Lureau --=20 Marc-Andr=C3=A9 Lureau