Re: [PATCH v15 02/14] audio: Move telephony drivers to D-Bus interface

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Frédéric,

On Thu, Jul 26, 2012, Frédéric Danis wrote:
> +static int parse_properties(DBusMessageIter *props, const char **uuid,
> +				uint16_t *version, uint16_t *features)
> +{
> +	gboolean has_uuid = FALSE;
> +
> +	while (dbus_message_iter_get_arg_type(props) == DBUS_TYPE_DICT_ENTRY) {
> +		const char *key;
> +		DBusMessageIter value, entry;
> +		int var;
> +
> +		dbus_message_iter_recurse(props, &entry);
> +		dbus_message_iter_get_basic(&entry, &key);
> +
> +		dbus_message_iter_next(&entry);
> +		dbus_message_iter_recurse(&entry, &value);
> +
> +		var = dbus_message_iter_get_arg_type(&value);
> +		if (strcasecmp(key, "UUID") == 0) {
> +			if (var != DBUS_TYPE_STRING)
> +				return -EINVAL;
> +			dbus_message_iter_get_basic(&value, uuid);
> +			has_uuid = TRUE;
> +		} else if (strcasecmp(key, "Version") == 0) {
> +			if (var != DBUS_TYPE_UINT16)
> +				return -EINVAL;
> +			dbus_message_iter_get_basic(&value, version);
> +		} else if (strcasecmp(key, "Features") == 0) {
> +			if (var != DBUS_TYPE_UINT16)
> +				return -EINVAL;
> +			dbus_message_iter_get_basic(&value, features);
> +		}
> +
> +		dbus_message_iter_next(props);
> +	}
> +
> +	return (has_uuid) ? 0 : -EINVAL;
> +}

I suppose you could just make the above function return gboolean as it
only has two possible return values.

> +static int dev_close(struct telephony_device *tel_dev)
> +{
> +	int sock;
> +
> +	if (tel_dev->rfcomm) {
> +		sock = g_io_channel_unix_get_fd(tel_dev->rfcomm);
> +		shutdown(sock, SHUT_RDWR);
> +		tel_dev->rfcomm = NULL;
> +	}

Looks like you're missing a g_io_channel_unref there.

> +static void hs_newconnection_reply(DBusPendingCall *call, void *user_data)
> +{
> +	struct telephony_device *tel_dev = user_data;
> +	DBusMessage *reply = dbus_pending_call_steal_reply(call);
> +	DBusError derr;
> +
> +	dbus_error_init(&derr);
> +	if (!dbus_set_error_from_message(&derr, reply)) {
> +		DBG("Agent reply: file descriptor passed successfully");
> +		g_io_add_watch(tel_dev->rfcomm, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> +				(GIOFunc) hs_dev_disconnect_cb, tel_dev);
> +		headset_slc_complete(tel_dev->au_dev);
> +		goto done;
> +	}

Firstly, a more common way would be to test for positive return of
dbus_set_error_from_message and handle the error reply within the
if-clause. Secondly, please don't do callback typecasts (GIOFunc) but
instead just assign to the right type inside the callback function
itself.

> +static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
> +{
> +	struct telephony_device *tel_dev = user_data;

Here you do the right kind of handling of callback types. Why the
inconsistency?

> +	sdp_get_profile_descs(recs->data, &profiles);
> +	if (profiles == NULL)
> +		goto failed;

I think it'd be cleaner/simpler to do:

	if (sdp_get_profile_descs(...) < 0)
		goto failed;


> +	desc = profiles->data;
> +
> +	if (sdp_uuid16_cmp(&desc->uuid, &uuid) == 0)
> +		tel_dev->version = desc->version;

I don't think it's safe to assume that what's returned by
sdp_get_profile_descs is always a uuid16. Instead using sdp_uuid_cmp()
would seem more appropriate.

> +struct telephony_device *telephony_device_connecting(GIOChannel *io,
> +					struct btd_device *btd_dev,
> +					struct audio_device *au_dev,
> +					const char *uuid)
> +{
> +	struct btd_adapter *adapter;
> +	struct telephony_agent *agent;
> +	struct telephony_device *tel_dev;
> +	uuid_t r_uuid;
> +	int err;
> +
> +	adapter = device_get_adapter(btd_dev);
> +	agent = find_agent(adapter, NULL, NULL, uuid);
> +	if (agent == NULL)
> +		return NULL;
> +
> +	tel_dev = g_new0(struct telephony_device, 1);
> +	tel_dev->btd_dev = btd_device_ref(btd_dev);
> +	tel_dev->name = g_strdup(agent->name);
> +	tel_dev->path = g_strdup(agent->path);
> +	tel_dev->config = agent->config;
> +	tel_dev->au_dev = au_dev;
> +	tel_dev->rfcomm = io;

Missing g_io_channel_ref here.

> +	err = bt_search_service(&au_dev->src, &au_dev->dst, &r_uuid,
> +				get_record_cb, tel_dev, NULL);
> +	if (err < 0) {
> +		telephony_device_disconnect(tel_dev);
> +		return NULL;
> +	}
> +	tel_dev->pending_sdp = TRUE;

An empty line should follow after }

> +void telephony_device_disconnect(struct telephony_device *device)
> +{
> +	dev_close(device);
> +
> +	if (device->pending_sdp)
> +		return;

Shouldn't you cancel the SDP operation here with bt_cancel_discovery?

> +gboolean telephony_get_ready_state(struct btd_adapter *adapter)
> +{
> +	return find_agent(adapter, NULL, NULL, HFP_AG_UUID) ? TRUE : FALSE;
> +}

If such a function is needed just call it telephony_is_ready. It makes
the calling side look more natural: "if (telephony_is_ready(adapter))".

> +static int register_interface(struct btd_adapter *adapter)
> +{
> +	const char *path;
> +
> +	path = adapter_get_path(adapter);
> +
> +	if (!g_dbus_register_interface(connection, path,
> +					AUDIO_TELEPHONY_INTERFACE,
> +					telsrv_methods, NULL,
> +					NULL, adapter, path_unregister)) {
> +		error("D-Bus failed to register %s interface",
> +				AUDIO_TELEPHONY_INTERFACE);
> +		return -1;
> +	}
> +
> +	DBG("Registered interface %s", AUDIO_TELEPHONY_INTERFACE);
> +
> +	return 0;
> +}
> +
> +static void unregister_interface(struct btd_adapter *adapter)
> +{
> +	g_dbus_unregister_interface(connection, adapter_get_path(adapter),
> +			AUDIO_TELEPHONY_INTERFACE);
> +}
> +
> +int telephony_adapter_init(struct btd_adapter *adapter)
> +{
> +	DBG("adapter: %p", adapter);
> +
> +	return register_interface(adapter);
> +}
> +
> +void telephony_adapter_exit(struct btd_adapter *adapter)
> +{
> +	struct telephony_agent *agent;
> +
> +	DBG("adapter: %p", adapter);
> +
> +	unregister_interface(adapter);
> +
> +	while ((agent = find_agent(adapter, NULL, NULL, NULL)) != NULL) {
> +		agents = g_slist_remove(agents, agent);
> +		free_agent(agent);
> +	}
> +}

The register_interface and unregister_interface functions above seem
unnecessary to me. Just include their code directly within
telephony_adapter_init and telephony_adapter_exit.

Johan