[PATCH] libmbim-glib: mbim-device: Fix adding multiple timeouts for a transaction

[Greg Suarez]

On Tue, Feb 4, 2014 at 12:36 AM, Aleksander Morgado <
aleksander at aleksander.es> wrote:

> Hey,
>
> Good catch indeed, see a minor comment below.
>
> On Tue, Feb 4, 2014 at 4:25 AM, Greg Suarez <gpsuarez2512 at gmail.com>
> wrote:
> > When receiving multiple fragments device_store_transaction() is called
> > for each fragment received thus adding a timeout callback for each
> > fragment.  A timeout callback is only removed when all fragments are
> > received thus leaving dangling timeout callbacks.  This leads to a crash
> > when the MbimDevice is finalized.
> >
> > This patch checks if the transaction has already registered a timeout
> > callback before adding a new one.
> >
> > Signed-off-by: Greg Suarez <gpsuarez2512 at gmail.com>
> > ---
> >  src/libmbim-glib/mbim-device.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libmbim-glib/mbim-device.c
> b/src/libmbim-glib/mbim-device.c
> > index d8c6f81..ef47b7e 100644
> > --- a/src/libmbim-glib/mbim-device.c
> > +++ b/src/libmbim-glib/mbim-device.c
> > @@ -267,21 +267,24 @@ device_store_transaction (MbimDevice       *self,
> >      tr->wait_ctx->transaction_id = tr->transaction_id;
> >      tr->wait_ctx->type = type;
> >
> > -    tr->timeout_id = g_timeout_add (timeout_ms,
> > -                                    (GSourceFunc)transaction_timed_out,
> > -                                    tr->wait_ctx);
> > -
> > -    if (tr->cancellable) {
> > -        tr->cancellable_id = g_cancellable_connect (tr->cancellable,
> > -
>  (GCallback)transaction_cancelled,
> > -                                                    tr->wait_ctx,
> > -                                                    NULL);
> > -        if (!tr->cancellable_id) {
> > -            g_set_error_literal (error,
> > -                                 MBIM_CORE_ERROR,
> > -                                 MBIM_CORE_ERROR_ABORTED,
> > -                                 "Request is already cancelled");
> > -            return FALSE;
> > +    /* don't add timeout if one already exists */
> > +    if (!tr->timeout_id) {
> > +        tr->timeout_id = g_timeout_add (timeout_ms,
> > +
>  (GSourceFunc)transaction_timed_out,
> > +                                        tr->wait_ctx);
> > +
>
> How about moving the if (tr->cancellable) change  out of the new if
> (!tr->timeout_id) you added, and also extend it so that it checks if
> (tr->cancellable && !tr->cancellable_id) ?
>
> Just to separate the logic of the timeout with that of the cancellable.
>
>
Sure, I'll resubmit the patches as a set.

Greg
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libmbim-devel/attachments/20140204/1bb41ffc/attachment.html>