From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57624) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UgpCk-0005hd-7W for qemu-devel@nongnu.org; Mon, 27 May 2013 00:38:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UgpCj-0003pt-3P for qemu-devel@nongnu.org; Mon, 27 May 2013 00:38:42 -0400 Received: from v220110690675601.yourvserver.net ([78.47.199.172]:35965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UgpCi-0003pn-PL for qemu-devel@nongnu.org; Mon, 27 May 2013 00:38:41 -0400 Message-ID: <51A2E34B.6010301@weilnetz.de> Date: Mon, 27 May 2013 06:38:35 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1369624858-26983-1-git-send-email-mdroth@linux.vnet.ibm.com> In-Reply-To: <1369624858-26983-1-git-send-email-mdroth@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qapi: pad GenericList value fields to 64 bits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: blauwirbel@gmail.com, qemu-devel@nongnu.org, lcapitulino@redhat.com 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 > --- > 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).