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

[Dan Williams]

On Mon, 2014-06-16 at 14:49 -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 | 60 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/src/mm-bearer-mbim.c b/src/mm-bearer-mbim.c
> index b1e3176..09bb29b 100644
> --- a/src/mm-bearer-mbim.c
> +++ b/src/mm-bearer-mbim.c
> @@ -950,35 +950,47 @@ 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 ||
> +            error->code == MBIM_STATUS_ERROR_FAILURE ||
> +            error->code == MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED) {

I know the code was like this before, but typically we should use:

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)) {

one reason to do that is that it correctly handles a NULL error,
preventing NULL dereference.  Also, if the returned error doesn't happen
to be in the MBIM_STATUS_ERROR we might mis-match it.

> +            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 (error->code == MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED) {

Same here...

> +            mm_dbg ("Session ID '%u' already disconnected.", session_id);
> +            g_clear_error (&error);
> +            g_clear_error (&inner_error);
> +        } else if (error->code == MBIM_STATUS_ERROR_FAILURE) {

and here...

>              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;

Don't need {} for the if() in this case since there's only one
statement, might as well remove both of them.

>          }
> +        g_clear_error (&inner_error);
>      }

Dan