Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits

[Stefan Weil]

Am 27.05.2013 05:20, schrieb Michael Roth:
> With the introduction of native list types, we now have types such as
> int64List where the 'value' field is not a pointer, but the actual
> 64-bit value.
>
> On 32-bit architectures, this can lead to situations where 'next' field
> offset in GenericList does not correspond to the 'next' field in the
> types that we cast to GenericList when using the visit_next_list()
> interface, causing issues when we attempt to traverse linked list
> structures of these types.
>
> To fix this, pad the 'value' field of GenericList and other
> schema-defined/native *List types out to 64-bits.
>
> This is less memory-efficient for 32-bit architectures, but allows us to
> continue to rely on list-handling interfaces that target GenericList to
> simply visitor implementations.
>
> In the future we can improve efficiency by defaulting to using native C
> array backends to handle list of non-pointer types, which would be more
> memory efficient in itself and allow us to roll back this change.
>
> Signed-off-by: Michael Roth <address@hidden>
> ---
>  include/qapi/visitor.h          |    5 ++++-
>  scripts/qapi-types.py           |   10 ++++++++--
>  tests/test-qmp-output-visitor.c |    5 ++++-
>  3 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 1fef18c..28c21d8 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -18,7 +18,10 @@
>  
>  typedef struct GenericList
>  {
> -    void *value;
> +    union {
> +        void *value;
> +        uint64_t padding;
> +    };
>      struct GenericList *next;
>  } GenericList;
>  
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index fd42d71..ddcfed9 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -22,7 +22,10 @@ def generate_fwd_struct(name, members, builtin_type=False):
>  
>  typedef struct %(name)sList
>  {
> -    %(type)s value;
> +    union {
> +        %(type)s value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> @@ -35,7 +38,10 @@ typedef struct %(name)s %(name)s;
>  
>  typedef struct %(name)sList
>  {
> -    %(name)s *value;
> +    union {
> +        %(name)s *value;
> +        uint64_t padding;
> +    };
>      struct %(name)sList *next;
>  } %(name)sList;
>  ''',
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 0942a41..b2fa9a7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -295,7 +295,10 @@ static void 
> test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  
>  typedef struct TestStructList
>  {
> -    TestStruct *value;
> +    union {
> +        TestStruct *value;
> +        uint64_t padding;
> +    };
>      struct TestStructList *next;
>  } TestStructList;
>  

Looks good. Would reordering of value, next work, too
(without memory overhead for 32 bit systems)?

 typedef struct GenericList
 {
    struct GenericList *next;
    void *value;
 } GenericList;

 typedef struct %(name)sList
 {
    struct %(name)sList *next;
    %(type)s value;
 } %(name)sList;


...

It looks like memory allocation (g_malloc0) for GenericList
was also wrong in the old code (this is fixed with your patch).