Re: [Qemu-devel] [PATCH v9 26/27] qapi: Remove dead visitor code

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Commit cbc95538 removed unused start_handle() and end_handle(),
> but forgot got remove their declarations.
>
> Commit 4e27e819 introduced optional visitor callbacks for all
> sorts of int types, but except for type_uint64 and type_size,
> none of them have ever been supplied (the generic implementation
> based on using type_int then 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.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: no change
> v8: no change
> v7: no change
> v6: no change
> ---
>  include/qapi/visitor-impl.h |  19 +++----
>  include/qapi/visitor.h      |   3 -
>  qapi/qapi-visit-core.c      | 131 
> +++++++++++++++++---------------------------
>  3 files changed, 58 insertions(+), 95 deletions(-)

Easier to review with whitespace change ignored:

| diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
| index 1d09b7b..370935a 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>
|   *
| @@ -48,18 +48,15 @@ struct Visitor
|      void (*optional)(Visitor *v, bool *present, const char *name,
|                       Error **errp);
|  
| -    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);
| +
| +    /* Only required to visit uint64 differently than (*type_int)().  */

If you don't supply it, what happens for uint64_t values that aren't
representable as int64_t?

| +    void (*type_uint64)(Visitor *v, uint64_t *obj, const char *name,
| +                        Error **errp);
| +    /* Only required to visit sizes differently than (*type_uint64)().  */
| +    void (*type_size)(Visitor *v, uint64_t *obj, const char *name,
| +                      Error **errp);
|  };
|  
|  void input_type_enum(Visitor *v, int *obj, const char * const strings[],
| diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
| index baea594..67ddd83 100644
| --- a/include/qapi/visitor.h
| +++ b/include/qapi/visitor.h
| @@ -27,9 +27,6 @@ typedef struct GenericList
|      struct GenericList *next;
|  } GenericList;
|  
| -void visit_start_handle(Visitor *v, void **obj, const char *kind,
| -                        const char *name, Error **errp);
| -void visit_end_handle(Visitor *v, Error **errp);
|  void visit_start_struct(Visitor *v, void **obj, const char *kind,
|                          const char *name, size_t size, Error **errp);
|  void visit_end_struct(Visitor *v, Error **errp);
| diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
| index 884fe94..cbf7780 100644
| --- a/qapi/qapi-visit-core.c
| +++ b/qapi/qapi-visit-core.c
| @@ -104,9 +104,6 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const 
char *name, Error **errp)
|  {
|      int64_t value;
|  
| -    if (v->type_uint8) {
| -        v->type_uint8(v, obj, name, errp);
| -    } else {
|      value = *obj;
|      v->type_int(v, &value, name, errp);
|      if (value < 0 || value > UINT8_MAX) {
| @@ -116,15 +113,12 @@ void visit_type_uint8(Visitor *v, uint8_t *obj, const 
char *name, Error **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)
|  {
|      int64_t value;
|  
| -    if (v->type_uint16) {
| -        v->type_uint16(v, obj, name, errp);
| -    } else {
|      value = *obj;
|      v->type_int(v, &value, name, errp);
|      if (value < 0 || value > UINT16_MAX) {
| @@ -134,15 +128,12 @@ void visit_type_uint16(Visitor *v, uint16_t *obj, const 
char *name, Error **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)
|  {
|      int64_t value;
|  
| -    if (v->type_uint32) {
| -        v->type_uint32(v, obj, name, errp);
| -    } else {
|      value = *obj;
|      v->type_int(v, &value, name, errp);
|      if (value < 0 || value > UINT32_MAX) {
| @@ -152,9 +143,9 @@ void visit_type_uint32(Visitor *v, uint32_t *obj, const 
char *name, Error **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)
|  {
|      int64_t value;
|  
       if (v->type_uint64) {
           v->type_uint64(v, obj, name, errp);
       } else {
           value = *obj;
           v->type_int(v, &value, name, errp);
           *obj = value;
       }
   }

Answering my "what happens" question:

* Input visitor

  If your type_int() accepts large positive input values and casts them
  to the corresponding large negative value, *obj = value will cast them
  right back, and the sloppiness cancels out.

  If it rejects them, they stay rejected.

* Output visitor

  You'll output large positive values as the corresponding large
  negative value.

I doubt not defining this makes much sense.  Do we have such visitors?

| @@ -171,9 +162,6 @@ 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_int(v, &value, name, errp);
|      if (value < INT8_MIN || value > INT8_MAX) {
| @@ -183,15 +171,11 @@ void visit_type_int8(Visitor *v, int8_t *obj, const 
char *name, Error **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_int(v, &value, name, errp);
|      if (value < INT16_MIN || value > INT16_MAX) {
| @@ -201,15 +185,11 @@ void visit_type_int16(Visitor *v, int16_t *obj, const 
char *name, Error **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_int(v, &value, name, errp);
|      if (value < INT32_MIN || value > INT32_MAX) {
| @@ -219,29 +199,18 @@ void visit_type_int32(Visitor *v, int32_t *obj, const 
char *name, Error **errp)
|      }
|      *obj = value;
|  }
| -}
|  
|  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);
|  }
| -}
|  
|  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_int(v, &value, name, errp);
| -        *obj = value;
| +        visit_type_uint64(v, obj, name, errp);
|      }
|  }
|  

This one's problematic, I think.  If you supply type_uint64(), but not
type_size(), I'd certainly expect type_uint64() to be used.  If not,
what happens for values that can be represented as uint64_t, but not as
int64_t?