<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 18, 2014 at 7:46 AM, Dan Williams <span dir="ltr"><<a href="mailto:dcbw@redhat.com" target="_blank">dcbw@redhat.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, 2014-06-18 at 07:24 -0700, <a href="mailto:pprabhu@chromium.org">pprabhu@chromium.org</a> wrote:<br>


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

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

<div>        error = g_error_copy (inner_error);</div><div>    g_clear_error (&inner_error); </div><div><br></div><div>You're the style czar. You decide ;)</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<span class="HOEnZb"><font color="#888888"><br>
Dan<br>
<br>
</font></span></blockquote></div><br></div></div>