Re: [PATCH BlueZ v6 03/11] core: Mutually exclude concurrent connections

Hi,

On Tue, Sep 11, 2012, João Paulo Rechi Vita wrote:

Since controllers don't support more than one ongoing connection
procedure at the same time, new connection attempts needs to yield if
there is an ongoing connection procedure already.
---
 src/adapter.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
 src/device.c  |  6 +++---
 src/device.h  |  2 +-
 3 files changed, 57 insertions(+), 12 deletions(-)

Couple of coding style things I'd like to get fixed in this one. Patches
1/11 and 2/11 have been already applied though so no need to resend
them.

+	if (g_slist_length(adapter->connect_list))
+		mgmt_start_discovery(adapter->dev_id);

Since you're not testing for a boolean I'd rather have a clear > 0 here.

 	const char *path = adapter->path;
+	guint connect_list_size;

Call this conn_list_len (to make it shorter and to match what the
function you get this value from is called)

+	if (!adapter_has_discov_sessions(adapter) && !connect_list_size)
 		return;

Again since conn_list_len is not a boolean I'd rather have a clear == 0.

-	DBG("hci%u restarting discovery, disc_sessions %u", adapter->dev_id,
-					g_slist_length(adapter->disc_sessions));
+	DBG("hci%u restarting discovery: disc_sessions %u, connect_list size "
+		"%u", adapter->dev_id, g_slist_length(adapter->disc_sessions),

Splitting strings like this is something that we really should try to
avoid. If you do the variable rename as I suggested you can get this on
the same line.

+static gboolean clean_connecting_state(GIOChannel *io, GIOCondition cond, gpointer user_data)

Looks like a too long line to me.

+	if (adapter->waiting_to_connect == 0 &&
+					g_slist_length(adapter->connect_list))
+		mgmt_start_discovery(adapter->dev_id);

Again > 0 here (which is especially strange that you didn't do it from
the start as in the first test you do use == 0).

+	btd_device_unref(device);
+	return FALSE;
+}

Empty line before the return statement please.

+	btd_device_unref(device);
 	return FALSE;
 }

Same here.

As a general note (not relating specifically to this patch) I'd like to
see a clearer split and naming of LE/GATT-specific functionality and
variables (function names, struct btd_adapter/device member naming &
grouping, etc.). Right now these seem to be sprinkled all over the place
which can be confusing for someone looking at the code from a
BR/EDR-only or LE-only perspective.

Johan