[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits
From: |
mdroth |
Subject: |
Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits |
Date: |
Wed, 29 May 2013 13:12:18 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, May 29, 2013 at 01:32:52PM -0400, Luiz Capitulino wrote:
> On Sun, 26 May 2013 22:20:58 -0500
> Michael Roth <address@hidden> wrote:
>
> > 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.
>
> I'm also concerned with the small complexity this change is adding.
> How hard would it be to do the proper solution with arrays instead?
It's not *too* bad, we'd need patches 9-11 from here:
http://lists.gnu.org/archive/html/qemu-devel/2012-10/threads.html#05755
Along with code generation bits, and then all the unit test stuff.
I think we should be able to get it in for 1.6, but I'd rather not leave
32-bit busted in the meantime.
>
> >
> > 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;
> >
>
>