[PATCH] mbimcli: check status code in command-done message before parsing response

[Aleksander Morgado]

On 27/02/14 19:23, Ben Chan wrote:
> This patch adds missing mbim_message_command_done_get_result() checks in
> mbimcli to ensure that the status code in a command-done message reports
> no error before parsing further parsing the response. It also adds
> missing mbim_message_unref() calls to ensure that the response is freed
> in case of an error.
> ---

Pushed, thanks.

> Hi Aleksander,
> 
> The error handling code seems quite repetitive. Perhaps we can later refactor
> the common code into some helper function.
> 

Yeah, we could do that. But then, we still need to handle the
non-standard cases where a status error code is given along with a valid
information buffer (i.e. the helper function will be only for a subset
of the cases).

> Thanks,
> Ben
> 
>  src/mbimcli/mbimcli-basic-connect.c  | 34 ++++++++++++++++++++++++++++++----
>  src/mbimcli/mbimcli-ms-firmware-id.c |  4 +++-
>  src/mbimcli/mbimcli-phonebook.c      | 16 ++++++++++++----
>  3 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/src/mbimcli/mbimcli-basic-connect.c b/src/mbimcli/mbimcli-basic-connect.c
> index a73da19..e90a767 100644
> --- a/src/mbimcli/mbimcli-basic-connect.c
> +++ b/src/mbimcli/mbimcli-basic-connect.c
> @@ -265,9 +265,11 @@ query_device_caps_ready (MbimDevice   *device,
>      gchar *hardware_info;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -359,9 +361,11 @@ query_subscriber_ready_status_ready (MbimDevice   *device,
>      gchar *telephone_numbers_str;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -420,9 +424,11 @@ query_radio_state_ready (MbimDevice   *device,
>      const gchar *software_radio_state_str;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -463,9 +469,11 @@ query_device_services_ready (MbimDevice   *device,
>      guint32 max_dss_sessions;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -556,6 +564,8 @@ pin_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -658,6 +668,8 @@ connect_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -787,6 +799,8 @@ home_provider_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -841,6 +855,8 @@ preferred_providers_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -909,6 +925,8 @@ visible_providers_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -986,6 +1004,8 @@ register_state_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -1064,6 +1084,8 @@ signal_state_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -1125,6 +1147,8 @@ packet_service_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -1195,6 +1219,8 @@ packet_statistics_ready (MbimDevice   *device,
>      if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> diff --git a/src/mbimcli/mbimcli-ms-firmware-id.c b/src/mbimcli/mbimcli-ms-firmware-id.c
> index b295795..3563684 100644
> --- a/src/mbimcli/mbimcli-ms-firmware-id.c
> +++ b/src/mbimcli/mbimcli-ms-firmware-id.c
> @@ -116,9 +116,11 @@ query_firmware_id_ready (MbimDevice   *device,
>      gchar *firmware_id_str;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> diff --git a/src/mbimcli/mbimcli-phonebook.c b/src/mbimcli/mbimcli-phonebook.c
> index 1c124e2..0a544d6 100644
> --- a/src/mbimcli/mbimcli-phonebook.c
> +++ b/src/mbimcli/mbimcli-phonebook.c
> @@ -201,9 +201,11 @@ set_phonebook_write_ready (MbimDevice   *device,
>      GError *error = NULL;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -229,9 +231,11 @@ set_phonebook_delete_ready (MbimDevice   *device,
>      GError *error = NULL;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -259,9 +263,11 @@ query_phonebook_read_ready (MbimDevice   *device,
>      gint i = 0;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> @@ -305,9 +311,11 @@ query_phonebook_configuration_ready (MbimDevice   *device,
>      guint32 max_name;
>  
>      response = mbim_device_command_finish (device, res, &error);
> -    if (!response) {
> +    if (!response || !mbim_message_command_done_get_result (response, &error)) {
>          g_printerr ("error: operation failed: %s\n", error->message);
>          g_error_free (error);
> +        if (response)
> +            mbim_message_unref (response);
>          shutdown (FALSE);
>          return;
>      }
> 


-- 
Aleksander
https://aleksander.es