GNOME Bugzilla – Bug 602709
Structs in arrays are not marshalled correctly
Last modified: 2009-11-27 12:49:08 UTC
Patch with test attached.
Created attachment 148307 [details] [review] Structs in arrays are not marshalled correctly
Review of attachment 148307 [details] [review]: ::: gi/pygi-argument.c @@ +1495,3 @@ PyObject *py_item; + void *pointer = g_malloc(item_size); + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { This should be freed at some point ::: tests/libtestgi.c @@ +1841,3 @@ + * @structs: (out) (array length=length) (transfer none): + * test_gi_array_out_struct: transfer is wrong here, should be at least container
Created attachment 148399 [details] [review] Structs in arrays are not marshalled correctly
(In reply to comment #3) > Review of attachment 148307 [details] [review]: > > ::: gi/pygi-argument.c > @@ +1495,3 @@ > PyObject *py_item; > + void *pointer = g_malloc(item_size); > + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { > > This should be freed at some point Removed the malloc, not sure what it was doing there. > ::: tests/libtestgi.c > @@ +1841,3 @@ > > + * @structs: (out) (array length=length) (transfer none): > + * test_gi_array_out_struct: > > transfer is wrong here, should be at least container Would transfer-container match the implementation? I understood that transfer-container would be appropriate if we were creating a new container at every invocation.
Review of attachment 148399 [details] [review]: ::: gi/pygi-argument.c @@ +1512,3 @@ PyObject *py_item; + item.v_pointer = &_g_array_index(array, GArgument, i); + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { This should only be done for structures; it'd break anything else. @@ +1514,3 @@ + } else { + item.v_pointer = &_g_array_index(array, GArgument, i); + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { You could keep the old statement. It's probably worth investigating for multi-dimensional arrays, though. ::: tests/libtestgi.h @@ +358,2 @@ void test_gi_array_out (gint **ints, gint *length); +void test_gi_array_out_struct (TestGISimpleStruct **structs, gint *length); Use a fixed-length array instead; it's simpler.
Review of attachment 148399 [details] [review]: ::: gi/pygi-argument.c @@ +1512,3 @@ + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { + item.v_pointer = &_g_array_index(array, GArgument, i); Done. @@ +1514,3 @@ + item.v_pointer = &_g_array_index(array, GArgument, i); + } else { + memcpy(&item, &_g_array_index(array, GArgument, i), item_size); You mean item = _g_array_index(array, GArgument,...? The problem is that for item sizes smaller than 4, we'll be accessing more than we should, causing a valgrind warning. I know it should be harmless, but hides real problems we may have. ::: tests/libtestgi.h @@ +357,3 @@ void test_gi_array_out (gint **ints, gint *length); +void test_gi_array_out_struct (TestGISimpleStruct **structs, gint *length); Done.
Created attachment 148518 [details] [review] Structs in arrays are not marshalled correctly
(In reply to comment #7) > Review of attachment 148399 [details] [review]: > > ::: gi/pygi-argument.c > @@ +1512,3 @@ > > + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { > + item.v_pointer = &_g_array_index(array, GArgument, i); > > Done. > > @@ +1514,3 @@ > + item.v_pointer = &_g_array_index(array, GArgument, i); > + } else { > + memcpy(&item, &_g_array_index(array, GArgument, i), > item_size); > > You mean item = _g_array_index(array, GArgument,...? > > The problem is that for item sizes smaller than 4, we'll be accessing more than > we should, causing a valgrind warning. I know it should be harmless, but hides > real problems we may have. You're right.
Review of attachment 148518 [details] [review]: ::: gi/pygi-argument.c @@ +1513,3 @@ + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { + + gboolean is_struct = FALSE; Should be freed!
(In reply to comment #10) > Review of attachment 148518 [details] [review]: > > ::: gi/pygi-argument.c > @@ +1513,3 @@ > + if (item_type_tag == GI_TYPE_TAG_INTERFACE) { > + > + gboolean is_struct = FALSE; > > Should be freed! What should be freed?
(In reply to comment #11) > (In reply to comment #10) > What should be freed? iface_info
Created attachment 148583 [details] [review] Structs in arrays are not marshalled correctly
Review of attachment 148583 [details] [review]: Good. Thanks!
The following fix has been pushed: ac80e64 Structs in arrays are not marshalled correctly
Created attachment 148589 [details] [review] Structs in arrays are not marshalled correctly