Re: [PATCH 2/6] bluetooth: Add register service for bluetooth

[Gustavo F. Padovan <gustavo@padovan.org>]

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

Hi Zhenhua,

* Zhenhua Zhang <zhenhua.zhang@intel.com> [2010-07-05 13:45:05 +0800]:

> Add bluetooth_register_service() and bluetooth_unregister_service()
> where bluetooth profiles plugins like DUN GW can register themselves per
> adapter. It shares existing bluetooth framework to listen bluetooth
> events (new adapters, bluetoothd shutdown, etc..)
> ---
>  plugins/bluetooth.c |  125 ++++++++++++++++++++++++++++++++++++++++++---------
>  plugins/bluetooth.h |    8 +++
>  2 files changed, 112 insertions(+), 21 deletions(-)
> 
> diff --git a/plugins/bluetooth.c b/plugins/bluetooth.c
> index 5a85eaa..3a3b5e6 100644
> --- a/plugins/bluetooth.c
> +++ b/plugins/bluetooth.c
> @@ -38,8 +38,19 @@
>  
>  static DBusConnection *connection;
>  static GHashTable *uuid_hash = NULL;
> +static GHashTable *service_hash = NULL;
>  static GHashTable *adapter_address_hash = NULL;
>  
> +static char *bluetooth_service_type_to_str(enum bluetooth_service_type type)
> +{
> +	switch (type) {
> +	case DUN_GATEWAY:
> +		return "DUN";
> +	default:
> +		return "";
> +	}
> +}
> +

We actually don't need this function, you can put the 
'enum bluetooth_service_type' value as hash key. Sounds better.

>  void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
>  				char *buf, int size)
>  {
> @@ -376,6 +387,9 @@ static void adapter_properties_cb(DBusPendingCall *call, gpointer user_data)
>  	GSList *device_list = NULL;
>  	GSList *l;
>  	const char *addr;
> +	GHashTableIter hash_iter;
> +	gpointer key, value;
> +	struct bluetooth_profile *profile;
>  
>  	reply = dbus_pending_call_steal_reply(call);
>  
> @@ -393,6 +407,12 @@ static void adapter_properties_cb(DBusPendingCall *call, gpointer user_data)
>  	g_hash_table_insert(adapter_address_hash,
>  				g_strdup(path), g_strdup(addr));
>  
> +	g_hash_table_iter_init(&hash_iter, service_hash);
> +	while (g_hash_table_iter_next(&hash_iter, &key, &value)) {
> +		profile = value;
> +		profile->create(path, NULL, addr, NULL);

You have to check if profile and profile->create is not NULL. It is more
safe.

> +	}
> +
>  	for (l = device_list; l; l = l->next) {
>  		const char *device = l->data;
>  
> @@ -492,10 +512,13 @@ static void bluetooth_remove_all_modem(gpointer key, gpointer value,
>  
>  static void bluetooth_disconnect(DBusConnection *connection, void *user_data)
>  {
> -	if (!uuid_hash)
> -		return;
> +	if (uuid_hash)
> +		g_hash_table_foreach(uuid_hash, bluetooth_remove_all_modem,
> +					NULL);
>  
> -	g_hash_table_foreach(uuid_hash, bluetooth_remove_all_modem, NULL);
> +	if (service_hash)
> +		g_hash_table_foreach(service_hash, bluetooth_remove_all_modem,
> +					NULL);
>  }
>  
>  static guint bluetooth_watch;
> @@ -503,12 +526,12 @@ static guint adapter_added_watch;
>  static guint adapter_removed_watch;
>  s.tatic guint property_watch;
>  
> -int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile *profile)
> +static int bluetooth_init()
>  {

So I prefer a real refcount structure here, and call this
bluetooth_ref(). What do you think?

>  	int err;
>  
> -	if (uuid_hash)
> -		goto done;
> +	if (adapter_address_hash)
> +		return 0;
>  
>  	connection = ofono_dbus_get_connection();
>  
> @@ -536,19 +559,9 @@ int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile *profile)
>  		goto remove;
>  	}
>  
> -	uuid_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> -						g_free, NULL);
> -
>  	adapter_address_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
>  						g_free, g_free);
>  
> -done:
> -	g_hash_table_insert(uuid_hash, g_strdup(uuid), profile);
> -
> -	bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> -				manager_properties_cb, NULL, NULL, -1,
> -				DBUS_TYPE_INVALID);
> -
>  	return 0;
>  
>  remove:
> @@ -556,14 +569,13 @@ remove:
>  	g_dbus_remove_watch(connection, adapter_added_watch);
>  	g_dbus_remove_watch(connection, adapter_removed_watch);
>  	g_dbus_remove_watch(connection, property_watch);
> +
>  	return err;
>  }
>  
> -void bluetooth_unregister_uuid(const char *uuid)
> +static void bluetooth_exit()

