Re: [Qemu-devel] [PATCH v8 09/35] qapi: Prefer type_int64 over type_int in visitors

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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 **errp);
>      void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
>      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 *name, 
> 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 = to_ov(v);
>      const QemuOpt *opt;
> @@ -528,7 +528,7 @@ opts_visitor_new(const QemuOpts *opts)
>       */
>      ov->visitor.type_enum = &input_type_enum;
>
> -    ov->visitor.type_int    = &opts_type_int;
> +    ov->visitor.type_int64  = &opts_type_int64;
>      ov->visitor.type_uint64 = &opts_type_uint64;
>      ov->visitor.type_size   = &opts_type_size;
>      ov->visitor.type_bool   = &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 = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
> -    v->visitor.type_int = qapi_dealloc_type_int;
> +    v->visitor.type_int64 = qapi_dealloc_type_int64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
>      v->visitor.type_str = qapi_dealloc_type_str;
>      v->visitor.type_number = 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, const 
> char *name, Error **errp)
>          v->type_uint8(v, obj, name, errp);
>      } else {
>          value = *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, const 
> char *name, Error **errp
>          v->type_uint16(v, obj, name, errp);
>      } else {
>          value = *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, const 
> char *name, Error **errp
>          v->type_uint32(v, obj, name, errp);
>      } else {
>          value = *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, const 
> char *name, Error **errp
>          v->type_uint64(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int(v, &value, name, errp);
> +        v->type_int64(v, &value, name, errp);
>          *obj = 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 = *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, const 
> char *name, Error **errp)
>          v->type_int16(v, obj, name, errp);
>      } else {
>          value = *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, const 
> char *name, Error **errp)
>          v->type_int32(v, obj, name, errp);
>      } else {
>          value = *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, const 
> 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 = *obj;
> -        v->type_int(v, &value, name, errp);
> +        v->type_int64(v, &value, name, errp);
>          *obj = 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 *name,
> -                               Error **errp)
> +static void qmp_input_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                                 Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      QInt *qint = 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 = qmp_input_next_list;
>      v->visitor.end_list = qmp_input_end_list;
>      v->visitor.type_enum = input_type_enum;
> -    v->visitor.type_int = qmp_input_type_int;
> +    v->visitor.type_int64 = qmp_input_type_int64;
>      v->visitor.type_bool = qmp_input_type_bool;
>      v->visitor.type_str = qmp_input_type_str;
>      v->visitor.type_number = 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 **errp)
>      qmp_output_pop(qov);
>  }
>
> -static void qmp_output_type_int(Visitor *v, int64_t *obj, const char *name,
> -                                Error **errp)
> +static void qmp_output_type_int64(Visitor *v, int64_t *obj, const char *name,
> +                                  Error **errp)
>  {
>      QmpOutputVisitor *qov = 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 = qmp_output_next_list;
>      v->visitor.end_list = qmp_output_end_list;
>      v->visitor.type_enum = output_type_enum;
> -    v->visitor.type_int = qmp_output_type_int;
> +    v->visitor.type_int64 = qmp_output_type_int64;
>      v->visitor.type_bool = qmp_output_type_bool;
>      v->visitor.type_str = qmp_output_type_str;
>      v->visitor.type_number = 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 = 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 = to_siv(v);
>
> @@ -335,7 +335,7 @@ StringInputVisitor *string_input_visitor_new(const char 
> *str)
>      v = g_malloc0(sizeof(*v));
>
>      v->visitor.type_enum = input_type_enum;
> -    v->visitor.type_int = parse_type_int;
> +    v->visitor.type_int64 = parse_type_int64;
>      v->visitor.type_size = parse_type_size;
>      v->visitor.type_bool = parse_type_bool;
>      v->visitor.type_str = 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, Range 
> *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 = to_sov(v);
>      GList *l;
> @@ -345,7 +345,7 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>      v->string = g_string_new(NULL);
>      v->human = human;
>      v->visitor.type_enum = output_type_enum;
> -    v->visitor.type_int = print_type_int;
> +    v->visitor.type_int64 = print_type_int64;
>      v->visitor.type_size = print_type_size;
>      v->visitor.type_bool = print_type_bool;
>      v->visitor.type_str = print_type_str;
> --
> 2.4.3
>
>

Anyhow, this patch is a welcome cleanup to me:
Reviewed-by: Marc-André Lureau <address@hidden>

-- 
Marc-André Lureau