Hi Vinicius,
On Mon, May 28, 2012 at 04:45:06PM -0300, Vinicius Costa Gomes wrote:
> Hi Ido,
>
> On 21:34 Mon 28 May, Ido Yariv wrote:
> > New requests and responses are never sent if a request was sent and the
> > response for it hasn't been received yet. As a result, if both end
> > points send requests at the same time, a deadlock could occur. This
> > could happen, for instance, if the client sends a read request and the
> > server sends an indication before responding to the read request.
> >
> > Fix this by introducing an additional queue for responses. Responses may
> > be sent while there's still a pending request/indication.
> > ---
> > attrib/gattrib.c | 86 +++++++++++++++++++++++++++++++++++++++++++----------
> > 1 files changed, 69 insertions(+), 17 deletions(-)
> >
> > diff --git a/attrib/gattrib.c b/attrib/gattrib.c
> > index 769be36..769d746 100644
> > --- a/attrib/gattrib.c
> > +++ b/attrib/gattrib.c
> > @@ -47,6 +47,7 @@ struct _GAttrib {
> > guint write_watch;
> > guint timeout_watch;
> > GQueue *queue;
> > + GQueue *response_queue;
>
> I would change the name of the other queue as well, to make it clear
> that the two queues have different objectives.
Sure, why not.
>
> > GSList *events;
> > guint next_cmd_id;
> > guint next_evt_id;
> > @@ -175,9 +176,13 @@ static void attrib_destroy(GAttrib *attrib)
> >
> > while ((c = g_queue_pop_head(attrib->queue)))
> > command_destroy(c);
>
> I would add a empty line here.
Done.
>
> > + while ((c = g_queue_pop_head(attrib->response_queue)))
> > + command_destroy(c);
> >
> > g_queue_free(attrib->queue);
> > attrib->queue = NULL;
>
> And here.
Done.
>
> > + g_queue_free(attrib->response_queue);
> > + attrib->response_queue = NULL;
...
> > @@ -421,6 +442,7 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> > gpointer user_data, GDestroyNotify notify)
> > {
> > struct command *c;
> > + GQueue *queue;
> >
> > c = g_try_new0(struct command, 1);
> > if (c == NULL)
> > @@ -435,15 +457,25 @@ guint g_attrib_send(GAttrib *attrib, guint id, guint8 opcode,
> > c->user_data = user_data;
> > c->notify = notify;
> >
> > + if (is_response(opcode))
> > + queue = attrib->response_queue;
> > + else
> > + queue = attrib->queue;
> > +
> > if (id) {
> > c->id = id;
> > - g_queue_push_head(attrib->queue, c);
> > + g_queue_push_head(queue, c);
>
> I don't know if this is the right thing to do for responses. For
> requests there's is no problem if I rearrange them, but if I rearrange
> responses I guess that it would confuse the remote side.
Good catch. Though I don't see how this could happen in practice, I'll
verify that we don't re-order responses to be on the safe side.
>
> > } else {
> > c->id = ++attrib->next_cmd_id;
> > - g_queue_push_tail(attrib->queue, c);
> > + g_queue_push_tail(queue, c);
> > }
...
> > -gboolean g_attrib_cancel_all(GAttrib *attrib)
> > +static gboolean g_attrib_cancel_all_per_queue(GQueue *queue)
>
> I don't think that the g_attrib_ prefix here adds much, if that function
> is not exported.
Sure, I'll change that.
...
> Other than that, patch looks good.
Thanks for reviewing it!
>
> Just for information, some time ago I did something similar, but I never
> felt sure enough about it, but I guess that I was not that far off, in case
> you want to take a look:
>
> http://git.infradead.org/users/vcgomes/bluez.git/commitdiff/refs/heads/two-queues
Nice, it looks very similar :)
Thanks,
Ido.
--
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
