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 701446 - Fix callback annotation in gupnp_service_proxy_add_notify
Fix callback annotation in gupnp_service_proxy_add_notify
Status: RESOLVED FIXED
Product: GUPnP
Classification: Other
Component: gupnp
0.20.x
Other Linux
: Normal normal
: ---
Assigned To: Jens Georg
GUPnP Maintainers
: 730410 (view as bug list)
Depends on: 730690
Blocks:
 
 
Reported: 2013-06-01 21:39 UTC by Andrzej Bieniek
Modified: 2014-05-24 16:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix callback annotation in gupnp_service_proxy_add_notify (1.37 KB, patch)
2013-06-01 21:40 UTC, Andrzej Bieniek
needs-work Details | Review
Add _add_notify_full variant with a GDestroyNotify (5.64 KB, patch)
2013-11-30 15:05 UTC, Jens Georg
needs-work Details | Review
Add _add_notify_full variant with a GDestroyNotify (5.96 KB, patch)
2014-05-24 16:09 UTC, Jens Georg
committed Details | Review

Description Andrzej Bieniek 2013-06-01 21:39:48 UTC
Second call to the callback was causing the segfault.
Changing annotation from async to notified fixed the problem.
(tested with python)

From https://live.gnome.org/GObjectIntrospection/Annotations
* async - Only valid for the duration of the first callback invokation. Can only be called once.
* notified - valid until the GDestroyNotify argument is called. Can be called multiple times before the GDestroyNotify is called.


Patch attached.
Comment 1 Andrzej Bieniek 2013-06-01 21:40:42 UTC
Created attachment 245840 [details] [review]
Fix callback annotation in gupnp_service_proxy_add_notify
Comment 2 Jens Georg 2013-07-30 07:30:08 UTC
Review of attachment 245840 [details] [review]:

Sorry that it took so long to review this, somehow went under the radar

::: libgupnp/gupnp-service-proxy.c
@@ -1545,3 +1545,3 @@
  * @variable: The variable to add notification for
  * @type: The type of the variable
- * @callback: (scope async): The callback to call when @variable changes
+ * @callback: (scope notified): The callback to call when @variable changes

From the description in the linked wiki-page, wouldn't "call" be more correct? Since we don't have a GDestroyNotify here
Comment 3 Andrzej Bieniek 2013-09-05 21:57:05 UTC
Thanks, it looks like "notified" is a closest but not fully correct.
With "call", callback would be valid only for a duration of the gupnp_service_proxy_add_notify function call.

The correct solution would be to create new gupnp_service_proxy_add_notify_gi function with GDestroyNotify, used by introspection. Should I go ahead and propose the patch?
Comment 4 Jens Georg 2013-11-30 15:05:53 UTC
Created attachment 263204 [details] [review]
Add _add_notify_full variant with a GDestroyNotify

This is to help introspection with the call-back.
Comment 5 Andrzej Bieniek 2013-12-19 22:34:39 UTC
Review of attachment 263204 [details] [review]:

Thanks for the patch, I've tested it with gupnp 0.20.4 - it runs fine.

On service proxy finalize callback notify should also be called.
In our case gupnp_service_proxy_finalize() invokes notify_data_free(),
notify_data_free() should call callback notify before freeing it.

::: libgupnp/gupnp-service-proxy.c
@@ +1592,3 @@
+                                                    callback,
+                                                    NULL,
+                                                    user_data);

Parameters order should be ([..] callback, user_data, NULL)
Comment 6 Jens Georg 2013-12-20 07:51:12 UTC
Good catch, thanks
Comment 7 Jens Georg 2014-05-20 06:38:11 UTC
*** Bug 730410 has been marked as a duplicate of this bug. ***
Comment 8 Jens Georg 2014-05-24 16:09:13 UTC
Created attachment 277118 [details] [review]
Add _add_notify_full variant with a GDestroyNotify

This is to help introspection with the call-back.
Comment 9 Jens Georg 2014-05-24 16:31:06 UTC
Attachment 277118 [details] pushed as 9ffedf4 - Add _add_notify_full variant with a GDestroyNotify