Re: [Qemu-devel] [PATCH v9 11/37] qapi: Consolidate visitor small integer callbacks

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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 <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v9: hoist some of visitor-impl.h changes into 9/35 and 10/35
> 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 |   8 +--
>  qapi/qapi-visit-core.c      | 158 
> +++++++++++++++++---------------------------
>  2 files changed, 60 insertions(+), 106 deletions(-)

Nice diffstat.

>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 45c1d3e..e6399d1 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-2016 Red Hat, Inc.

git-log has authors @redhat.com in 2013 and 2014 as well.

>   *
>   * Author: Paolo Bonizni <address@hidden>
>   *
> @@ -56,12 +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, Error 
> **errp);
> -    void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error 
> **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);
>      bool (*start_union)(Visitor *v, bool data_present, Error **errp);
>      void (*end_union)(Visitor *v, bool data_present, Error **errp);
>  };
> 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, const 
> 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 **errp)
> +{
> +    Error *err = NULL;
> +    uint64_t value = *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);

We should clean up this name ? name : "null" nonsense some day.

> +    } else {
> +        *obj = value;
> +    }
> +}
[...]