[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small intege
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small integer callbacks |
Date: |
Tue, 5 Jan 2016 15:07:32 +0100 |
Hi
On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> 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>
>
> ---
> 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 | 22 +++---
> qapi/qapi-visit-core.c | 158
> +++++++++++++++++---------------------------
> 2 files changed, 70 insertions(+), 110 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 70326e0..5ee2974 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 Red Hat, Inc.
> *
> * Author: Paolo Bonizni <address@hidden>
> *
> @@ -36,6 +36,16 @@ struct Visitor
> void (*get_next_type)(Visitor *v, QType *type, bool promote_int,
> const char *name, Error **errp);
>
> + /* Must be set. */
> + void (*type_int64)(Visitor *v, int64_t *obj, const char *name,
> + Error **errp);
> + /* Must be set. */
> + void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
> + Error **errp);
> + /* Optional; fallback is type_uint64(). */
> + void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
> + Error **errp);
> + /* Must be set. */
> 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,
> @@ -46,16 +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_uint64)(Visitor *v, uint64_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);
> - void (*type_int64)(Visitor *v, int64_t *obj, const char *name, Error
> **errp);
> - /* visit_type_size() falls back to (*type_uint64)() if type_size is
> unset */
> - void (*type_size)(Visitor *v, uint64_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);
> };
I think it would be a good idea to move this chunk in the previous patch.
> 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);
> + } else {
> + *obj = value;
> + }
> +}
> +
> void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error
> **errp)
> {
> - uint64_t value;
> -
> - if (v->type_uint8) {
> - v->type_uint8(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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");
> - return;
> - }
> - *obj = value;
> - }
> + uint64_t value = *obj;
> + visit_type_uintN(v, &value, name, UINT8_MAX, "uint8_t", errp);
> + *obj = value;
> }
>
> -void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error
> **errp)
> +void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name,
> + Error **errp)
> {
> - uint64_t value;
> -
> - if (v->type_uint16) {
> - v->type_uint16(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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");
> - return;
> - }
> - *obj = value;
> - }
> + uint64_t value = *obj;
> + visit_type_uintN(v, &value, name, UINT16_MAX, "uint16_t", errp);
> + *obj = value;
> }
>
> -void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error
> **errp)
> +void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name,
> + Error **errp)
> {
> - uint64_t value;
> -
> - if (v->type_uint32) {
> - v->type_uint32(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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");
> - return;
> - }
> - *obj = value;
> - }
> + uint64_t value = *obj;
> + visit_type_uintN(v, &value, name, UINT32_MAX, "uint32_t", errp);
> + *obj = value;
> }
>
> -void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error
> **errp)
> +void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> + Error **errp)
> {
> v->type_uint64(v, obj, name, errp);
> }
>
> +static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
> + int64_t min, int64_t max, const char *type,
> + Error **errp)
> +{
> + Error *err = NULL;
> + int64_t value = *obj;
> +
> + v->type_int64(v, &value, name, &err);
> + if (err) {
> + error_propagate(errp, err);
> + } else if (value < min || value > max) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> + name ? name : "null", type);
> + } else {
> + *obj = value;
> + }
> +}
> +
> void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
> {
> - int64_t value;
> -
> - if (v->type_int8) {
> - v->type_int8(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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;
> - }
> - *obj = value;
> - }
> + int64_t value = *obj;
> + visit_type_intN(v, &value, name, INT8_MIN, INT8_MAX, "int8_t", errp);
> + *obj = value;
> }
>
> void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error
> **errp)
> {
> - int64_t value;
> -
> - if (v->type_int16) {
> - v->type_int16(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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;
> - }
> - *obj = value;
> - }
> + int64_t value = *obj;
> + visit_type_intN(v, &value, name, INT16_MIN, INT16_MAX, "int16_t", errp);
> + *obj = value;
> }
>
> void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error
> **errp)
> {
> - int64_t value;
> -
> - if (v->type_int32) {
> - v->type_int32(v, obj, name, errp);
> - } else {
> - value = *obj;
> - 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;
> - }
> - *obj = value;
> - }
> + int64_t value = *obj;
> + visit_type_intN(v, &value, name, INT32_MIN, INT32_MAX, "int32_t", errp);
> + *obj = value;
> }
>
> void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error
> **errp)
> --
> 2.4.3
>
>
Looks good otherwise.
Reviewed-by: Marc-André Lureau <address@hidden>
--
Marc-André Lureau
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small integer callbacks,
Marc-André Lureau <=