and this one coul be bluetooth_unref()

>  {
> -	g_hash_table_remove(uuid_hash, uuid);
> -
> -	if (g_hash_table_size(uuid_hash))
> +	if (uuid_hash != NULL || service_hash != NULL)
>  		return;
>  
>  	g_dbus_remove_watch(connection, bluetooth_watch);
> @@ -571,9 +583,80 @@ void bluetooth_unregister_uuid(const char *uuid)
>  	g_dbus_remove_watch(connection, adapter_removed_watch);
>  	g_dbus_remove_watch(connection, property_watch);
>  
> -	g_hash_table_destroy(uuid_hash);
>  	g_hash_table_destroy(adapter_address_hash);
> +}
> +
> +
> +int bluetooth_register_uuid(const char *uuid, struct bluetooth_profile *profile)
> +{
> +	int err;
> +
> +	err = bluetooth_init();
> +	if (err)
> +		return err;
> +
> +	if (!uuid_hash)
> +		uuid_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> +							g_free, NULL);
> +
> +	g_hash_table_insert(uuid_hash, g_strdup(uuid), profile);
> +
> +	bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> +				manager_properties_cb, NULL, NULL, -1,
> +				DBUS_TYPE_INVALID);
> +
> +	return 0;
> +}
> +
> +void bluetooth_unregister_uuid(const char *uuid)
> +{
> +	g_hash_table_remove(uuid_hash, uuid);
> +
> +	if (g_hash_table_size(uuid_hash) > 0)
> +		return;
> +
> +	g_hash_table_destroy(uuid_hash);
>  	uuid_hash = NULL;
> +	bluetooth_exit();
> +}
> +
> +int bluetooth_register_service(enum bluetooth_service_type type,
> +					struct bluetooth_profile *profile)
> +{
> +	int err;
> +	char *type_str;
> +
> +	err = bluetooth_init();
> +	if (err)
> +		return err;
> +
> +	type_str = bluetooth_service_type_to_str(type);
> +
> +	if (!service_hash)
> +		service_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> +							g_free, NULL);
> +
> +	g_hash_table_insert(service_hash, g_strdup(type_str), profile);
> +
> +	bluetooth_send_with_reply("/", BLUEZ_MANAGER_INTERFACE, "GetProperties",
> +				manager_properties_cb, NULL, NULL, -1,
> +				DBUS_TYPE_INVALID);
> +
> +	return 0;
> +}
> +
> +void bluetooth_unregister_service(enum bluetooth_service_type type)
> +{
> +	char *type_str = bluetooth_service_type_to_str(type);
> +
> +	g_hash_table_remove(service_hash, type_str);
> +
> +	if (g_hash_table_size(service_hash) > 0)
> +		return;
> +
> +	g_hash_table_destroy(service_hash);
> +	service_hash = NULL;
> +	bluetooth_exit();
>  }
>  
>  OFONO_PLUGIN_DEFINE(bluetooth, "Bluetooth Utils Plugins", VERSION,
> diff --git a/plugins/bluetooth.h b/plugins/bluetooth.h
> index fb0d841..84707a9 100644
> --- a/plugins/bluetooth.h
> +++ b/plugins/bluetooth.h
> @@ -28,6 +28,10 @@
>  /* Profiles bitfield */
>  #define HFP_AG 0x01
>  
> +enum bluetooth_service_type {
> +	DUN_GATEWAY,
> +};
> +
>  struct bluetooth_profile {
>  	const char *name;
>  	int (*create)(const char *device, const char *dev_addr,
> @@ -40,6 +44,10 @@ int bluetooth_register_uuid(const char *uuid,
>  				struct bluetooth_profile *profile);
>  void bluetooth_unregister_uuid(const char *uuid);
>  
> +int bluetooth_register_service(enum bluetooth_service_type type,
> +					struct bluetooth_profile *profile);
> +void bluetooth_unregister_service(enum bluetooth_service_type type);
> +
>  void bluetooth_create_path(const char *dev_addr, const char *adapter_addr,
>  							char *buf, int size);
>  
> -- 
> 1.6.3.3

-- 
Gustavo F. Padovan
http://padovan.org