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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
