qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v8 10/35] qapi: Make all visitors supply uint64


From: Marc-André Lureau
Subject: Re: [Qemu-devel] [PATCH v8 10/35] qapi: Make all visitors supply uint64 callbacks
Date: Tue, 5 Jan 2016 15:07:41 +0100

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> Our qapi visitor contract supports multiple integer visitors,
> but left the type_uint64 visitor as optional (falling back on
> type_int64); it also marks the type_size visitor as optional
> (falling back on type_uint64 or even type_int64).
>
> Note that the default of falling back on type_int for unsigned
> visitors can cause confusing results for values larger than
> INT64_MAX (such as having to pass in a negative 2s complement
> value on input, and getting a negative result on output).
>
> This patch does not fully address the disparity in handling
> large values as negatives, but does move towards a cleaner
> situation where EVERY visitor provides both type_int64 and
> type_uint64 variants as entry points; then each client can
> either implement sane differences between the two, or document
> in place with a FIXME that there is munging going on.
>
> The dealloc visitor no longer needs a type_size callback,
> since that now safely falls back to the type_uint64 callback.
>
> Then, in qapi-visit-core.c, we can now use the guaranteed
> type_uint64 callback as the fallback for all smaller unsigned
> int visits.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v8: no change
> v7: split off int64 callbacks and retitle, add more FIXMEs in the
> code, hoist use of type_uint64 here from 3/23, improved commit
> message
> v6: new patch, but stems from v5 23/46
> ---
>  qapi/qapi-dealloc-visitor.c  | 12 ++++++------
>  qapi/qapi-visit-core.c       | 42 ++++++++++++++----------------------------
>  qapi/qmp-input-visitor.c     | 17 +++++++++++++++++
>  qapi/qmp-output-visitor.c    |  9 +++++++++
>  qapi/string-input-visitor.c  | 15 +++++++++++++++
>  qapi/string-output-visitor.c |  9 +++++++++
>  6 files changed, 70 insertions(+), 34 deletions(-)
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index e9b9f3f..11eb828 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -140,6 +140,11 @@ static void qapi_dealloc_type_int64(Visitor *v, int64_t 
> *obj, const char *name,
>  {
>  }
>
> +static void qapi_dealloc_type_uint64(Visitor *v, uint64_t *obj,
> +                                     const char *name, Error **errp)
> +{
> +}
> +
>  static void qapi_dealloc_type_bool(Visitor *v, bool *obj, const char *name,
>                                     Error **errp)
>  {
> @@ -158,11 +163,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,11 +220,11 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.end_list = qapi_dealloc_end_list;
>      v->visitor.type_enum = qapi_dealloc_type_enum;
>      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);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 6295fa8..4a8ad43 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -102,15 +102,15 @@ void visit_type_int(Visitor *v, int64_t *obj, const 
> char *name, Error **errp)
>
>  void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error 
> **errp)
>  {
> -    int64_t value;
> +    uint64_t value;
>
>      if (v->type_uint8) {
>          v->type_uint8(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int64(v, &value, name, errp);
> -        if (value < 0 || value > UINT8_MAX) {
> -            /* FIXME questionable reuse of errp if type_int64() changes
> +        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");
> @@ -122,15 +122,15 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const 
> char *name, Error **errp)
>
>  void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error 
> **errp)
>  {
> -    int64_t value;
> +    uint64_t value;
>
>      if (v->type_uint16) {
>          v->type_uint16(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int64(v, &value, name, errp);
> -        if (value < 0 || value > UINT16_MAX) {
> -            /* FIXME questionable reuse of errp if type_int64() changes
> +        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");
> @@ -142,15 +142,15 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, const 
> char *name, Error **errp
>
>  void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error 
> **errp)
>  {
> -    int64_t value;
> +    uint64_t value;
>
>      if (v->type_uint32) {
>          v->type_uint32(v, obj, name, errp);
>      } else {
>          value = *obj;
> -        v->type_int64(v, &value, name, errp);
> -        if (value < 0 || value > UINT32_MAX) {
> -            /* FIXME questionable reuse of errp if type_int64() changes
> +        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");
> @@ -162,15 +162,7 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, const 
> char *name, Error **errp
>
>  void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error 
> **errp)
>  {
> -    int64_t value;
> -
> -    if (v->type_uint64) {
> -        v->type_uint64(v, obj, name, errp);
> -    } else {
> -        value = *obj;
> -        v->type_int64(v, &value, name, errp);
> -        *obj = value;
> -    }
> +    v->type_uint64(v, obj, name, errp);
>  }
>
>  void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
> @@ -240,16 +232,10 @@ void visit_type_int64(Visitor *v, int64_t *obj, const 
> char *name, Error **errp)
>
>  void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error 
> **errp)
>  {
> -    int64_t value;
> -
>      if (v->type_size) {
>          v->type_size(v, obj, name, errp);
> -    } else if (v->type_uint64) {
> -        v->type_uint64(v, obj, name, errp);
>      } else {
> -        value = *obj;
> -        v->type_int64(v, &value, name, errp);
> -        *obj = value;
> +        v->type_uint64(v, obj, name, errp);
>      }
>  }
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 0d8a3c3..32b60bb 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -239,6 +239,22 @@ static void qmp_input_type_int64(Visitor *v, int64_t 
> *obj, const char *name,
>      *obj = qint_get_int(qint);
>  }
>
> +static void qmp_input_type_uint64(Visitor *v, uint64_t *obj, const char 
> *name,
> +                                  Error **errp)
> +{
> +    /* FIXME: qobject_to_qint mishandles values over INT64_MAX */
> +    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_bool(Visitor *v, bool *obj, const char *name,
>                                  Error **errp)
>  {
> @@ -342,6 +358,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.end_list = qmp_input_end_list;
>      v->visitor.type_enum = input_type_enum;
>      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 3984011..f8eebaa 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -165,6 +165,14 @@ static void qmp_output_type_int64(Visitor *v, int64_t 
> *obj, const char *name,
>      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)
> +{
> +    /* FIXME: QMP outputs values larger than INT64_MAX as negative */
> +    QmpOutputVisitor *qov = to_qov(v);
> +    qmp_output_add(qov, name, qint_from_int(*obj));
> +}
> +
>  static void qmp_output_type_bool(Visitor *v, bool *obj, const char *name,
>                                   Error **errp)
>  {
> @@ -242,6 +250,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>      v->visitor.end_list = qmp_output_end_list;
>      v->visitor.type_enum = output_type_enum;
>      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 2f422f0..d7546b5 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -226,6 +226,20 @@ error:
>                 "an int64 value or range");
>  }
>
> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                              Error **errp)
> +{
> +    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
> +    int64_t i;
> +    Error *err = NULL;
> +    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)
>  {
> @@ -336,6 +350,7 @@ StringInputVisitor *string_input_visitor_new(const char 
> *str)
>
>      v->visitor.type_enum = input_type_enum;
>      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 c0a9331..3ed2b2c 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -197,6 +197,14 @@ static void print_type_int64(Visitor *v, int64_t *obj, 
> const char *name,
>      }
>  }
>
> +static void print_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                             Error **errp)
> +{
> +    /* FIXME: print_type_int64 mishandles values over INT64_MAX */
> +    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)
>  {
> @@ -346,6 +354,7 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>      v->human = human;
>      v->visitor.type_enum = output_type_enum;
>      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;
> --
> 2.4.3
>
>

Reviewed-by: Marc-André Lureau <address@hidden>


-- 
Marc-André Lureau



reply via email to

[Prev in Thread] Current Thread [Next in Thread]