GNOME Bugzilla – Bug 770318
various patches for connman-manager
Last modified: 2016-10-15 10:13:52 UTC
We are using libgupnp with connman on the Raumfeld devices and we had to do some changes to get connman-manager working correctly. I am attaching the patches here for review in the hope that you will find them useful.
Created attachment 334054 [details] [review] connman: delay context creation by 1000 milliseconds
Created attachment 334056 [details] [review] connman-manager: Fix inverse g_strcmp0 checks
Created attachment 334057 [details] [review] connman: fix a potential crash
Created attachment 334058 [details] [review] connman: cancel a pending g_dbus_proxy_new_for_bus() call
Created attachment 334059 [details] [review] connman: make the GetServices call cancellable
Created attachment 334060 [details] [review] connman: fix a race on the DBus interface to connman
I don't expect the connman plugin to be that well-exercised, tbh. Thanks for sharing, I will have a look
(In reply to Sven Neumann from comment #1) > Created attachment 334054 [details] [review] [review] > connman: delay context creation by 1000 milliseconds Did you already file an upstream issue for that which we can eventually link to? Could "online" be a more appropriate state?
Review of attachment 334056 [details] [review]: You can set the interface, I'm just not sure what its implications are. I think it will just be ignored.
Review of attachment 334057 [details] [review]: +1
Review of attachment 334058 [details] [review]: ::: libgupnp/gupnp-connman-manager.c @@ +289,3 @@ + if (cm_service->cancellable != NULL) { + g_object_unref (cm_service->cancellable); + cm_service->cancellable = NULL; Can't we just unconditonally cancel the cancellable here? Would that cause an issue? The two ifs look odd.
Review of attachment 334059 [details] [review]: +1
Review of attachment 334060 [details] [review]: ::: libgupnp/gupnp-connman-manager.c @@ +218,1 @@ g_object_set (G_OBJECT (cm_service->context), This seems unrelated. @@ +230,1 @@ service_context_delete (cm_service); Same as above. Unrelated change @@ +300,2 @@ + if (cm_service->cancellable != NULL) { + g_cancellable_cancel (cm_service->cancellable); Ah. That's what I was wondering about in patch 4 - can put this change in that patch?
(In reply to Jens Georg from comment #8) > (In reply to Sven Neumann from comment #1) > > Created attachment 334054 [details] [review] [review] [review] > > connman: delay context creation by 1000 milliseconds > > Did you already file an upstream issue for that which we can eventually link > to? > Could "online" be a more appropriate state? No, we didn't file an upstream issue. "Online" would work better in most cases, but there are cases where the device has enough connectivity to do UPnP in a local network but it is still not "Online" as being "Online" means that internet services can be reached. So, I don't think that "Online" is appropriate.
Ah, it's that kind of online, pity. I would be feeling better to merge that work-around if we had some kind of connman ticket we could refer to
Attachment 334054 [details] pushed as 48ce642 - connman: delay context creation by 1000 milliseconds Attachment 334057 [details] pushed as a1ad610 - connman: fix a potential crash Attachment 334058 [details] pushed as 9385ccf - connman: cancel a pending g_dbus_proxy_new_for_bus() call Attachment 334059 [details] pushed as 4a7d434 - connman: make the GetServices call cancellable Attachment 334060 [details] pushed as a8c8de2 - connman: fix a race on the DBus interface to connman