Re: [PATCH 01/10] hfp_hf_bluez5: Initial GDBusClient for BlueZ

[Vinicius Costa Gomes <vinicius.gomes@openbossa.org>]

[-- Attachment #1: Type: text/plain, Size: 4248 bytes --]

Hi Denis,

On 22:30 Tue 22 Jan, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 01/22/2013 03:43 PM, Vinicius Costa Gomes wrote:
> >This patch adds the initial callbacks to track when BlueZ connects or
> >disconnects from the system bus. And tracks the interfaces added or
> >removed.
> >---
> >  plugins/bluez5.h        |  3 ++-
> >  plugins/hfp_hf_bluez5.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 65 insertions(+), 10 deletions(-)
> >

[snip]

> >@@ -143,6 +145,60 @@ static const GDBusMethodTable profile_methods[] = {
> >  	{ }
> >  };
> >
> >+static void connect_handler(DBusConnection *conn, void *user_data)
> >+{
> >+	DBG("Registering External Profile handler ...");
> >+
> >+	if (!g_dbus_register_interface(conn, HFP_EXT_PROFILE_PATH,
> >+					BLUEZ_PROFILE_INTERFACE,
> >+					profile_methods, NULL,
> >+					NULL, NULL, NULL)) {
> 
> We just performed this operation in hfp_init, why are we doing it
> again here?

Most probably it was a copy and paste error because our rebase. Fixed now.

> 
> >+		ofono_error("Register Profile interface failed: %s",
> >+						HFP_EXT_PROFILE_PATH);
> >+	}
> >+
> >+	bluetooth_register_profile(conn, HFP_HS_UUID, "hfp_hf",
> >+						   HFP_EXT_PROFILE_PATH);
> 
> You have a whitespace violation here, mixing tabs & spaces.

Don't know what happened here. Fixed.

> 
> >+}
> >+
> >+static void disconnect_handler(DBusConnection *conn, void *user_data)
> >+{
> >+
> >+	DBG("Unregistering External Profile handler ...");
> >+
> >+	g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> >+						BLUEZ_PROFILE_INTERFACE);
> 
> Why do we bother?  Should we not leave this always registered?

Fixed. This function is unnecessary really. Thanks for pointing this out.

> 
> >+}
> >+
> >+static void proxy_added(GDBusProxy *proxy, void *user_data)
> >+{
> >+	const char *path;
> >+
> >+	path = g_dbus_proxy_get_path(proxy);
> >+
> >+	DBG("Device proxy: %s(%p)", path, proxy);
> >+}
> >+
> >+static void proxy_removed(GDBusProxy *proxy, void *user_data)
> >+{
> >+	const char *path;
> >+
> >+	path = g_dbus_proxy_get_path(proxy);
> >+
> >+	DBG("Device proxy: %s(%p)", path, proxy);
> >+}
> >+
> >+static void property_changed(GDBusProxy *proxy, const char *name,
> >+					DBusMessageIter *iter, void *user_data)
> >+{
> >+	const char *interface, *path;
> >+
> >+	interface = g_dbus_proxy_get_interface(proxy);
> >+	path = g_dbus_proxy_get_path(proxy);
> >+
> >+	DBG("path: %s interface: %s", path, interface);
> >+}
> >+
> >  static int hfp_init(void)
> >  {
> >  	DBusConnection *conn = ofono_dbus_get_connection();
> >@@ -161,19 +217,16 @@ static int hfp_init(void)
> >  		return -EIO;
> >  	}
> >
> >-	err = ofono_modem_driver_register(&hfp_driver);
> >-	if (err<  0) {
> >-		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> >-						BLUEZ_PROFILE_INTERFACE);
> >-		return err;
> >-	}
> >+	bluez = g_dbus_client_new(conn, BLUEZ_SERVICE, BLUEZ_MANAGER_PATH);
> >+	g_dbus_client_set_connect_watch(bluez, connect_handler, NULL);
> >+	g_dbus_client_set_disconnect_watch(bluez, disconnect_handler, NULL);
> >+	g_dbus_client_set_proxy_handlers(bluez, proxy_added, proxy_removed,
> >+						property_changed, NULL);
> 
> Error handling here is all messed up.  ofono_modem_driver_register
> operation might still fail below and you never destroy the 'bluez'
> object in that case.

Fixed.

> 
> >
> >-	err = bluetooth_register_profile(conn, HFP_HS_UUID, "hfp_hf",
> >-						HFP_EXT_PROFILE_PATH);
> >+	err = ofono_modem_driver_register(&hfp_driver);
> >  	if (err<  0) {
> >  		g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> >  						BLUEZ_PROFILE_INTERFACE);
> >-		ofono_modem_driver_unregister(&hfp_driver);
> >  		return err;
> >  	}
> >
> >@@ -188,6 +241,7 @@ static void hfp_exit(void)
> >  	g_dbus_unregister_interface(conn, HFP_EXT_PROFILE_PATH,
> >  						BLUEZ_PROFILE_INTERFACE);
> >  	ofono_modem_driver_unregister(&hfp_driver);
> >+	g_dbus_client_unref(bluez);
> >  }
> >
> >  OFONO_PLUGIN_DEFINE(hfp_bluez5, "External Hands-Free Profile Plugin", VERSION,
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius