Re: [Qemu-devel] [PATCH v6 01/23] qapi: Make all visitors supply int64/uint64 callbacks

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Our qapi visitor contract supports multiple integer visitors:
> type_int (64-bit signed; mandatory), type_int64 (64-bit signed,
> optional), type_uint64 (64-bit unsigned, optional), and
> type_size (64-bit unsigned, optional, with the possibility of
> parsing differently than type_uint64 for the case of suffixed
> sizes).  But the default code treats any implementation without
> type_uint64 as just falling back on type_int, which can cause
> confusing results for values larger than LLONG_MAX (such as
> having to pass in a negative 2s complement value on input,
> and getting a negative result on output).

The visit_type_FOO() functions use their first argument's methods to do
their job.  For the integer visits, this involves picking the most
appropriate method from a set of candidates.  The set varies, because
some methods are optional.

Let me quickly review the state of things before this patch:

* visit_type_int() visits an int64_t and simply calls mandatory method
  type_int().

* The visit_type_intN() visit an intN_t.  They call optional method
  type_intN if it's there, else they use type_int() and check the result
  is in the type's range.

  Why optional type_int64() would ever differ from mandatory type_int()
  is anybody's guess.

* The visit_type_uintN() visit an uintN_t.  They call optional method
  type_uintN if it's there, else they use type_int() and check the
  result is in the type's range.  Except for type_uint64(), which simply
  converts the visited uint64_t to and from int64_t.  That's the issue
  you mentioned above.

* visit_type_size() visits an uint64_t.  It calls optional method
  type_size() if it's there, else optional type_uint64(), else
  type_int().  The latter again converts the visited uint64_t to and
  from int64_t.

Any change around here is prone to affect the weird cases, which always
confuse me, and that's why I'm going to read the patch very slowly.

> This patch does not fully address the disparity in parsing,
> but does move towards a cleaner situation where EVERY visitor
> provides both signed and unsigned variants as entry points;

Namely both type_int64() and type_uint64().

> then each client can either implement sane differences between
> the two, or document in place with a FIXME that there is
> munging going on for large values.  The next patch will then
> clean up the code to no longer allow conditional type_uint64.

I hope it also ditches type_int() as superfluous.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v6: new patch, but stems from v5 23/46
> ---
>  qapi/opts-visitor.c          |  5 +++--
>  qapi/qapi-dealloc-visitor.c  | 19 ++++++++++---------
>  qapi/qmp-input-visitor.c     | 23 ++++++++++++++++++++---
>  qapi/qmp-output-visitor.c    | 15 ++++++++++++---
>  qapi/string-input-visitor.c  | 22 +++++++++++++++++++---
>  qapi/string-output-visitor.c | 16 +++++++++++++---
>  6 files changed, 77 insertions(+), 23 deletions(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index ef5fb8b..bc2b976 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -354,7 +354,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 = DO_UPCAST(OptsVisitor, visitor, v);
>      const QemuOpt *opt;
> @@ -522,7 +522,8 @@ opts_visitor_new(const QemuOpts *opts)
>       */
>      ov->visitor.type_enum = &input_type_enum;
>
> -    ov->visitor.type_int    = &opts_type_int;
> +    ov->visitor.type_int    = &opts_type_int64;
> +    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;

Straightforward.  Note that type_int == type_int64 now.  Same for the
other visitors, actually.

> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 737deab..5d1b2e6 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -136,8 +136,13 @@ 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)
> +{
> +}
> +
> +static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
> +                                     const char *name, Error **errp)
>  {
>  }
>
> @@ -159,11 +164,6 @@ static void qapi_dealloc_type_anything(Visitor *v, 
> QObject **obj,
>      }
>  }
>
> -static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char 
> *name,
> -                                   Error **errp)
> -{
> -}
> -
>  static void qapi_dealloc_type_enum(Visitor *v, int *obj,
>                                     const char * const strings[],
>                                     const char *kind, const char *name,
> @@ -220,12 +220,13 @@ 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_int = qapi_dealloc_type_int64;
> +    v->visitor.type_int64 = qapi_dealloc_type_int64;
> +    v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
>      v->visitor.type_str = qapi_dealloc_type_str;
>      v->visitor.type_number = qapi_dealloc_type_number;
>      v->visitor.type_any = qapi_dealloc_type_anything;
> -    v->visitor.type_size = qapi_dealloc_type_size;
>      v->visitor.start_union = qapi_dealloc_start_union;
>
>      QTAILQ_INIT(&v->stack);

Same straightforward change to make type_int == type_int64.

Additionally, define type_uint64() instead of type_size().  No change
for visit_type_size().  visit_type_uint64() will now call type_uint64()
to do nothing instead of type_int().  Good.

> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 932b5f3..96dafcb 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -224,8 +224,23 @@ 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));
> +
> +    if (!qint) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "integer");
> +        return;
> +    }
> +
> +    *obj = qint_get_int(qint);
> +}
> +
> +static void qmp_input_type_uint64(Visitor *v, uint64_t *obj, const char 
> *name,
> +                                  Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      QInt *qint = qobject_to_qint(qmp_input_get_object(qiv, name, true));

Diff obscures what actually happens here, namely:

* Rename qmp_input_type_int() to qmp_input_type_int64().

* New qmp_input_type_uint64(), only difference to qmp_input_type_int64()
  is uint64_t instead of int64_t.  Note that QInt can only hold int64_t,
  so this still converts from int64_t to uint64_t, except it now does it
  in qmp_input_type_uint64() instead of visit_type_uint64().  Worth a
  FIXME?

> @@ -341,7 +356,9 @@ 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_int = qmp_input_type_int64;
> +    v->visitor.type_int64 = qmp_input_type_int64;
> +    v->visitor.type_uint64 = qmp_input_type_uint64;
>      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..a52f26f 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -158,8 +158,15 @@ 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));
> +}
> +
> +static void qmp_output_type_uint64(Visitor *v, uint64_t *obj, const char 
> *name,
> +                                   Error **errp)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
>      qmp_output_add(qov, name, qint_from_int(*obj));

