[PATCH v3] bearer-mbim: disconnect attempt should succeed if bearer is already disconnected

[Prathmesh Prabhu Chromium]

On Wed, Jun 18, 2014 at 7:46 AM, Dan Williams <dcbw at redhat.com> wrote:

> On Wed, 2014-06-18 at 07:24 -0700, pprabhu at chromium.org wrote:
> > From: Prathmesh Prabhu <pprabhu at chromium.org>
> >
> > When trying to disconnect bearer, if the modem responds with
> > MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED, take it to mean that the bearer
> has
> > already been disconnected.
> > ---
> >  src/mm-bearer-mbim.c | 64
> ++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 39 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/mm-bearer-mbim.c b/src/mm-bearer-mbim.c
> > index b1e3176..2ff3415 100644
> > --- a/src/mm-bearer-mbim.c
> > +++ b/src/mm-bearer-mbim.c
> > @@ -950,35 +950,49 @@ disconnect_set_ready (MbimDevice *device,
> >      guint32 nw_error;
> >
> >      response = mbim_device_command_finish (device, res, &error);
> > -    if (response &&
> > -        (mbim_message_command_done_get_result (response, &error) ||
> > -         error->code == MBIM_STATUS_ERROR_FAILURE)) {
> > +    if (response) {
> >          GError *inner_error = NULL;
> > +        gboolean result = FALSE, parsed_result = FALSE;
> > +
> > +        result = mbim_message_command_done_get_result (response,
> &error);
> > +        /* Parse the response only for the cases we need to */
> > +        if (result ||
> > +            g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_FAILURE) ||
> > +            g_error_matches (error, MBIM_STATUS_ERROR,
> > +                             MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED)) {
> > +            parsed_result = mbim_message_connect_response_parse (
> > +                    response,
> > +                    &session_id,
> > +                    &activation_state,
> > +                    NULL, /* voice_call_state */
> > +                    NULL, /* ip_type */
> > +                    NULL, /* context_type */
> > +                    &nw_error,
> > +                    &inner_error);
> > +        }
> >
> > -        if (mbim_message_connect_response_parse (
> > -                response,
> > -                &session_id,
> > -                &activation_state,
> > -                NULL, /* voice_call_state */
> > -                NULL, /* ip_type */
> > -                NULL, /* context_type */
> > -                &nw_error,
> > -                &inner_error)) {
> > +        /* Now handle different response / error cases */
> > +        if (parsed_result) {
> > +            mm_dbg ("Session ID '%u': %s",
> > +                    session_id,
> > +                    mbim_activation_state_get_string
> (activation_state));
> > +        } else if (g_error_matches (error,
> > +                                    MBIM_STATUS_ERROR,
> > +
>  MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED)) {
> > +            mm_dbg ("Session ID '%u' already disconnected.",
> session_id);
> > +            g_clear_error (&error);
> > +            g_clear_error (&inner_error);
> > +        } else if (g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_FAILURE)) {
> >              if (nw_error) {
> > -                if (error)
> > -                    g_error_free (error);
> > +                g_error_free (error);
> >                  error = mm_mobile_equipment_error_from_mbim_nw_error
> (nw_error);
> > -            } else
> > -                mm_dbg ("Session ID '%u': %s",
> > -                        session_id,
> > -                        mbim_activation_state_get_string
> (activation_state));
> > -        } else {
> > -            /* Prefer the error from the result to the parsing error */
> > -            if (!error)
> > -                error = inner_error;
> > -            else
> > -                g_error_free (inner_error);
> > -        }
> > +            }
> > +        }  /* else: For other errors, give precedence to error over
> nw_error */
> > +
> > +        /* Give precedence to original error over parsing error */
> > +        if (!error && inner_error)
> > +            error = inner_error;
> > +        g_clear_error (&inner_error);
>
> Oops!
Yes, your snippet is logically correct.


>  Missed this the first time around; this g_clear_error() will also clear
> 'error' due to the pointer assignment but leave 'error' dangling.  Would
> this:
>
>         if (!error && inner_error)
>             error = inner_error;
>         else
>             g_clear_error (&inner_error);
>
> be OK?  I can fix that up when I commit if that it fine.
>
I think this is not a performance critical path really.
In that case, it might be clearer to say

    if (!error && inner_error)
        error = g_error_copy (inner_error);
    g_clear_error (&inner_error);

You're the style czar. You decide ;)

>
> Dan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20140618/89375d81/attachment.html>