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

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