Re: [PATCH 03/10] hfp_hf_bluez5: Register modem for HFP AG capable devices

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Vinicius,

On 01/22/2013 03:43 PM, Vinicius Costa Gomes wrote:
> Now that we are able to keep track of devices appearing and
> disappearing from BlueZ, we are able to register the modem when a
> device that supports the HFP AG UUID appears.
> ---
>   plugins/bluez5.h        |  1 +
>   plugins/hfp_hf_bluez5.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/bluez5.h b/plugins/bluez5.h
> index e232fee..dbb1d8d 100644
> --- a/plugins/bluez5.h
> +++ b/plugins/bluez5.h
> @@ -27,6 +27,7 @@
>   #define BLUEZ_ERROR_INTERFACE		BLUEZ_SERVICE ".Error"
>
>   #define HFP_HS_UUID	"0000111e-0000-1000-8000-00805f9b34fb"
> +#define HFP_AG_UUID	"0000111f-0000-1000-8000-00805f9b34fb"
>
>   int bluetooth_register_profile(DBusConnection *conn, const char *uuid,
>   					const char *name, const char *object);
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 2daa4c9..35b9eb9 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -42,9 +42,37 @@
>
>   #define HFP_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_hf"
>
> +static GHashTable *modem_hash = NULL;
>   static GHashTable *devices_proxies = NULL;
>   static GDBusClient *bluez = NULL;
>
> +static struct ofono_modem *modem_register(const char *device,
> +						const char *alias)
> +{
> +	struct ofono_modem *modem;
> +	char *path;
> +
> +	modem = g_hash_table_lookup(modem_hash, device);
> +	if (modem != NULL)
> +		return modem;
> +
> +	path = g_strconcat("hfp", device, NULL);
> +
> +	modem = ofono_modem_create(path, "hfp");
> +
> +	g_free(path);
> +
> +	if (modem == NULL)
> +		return NULL;
> +
> +	ofono_modem_set_name(modem, alias);
> +	ofono_modem_register(modem);
> +
> +	g_hash_table_insert(modem_hash, g_strdup(device), modem);
> +
> +	return modem;
> +}
> +
>   static int hfp_probe(struct ofono_modem *modem)
>   {
>   	DBG("modem: %p", modem);
> @@ -171,9 +199,32 @@ static void disconnect_handler(DBusConnection *conn, void *user_data)
>   						BLUEZ_PROFILE_INTERFACE);
>   }
>
> +static void parse_uuids(DBusMessageIter *array, gpointer user_data)
> +{
> +	GSList **uuids = user_data;
> +	DBusMessageIter value;
> +
> +	if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
> +		return;
> +
> +	dbus_message_iter_recurse(array,&value);
> +
> +	while (dbus_message_iter_get_arg_type(&value) == DBUS_TYPE_STRING) {
> +		const char *uuid;
> +
> +		dbus_message_iter_get_basic(&value,&uuid);
> +
> +		*uuids = g_slist_prepend(*uuids, g_strdup(uuid));
> +
> +		dbus_message_iter_next(&value);

Okay, so, we build a list of strings, g_strdup()ing as we go along..

> +	}
> +}
> +
>   static void proxy_added(GDBusProxy *proxy, void *user_data)
>   {
> -	const char *interface, *path;
> +	const char *interface, *path, *alias;
> +	DBusMessageIter iter;
> +	GSList *l, *uuids = NULL;
>
>   	interface = g_dbus_proxy_get_interface(proxy);
>   	path = g_dbus_proxy_get_path(proxy);
> @@ -184,11 +235,36 @@ static void proxy_added(GDBusProxy *proxy, void *user_data)
>   	g_hash_table_insert(devices_proxies, g_strdup(path),
>   						g_dbus_proxy_ref(proxy));
>   	DBG("Device proxy: %s(%p)", path, proxy);
> +
> +	if (g_dbus_proxy_get_property(proxy, "UUIDs",&iter) == FALSE)
> +		return;
> +
> +	parse_uuids(&iter,&uuids);
> +
> +	for (l = uuids; l; l = l->next) {
> +		const char *uuid = l->data;
> +

Next we traverse the list

> +		if (g_str_equal(uuid, HFP_AG_UUID) == TRUE)
> +			break;

And if we find the UUID we break

> +	}
> +
> +	g_slist_free_full(uuids, g_free);
> +

And now we free the all the g_strdup()ed strings.  Why? Why do we bother 
doing all this work?  Just rename parse_uuids into something sensible, 
like 'has_hfp_ag_uuid' and make it return a boolean.

> +	if (l == NULL)
> +		return;
> +
> +	if (g_dbus_proxy_get_property(proxy, "Alias",&iter) == FALSE)
> +		return;
> +
> +	dbus_message_iter_get_basic(&iter,&alias);
> +
> +	modem_register(path, alias);

And why do we do all this work without first checking whether the modem 
is already in the modem_hash?

>   }
>
>   static void proxy_removed(GDBusProxy *proxy, void *user_data)
>   {
>   	const char *interface, *path;
> +	struct ofono_modem *modem;
>
>   	interface = g_dbus_proxy_get_interface(proxy);
>   	path = g_dbus_proxy_get_path(proxy);
> @@ -198,6 +274,12 @@ static void proxy_removed(GDBusProxy *proxy, void *user_data)
>   		DBG("Device proxy: %s(%p)", path, proxy);
>   	}
>
> +	modem = g_hash_table_lookup(modem_hash, path);
> +	if (modem == NULL)
> +		return;
> +
> +	g_hash_table_remove(modem_hash, path);
> +	ofono_modem_remove(modem);
>   }
>
>   static void property_changed(GDBusProxy *proxy, const char *name,
> @@ -242,6 +324,9 @@ static int hfp_init(void)
>   		return err;
>   	}
>
> +	modem_hash = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +								NULL);
> +
>   	devices_proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
>   				g_free, (GDestroyNotify) g_dbus_proxy_unref);
>
> @@ -258,6 +343,7 @@ static void hfp_exit(void)
>   	ofono_modem_driver_unregister(&hfp_driver);
>   	g_dbus_client_unref(bluez);
>
> +	g_hash_table_destroy(modem_hash);
>   	g_hash_table_destroy(devices_proxies);
>   }
>

Regards,
-Denis