Return-Path: Date: Mon, 24 Feb 2014 10:33:44 +0200 From: Johan Hedberg To: Claudio Takahasi Cc: linux-bluetooth@vger.kernel.org, Alvaro Silva Subject: Re: [PATCH BlueZ v7 07/11] gatt: Add external services tracking Message-ID: <20140224083344.GA29475@x220.p-661hnu-f1> References: <1392835893-6723-1-git-send-email-claudio.takahasi@openbossa.org> <1392835893-6723-8-git-send-email-claudio.takahasi@openbossa.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1392835893-6723-8-git-send-email-claudio.takahasi@openbossa.org> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Claudio, On Wed, Feb 19, 2014, Claudio Takahasi wrote: > From: Alvaro Silva > > All primary services declarations provided by an external application > will be automatically inserted in the attribute database. > --- > src/gatt-dbus.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 109 insertions(+) I've applied patches 1-6 but this one seems a bit strange. > +static void proxy_added(GDBusProxy *proxy, void *user_data) > +{ > + struct external_app *eapp = user_data; > + const char *interface, *path; > + > + interface = g_dbus_proxy_get_interface(proxy); > + path = g_dbus_proxy_get_path(proxy); > + > + DBG("path %s iface %s", path, interface); > + > + if (g_strcmp0(interface, SERVICE_IFACE) != 0) > + return; > + > + eapp->proxies = g_slist_append(eapp->proxies, proxy); > +} > + > +static void proxy_removed(GDBusProxy *proxy, void *user_data) > +{ > + struct external_app *eapp = user_data; > + const char *interface, *path; > + > + interface = g_dbus_proxy_get_interface(proxy); > + path = g_dbus_proxy_get_path(proxy); > + > + DBG("path %s iface %s", path, interface); > + > + eapp->proxies = g_slist_remove(eapp->proxies, proxy); > +} > + > + Double empty line above - please fix (not related to my main issue with this patch though) > +static gboolean finish_register(gpointer user_data) > +{ > + struct external_app *eapp = user_data; > + GSList *list; > + > + /* > + * It is not possible to detect when the last proxy object > + * was reported. "Proxy added" handler reports objects > + * added on demand or returned by GetManagedObjects(). > + * This timer helps to register all the GATT declarations > + * (services, characteristics and descriptors) after fetching > + * all the D-Bus objects. > + */ > + > + eapp->register_timer = 0; > + > + for (list = eapp->proxies; list; list = g_slist_next(list)) { > + const char *interface, *path; > + GDBusProxy *proxy = list->data; > + > + interface = g_dbus_proxy_get_interface(proxy); > + path = g_dbus_proxy_get_path(proxy); > + > + if (g_strcmp0(SERVICE_IFACE, interface) != 0) > + continue; > + > + if (g_strcmp0(path, eapp->path) != 0) > + continue; > + > + if (register_external_service(proxy) < 0) { > + DBG("Inconsistent external service: %s", path); > + continue; > + } > + > + DBG("External service: %s", path); > + } > + > + return FALSE; > +} This whole timer thing seems too hackish to me. A better way to fix this might be to have something like g_dbus_client_set_proxies_complete_watch (or some better name) to let the code wait until GetManagedObjects has returned and the initial proxies added. Do you agree? Johan