Re: [PATCH 10/10] hfp_hf_bluez5: Add SLC establishment procedure

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Vinicius,

On 01/22/2013 03:43 PM, Vinicius Costa Gomes wrote:
> When receiving a NewConnection call from BlueZ, initiates the Service
> Level Connection using the received file descriptor. The HFP modem
> sub-components (devinfo, voicecall, netreg, handsfree and callvolume)
> are created at this point.
> ---
>   plugins/hfp_hf_bluez5.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 192 insertions(+), 7 deletions(-)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 39853e3..79c78e5 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -24,17 +24,28 @@
>   #endif
>
>   #include<errno.h>
> +#include<stdint.h>
> +#include<stdlib.h>
>   #include<unistd.h>
> +#include<string.h>
>
>   #include<glib.h>
>
>   #include<gdbus.h>
> +#include<gatchat.h>
>
>   #define OFONO_API_SUBJECT_TO_CHANGE
>   #include<ofono/modem.h>
>   #include<ofono/dbus.h>
>   #include<ofono/plugin.h>
>   #include<ofono/log.h>
> +#include<ofono/devinfo.h>
> +#include<ofono/netreg.h>
> +#include<ofono/voicecall.h>
> +#include<ofono/call-volume.h>
> +#include<ofono/handsfree.h>
> +
> +#include<drivers/hfpmodem/slc.h>
>
>   #include "bluez5.h"
>
> @@ -44,14 +55,153 @@
>
>   #define HFP_EXT_PROFILE_PATH   "/bluetooth/profile/hfp_hf"
>
> +struct hfp {
> +	char *device_address;
> +	struct hfp_slc_info *info;
> +	DBusMessage *msg;
> +	GIOChannel *slcio;

Why do you want to track the GIOChannel?

> +};
> +
>   static GHashTable *modem_hash = NULL;
>   static GHashTable *devices_proxies = NULL;
>   static GDBusClient *bluez = NULL;
>
> +static void hfp_free(gpointer user_data)
> +{
> +	struct hfp *hfp = user_data;
> +
> +	if (hfp->msg)
> +		dbus_message_unref(hfp->msg);
> +
> +	if (hfp->slcio)
> +		g_io_channel_unref(hfp->slcio);
> +
> +	if (hfp->info)
> +		hfp_slc_info_free(hfp->info);
> +
> +	g_free(hfp->device_address);
> +	g_free(hfp);

What in the world are you doing here?  How about using hfp_remove() 
function for its intended purpose, and that is to destroy the user data 
pertaining to the modem object.

> +}
> +
> +static void modem_data_free(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +
> +	hfp_free(hfp);
> +}

Yeah... Get rid of this ;)

