After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702508 - Issue when marshaling C arrays
Issue when marshaling C arrays
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
3.8.x
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on: 727004
Blocks:
 
 
Reported: 2013-06-17 19:48 UTC by Anil Gunturu
Modified: 2014-08-08 01:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for issues with passing c arrays (2.53 KB, patch)
2013-08-05 14:34 UTC, Anil Gunturu
reviewed Details | Review
tests: Add regression test for callbacks with an inout array (12.87 KB, patch)
2014-08-07 16:27 UTC, Garrett Regier
committed Details | Review
Add test for a callback with an inout array (1.63 KB, patch)
2014-08-07 16:30 UTC, Garrett Regier
committed Details | Review

Description Anil Gunturu 2013-06-17 19:48:14 UTC
Passing a C array to python using the gobject-introspection seem to have couple of issues when using pygobject-3.8.2.

Everything works fine when the array is passed as an "in" parameter. But when it is passed as either "inout" or "out" there are couple of issues with marshaling parameters in c->python as well as python->c.

1. When the length parameter is passed as inout, the length value is not read correctly (in this case the length is set to the pointer)

2. In the python->c there are again two issues:
a) A GArray is being returned when expecting C array
b) The contents in the array are encapsulated as GIArgument, instead of the original data type of the array.

The following patch addresses both issues described above:
diff --git a/pygobject/pygobject-3.8.2/gi/pygi-argument.c b/pygobject/pygobject-3.8.2/gi/pygi-argument.c
index 7de90a2..a5b7fc5 100644
--- a/pygobject/pygobject-3.8.2/gi/pygi-argument.c
+++ b/pygobject/pygobject-3.8.2/gi/pygi-argument.c
@@ -835,7 +835,12 @@ _pygi_argument_to_array (GIArgument  *arg,
                     g_arg_info_load_type (&length_arg_info, &length_type_info);
 
                     if (args != NULL) {
-                        if (!gi_argument_to_gssize (args[length_arg_pos],
+                        GIArgument *p_length_arg = args[length_arg_pos];
+                        if (g_arg_info_get_direction (&length_arg_info) != GI_DIRECTION_IN) {
+                            p_length_arg = (GIArgument *)p_length_arg->v_pointer;
+                        }
+                        
+                        if (!gi_argument_to_gssize (p_length_arg,
                                                     g_type_info_get_tag (&length_type_info),
                                                     &length))
                             return NULL;
@@ -1172,7 +1177,8 @@ _pygi_argument_from_object (PyObject   *object,
             if (g_type_info_get_tag (item_type_info) == GI_TYPE_TAG_UINT8)
                item_size = 1;
             else
-               item_size = sizeof (GIArgument);
+               item_size = _pygi_g_type_info_size (item_type_info);
+            // item_size = sizeof (GIArgument);
 
             array = g_array_sized_new (is_zero_terminated, FALSE, item_size, length);
             if (array == NULL) {
diff --git a/pygobject/pygobject-3.8.2/gi/pygi-closure.c b/pygobject/pygobject-3.8.2/gi/pygi-closure.c
index f30fa52..03e4e87 100644
--- a/pygobject/pygobject-3.8.2/gi/pygi-closure.c
+++ b/pygobject/pygobject-3.8.2/gi/pygi-closure.c
@@ -193,8 +193,14 @@ _pygi_closure_assign_pyobj_to_out_argument (gpointer out_arg, PyObject *object,
         }
 
         default:
-           *((gpointer *) out_arg) = arg.v_pointer;
-           break;
+            if (g_type_info_get_array_type (type_info) == GI_ARRAY_TYPE_C) {
+                GArray *temp = (GArray *) arg.v_pointer;
+                *((gpointer *) out_arg) = temp->data;
+                g_array_free(temp, FALSE);
+            } else {
+                *((gpointer *) out_arg) = arg.v_pointer;
+            }
+            break;
       }
 }
Comment 1 Simon Feltman 2013-07-19 02:20:45 UTC
Hi,
Can you attach this as a .patch file as it will be easier to review? unittests will also help getting this pushed through.

See: https://wiki.gnome.org/GnomeLove/SubmittingPatches

Thanks
Comment 2 Anil Gunturu 2013-08-05 14:34:51 UTC
Created attachment 250884 [details] [review]
Patch for issues with passing c arrays
Comment 3 Anil Gunturu 2013-08-05 14:36:21 UTC
Submitted the requested patch.
Comment 4 Simon Feltman 2013-08-14 20:43:55 UTC
This looks like it might also be the same as bug #669496. I will test your patch against the tests in that ticket and see if things are fixed.
Comment 5 Simon Feltman 2014-08-05 20:12:51 UTC
Review of attachment 250884 [details] [review]:

Sorry for not reviewing this sooner. At this point the code path in question is going away as it's being unified with regular function call marshalling. This effort should also fix the bug reported here.

::: gi/pygi-closure.c
@@ +208,2 @@
         default:
+            if (g_type_info_get_array_type (type_info) == GI_ARRAY_TYPE_C) {

This should be moved into a separate case for GI_TYPE_TAG_ARRAY.
Comment 6 Garrett Regier 2014-08-07 16:27:23 UTC
Created attachment 282835 [details] [review]
tests: Add regression test for callbacks with an inout array

tests: Add regression test for callbacks with an inout array
Comment 7 Garrett Regier 2014-08-07 16:30:00 UTC
Created attachment 282836 [details] [review]
Add test for a callback with an inout array

This was broken until the closures used the caches for marshaling.


NOTE: This only works when the closures have been fully converted to using the caches for marshaling.

https://bugzilla.gnome.org/show_bug.cgi?id=727004
Comment 8 Simon Feltman 2014-08-07 17:26:55 UTC
Review of attachment 282835 [details] [review]:

Looks good. Normally I see the bugzilla URL as the last line of commit messages. See: https://wiki.gnome.org/GnomeLove/SubmittingPatches
Comment 9 Simon Feltman 2014-08-07 17:32:55 UTC
Review of attachment 282836 [details] [review]:

LGTM. Same comment about bugzilla URL. Please also commit with a decorator so our test suite still works with older versions of GI:

@unittest.skipUnless(hasattr(Everything, 'test_array_inout_callback'),
                     'Requires newer version of GI')
def test_callback_scope_call_array_inout(self):
    ...

Additionally if this is submitted before patches that fix it, add the @unittest.expectedFailure decorator, then remove that in the patch that fixes it.
Comment 10 Simon Feltman 2014-08-08 01:24:27 UTC
Closing now that relevant patches in bug 727004 have landed.