Again, diff obscures the actual change:

* Rename qmp_output_type_int() to qmp_output_type_int64().

* New qmp_output_type_uint64(), like qmp_output_type_int64(), but with
  uint64_t instead of int64_t.  As above, this still converts from
  uint64_t to int64_t, except it now does it in qmp_output_type_int64()
  instead of visit_type_uint64().  Worth a FIXME?

> @@ -241,7 +248,9 @@ 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_int = qmp_output_type_int64;
> +    v->visitor.type_int64 = qmp_output_type_int64;
> +    v->visitor.type_uint64 = qmp_output_type_uint64;
>      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 dee780a..0931321 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -179,8 +179,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 = DO_UPCAST(StringInputVisitor, visitor, v);
>
> @@ -221,6 +221,20 @@ error:
>                 "an int64 value or range");
>  }
>
> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                              Error **errp)
> +{
> +    /* FIXME - don't handle numbers > INT64_MAX as negative */
> +    int64_t i;
> +    Error *err = NULL;

Blank line here, please.

> +    parse_type_int64(v, &i, name, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +    } else {
> +        *obj = i;
> +    }
> +}
> +
>  static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
>                              Error **errp)
>  {
> @@ -330,7 +344,9 @@ 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_int = parse_type_int64;
> +    v->visitor.type_int64 = parse_type_int64;
> +    v->visitor.type_uint64 = parse_type_uint64;
>      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 b86ce2c..83ca6cc 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -116,8 +116,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 = DO_UPCAST(StringOutputVisitor, visitor, v);
>      GList *l;
> @@ -192,6 +192,14 @@ static void print_type_int(Visitor *v, int64_t *obj, 
> const char *name,
>      }
>  }
>
> +static void print_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                             Error **errp)
> +{
> +    /* FIXME - don't handle numbers > INT64_MAX as negative */
> +    int64_t i = *obj;
> +    print_type_int64(v, &i, name, errp);
> +}
> +
>  static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
>                             Error **errp)
>  {
> @@ -340,7 +348,9 @@ 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_int = print_type_int64;
> +    v->visitor.type_int64 = print_type_int64;
> +    v->visitor.type_uint64 = print_type_uint64;
>      v->visitor.type_size = print_type_size;
>      v->visitor.type_bool = print_type_bool;
>      v->visitor.type_str = print_type_str;