> +
> +static void hfp_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	ofono_info("%s%s", prefix, str);
> +}
> +
> +static void slc_established(gpointer userdata)
> +{
> +	struct ofono_modem *modem = userdata;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +	DBusMessage *msg;
> +
> +	ofono_modem_set_powered(modem, TRUE);
> +
> +	msg = dbus_message_new_method_return(hfp->msg);
> +	g_dbus_send_message(ofono_dbus_get_connection(), msg);
> +	dbus_message_unref(hfp->msg);
> +	hfp->msg = NULL;
> +
> +	ofono_info("Service level connection established");
> +}
> +
> +static void slc_failed(gpointer userdata)
> +{
> +	struct ofono_modem *modem = userdata;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +	DBusMessage *msg;
> +
> +	msg = g_dbus_create_error(hfp->msg, BLUEZ_ERROR_INTERFACE
> +						".Failed",
> +						"HFP Handshake failed");
> +
> +	g_dbus_send_message(ofono_dbus_get_connection(), msg);
> +	dbus_message_unref(hfp->msg);
> +	hfp->msg = NULL;
> +
> +	ofono_error("Service level connection failed");
> +	ofono_modem_set_powered(modem, FALSE);
> +
> +	if (hfp->info) {
> +		hfp_slc_info_free(hfp->info);
> +		hfp->info = NULL;
> +	}
> +}
> +
> +static void hfp_disconnected_cb(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +
> +	DBG("HFP disconnected");
> +
> +	if (hfp->info) {
> +		hfp_slc_info_free(hfp->info);
> +		hfp->info = NULL;
> +	}
> +
> +	ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static GIOChannel *service_level_connection(struct ofono_modem *modem,
> +					int fd, uint16_t version, int *err)
> +{
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +	GIOChannel *io;
> +	GAtSyntax *syntax;
> +	GAtChat *chat;
> +
> +	io = g_io_channel_unix_new(fd);
> +	if (io == NULL) {
> +		ofono_error("Service level connection failed: %s (%d)",
> +						strerror(errno), errno);
> +		*err = -EIO;
> +		return NULL;
> +	}
> +
> +	syntax = g_at_syntax_new_gsm_permissive();
> +	chat = g_at_chat_new(io, syntax);
> +	g_at_syntax_unref(syntax);
> +	g_io_channel_set_close_on_unref(io, TRUE);
> +

In general it is not customary to hold on to the GIOChannel.  It is 
assumed that the GAtChat object owns it now, so I'm not sure what you're 
trying to accomplish here...

> +	if (chat == NULL) {
> +		*err = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	g_at_chat_set_disconnect_function(chat, hfp_disconnected_cb, modem);
> +
> +	if (getenv("OFONO_AT_DEBUG"))
> +		g_at_chat_set_debug(chat, hfp_debug, "");
> +
> +	hfp->info = hfp_slc_info_init(chat, version);
> +	g_at_chat_unref(chat);
> +	hfp_slc_establish(hfp->info, slc_established, slc_failed, modem);
> +
> +	*err = -EINPROGRESS;
> +
> +	return io;
> +
> +fail:
> +	g_at_chat_unref(chat);
> +	g_io_channel_unref(io);
> +	return NULL;
> +}
> +
>   static struct ofono_modem *modem_register(const char *device,
> -						const char *alias)
> +				const char *device_address, const char *alias)
>   {
>   	struct ofono_modem *modem;
> +	struct hfp *hfp;
>   	char *path;
>
>   	modem = g_hash_table_lookup(modem_hash, device);
> @@ -70,6 +220,11 @@ static struct ofono_modem *modem_register(const char *device,
>   	ofono_modem_set_name(modem, alias);
>   	ofono_modem_register(modem);
>
> +	hfp = g_new0(struct hfp, 1);
> +	hfp->device_address = g_strdup(device_address);
> +
> +	ofono_modem_set_data(modem, hfp);
> +

In general this belongs in the probe() function.  The data should be 
created and ready prior to calling ofono_modem_register().  If the 
device_address is to only be used in the pre_sim() function, you can 
also use ofono_modem_set_string(modem, "Address", device_address) and 
avoid keeping track of this variable here.

>   	g_hash_table_insert(modem_hash, g_strdup(device), modem);
>
>   	return modem;
> @@ -104,7 +259,15 @@ static int hfp_disable(struct ofono_modem *modem)
>
>   static void hfp_pre_sim(struct ofono_modem *modem)
>   {
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +
>   	DBG("%p", modem);
> +
> +	ofono_devinfo_create(modem, 0, "hfpmodem", hfp->device_address);

And then here simply call ofono_modem_get_string(modem, "Address")

> +	ofono_voicecall_create(modem, 0, "hfpmodem", hfp->info);
> +	ofono_netreg_create(modem, 0, "hfpmodem", hfp->info);
> +	ofono_handsfree_create(modem, 0, "hfpmodem", hfp->info);
> +	ofono_call_volume_create(modem, 0, "hfpmodem", hfp->info);
>   }
>
>   static void hfp_post_sim(struct ofono_modem *modem)
> @@ -126,12 +289,13 @@ static struct ofono_modem_driver hfp_driver = {
>   static DBusMessage *profile_new_connection(DBusConnection *conn,
>   					DBusMessage *msg, void *user_data)
>   {
> +	struct hfp *hfp;
>   	struct ofono_modem *modem;
>   	DBusMessageIter iter;
>   	GDBusProxy *proxy;
>   	DBusMessageIter entry;
> -	const char *device, *alias;
> -	int fd;
> +	const char *device, *alias, *address;
> +	int fd, err;
>
>   	DBG("Profile handler NewConnection");
>
> @@ -152,6 +316,11 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	else
>   		dbus_message_iter_get_basic(&iter,&alias);
>
> +	if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
> +		goto error;
> +
> +	dbus_message_iter_get_basic(&iter,&address);
> +
>   	dbus_message_iter_next(&entry);
>   	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>   		goto error;
> @@ -160,10 +329,20 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	if (fd<  0)
>   		goto error;
>
> -	modem = modem_register(device, alias);
> +	modem = modem_register(device, address, alias);
>   	if (modem == NULL)
>   		goto error;
>
> +	hfp = ofono_modem_get_data(modem);
> +
> +	hfp->msg = dbus_message_ref(msg);
> +	hfp->slcio = service_level_connection(modem, fd,
> +					HFP_VERSION_LATEST,&err);
> +	if (err<  0&&  err != -EINPROGRESS) {
> +		hfp_free(hfp);
> +		goto error;
> +	}
> +
>   	return NULL;
>
>   error:
> @@ -263,7 +442,7 @@ static void parse_uuids(DBusMessageIter *array, gpointer user_data)
>
>   static void proxy_added(GDBusProxy *proxy, void *user_data)
>   {
> -	const char *interface, *path, *alias;
> +	const char *interface, *path, *alias, *address;
>   	DBusMessageIter iter;
>   	GSList *l, *uuids = NULL;
>
> @@ -299,7 +478,13 @@ static void proxy_added(GDBusProxy *proxy, void *user_data)
>
>   	dbus_message_iter_get_basic(&iter,&alias);
>
> -	modem_register(path, alias);
> +
> +	if (g_dbus_proxy_get_property(proxy, "Address",&iter) == FALSE)
> +		return;
> +
> +	dbus_message_iter_get_basic(&iter,&address);
> +
> +	modem_register(path, address, alias);
>   }
>
>   static void proxy_removed(GDBusProxy *proxy, void *user_data)
> @@ -382,7 +567,7 @@ static int hfp_init(void)
>   	}
>
>   	modem_hash = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> -								NULL);
> +							modem_data_free);

This chunk should not longer be required.

>
>   	devices_proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
>   				g_free, (GDestroyNotify) g_dbus_proxy_unref);

Regards,
-Denis