[PATCH] bearer-mbim: refactor disconnect_set_ready()

[Dan Williams]

On Fri, 2017-08-04 at 14:00 +0200, Aleksander Morgado wrote:
> Try to make it more clear which are the different branches in the
> logic, and jump out as soon as the branch is finished.
> ---
> 
> Hey,
> 
> How about something like this?

LGTM

> Cheers!
> 
> ---
>  src/mm-bearer-mbim.c | 96 +++++++++++++++++++++++++++++-------------
> ----------
>  1 file changed, 53 insertions(+), 43 deletions(-)
> 
> diff --git a/src/mm-bearer-mbim.c b/src/mm-bearer-mbim.c
> index b863366c..62f774d4 100644
> --- a/src/mm-bearer-mbim.c
> +++ b/src/mm-bearer-mbim.c
> @@ -1086,59 +1086,69 @@ disconnect_set_ready (MbimDevice *device,
>      guint32 session_id;
>      MbimActivationState activation_state;
>      guint32 nw_error;
> +    GError *inner_error = NULL;
> +    gboolean result = FALSE, parsed_result = FALSE;
> 
>      ctx = g_task_get_task_data (task);
> 
>      response = mbim_device_command_finish (device, res, &error);
> -    if (response) {
> -        GError *inner_error = NULL;
> -        gboolean result = FALSE, parsed_result = FALSE;
> -
> -        result = mbim_message_response_get_result (response,
> MBIM_MESSAGE_TYPE_COMMAND_DONE, &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 (!response)
> +        goto out;
> +
> +    result = mbim_message_response_get_result (response,
> MBIM_MESSAGE_TYPE_COMMAND_DONE, &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);
> +    }
> 
> -        /* Now handle different response / error cases */
> -        if (result && 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_AC
> TIVATED)) {
> -            if (parsed_result)
> -                mm_dbg ("Context not activated: session ID '%u'
> already disconnected", session_id);
> -            else
> -                mm_dbg ("Context not activated: already
> disconnected");
> +    /* Now handle different response / error cases */
> 
> -            g_clear_error (&error);
> -            g_clear_error (&inner_error);
> -        } else if (g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_FAILURE)) {
> -            if (parsed_result && nw_error != 0) {
> -                g_error_free (error);
> -                error = mm_mobile_equipment_error_from_mbim_nw_error
> (nw_error);
> -            }
> -        }  /* else: For other errors, give precedence to error over
> nw_error */
> +    if (result && parsed_result) {
> +        g_assert (!error);
> +        g_assert (!inner_error);
> +        mm_dbg ("Session ID '%u': %s", session_id,
> mbim_activation_state_get_string (activation_state));
> +        /* success */
> +        goto out;
> +    }
> 
> -        /* Give precedence to original error over parsing error */
> -        if (!error && inner_error)
> -            error = g_error_copy (inner_error);
> +    if (g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_CONTEXT_NOT_ACTIVATED)) {
> +        if (parsed_result)
> +            mm_dbg ("Context not activated: session ID '%u' already
> disconnected", session_id);
> +        else
> +            mm_dbg ("Context not activated: already disconnected");
> +
> +        g_clear_error (&error);
>          g_clear_error (&inner_error);
> +        /* success */
> +        goto out;
> +    }
> +
> +    if (g_error_matches (error, MBIM_STATUS_ERROR,
> MBIM_STATUS_ERROR_FAILURE) && parsed_result && nw_error != 0) {
> +        g_assert (!inner_error);
> +        g_error_free (error);
> +        error = mm_mobile_equipment_error_from_mbim_nw_error
> (nw_error);
> +        /* error out with nw_error error */
> +        goto out;
>      }
> 
> +    /* Give precedence to original error over parsing error */
> +    if (!error && inner_error)
> +        error = g_error_copy (inner_error);
> +    g_clear_error (&inner_error);
> +
> +out:
> +
>      if (response)
>          mbim_message_unref (response);
> 
> --
> 2.13.1
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel