[PATCH v2] Fixed wrong MEM1 value passed to +CPMS

[Aleksander Morgado]

Hey Carlo,

On Fri, Mar 24, 2017 at 12:48 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> Let modem_messaging_set_default_storage returns with error
> if current_sms_mem1_storage is MM_SMS_STORAGE_UNKNOWN
>
> ---
>
> I updated the patch according the code review, moreover I'd like
> to note a little difference respect the previous version:
>
> currently, even if +CPMS fails because of the wrong value for
> mem1, we do store "storage" as the current_sms_mem2_storage value.
>
>      self->priv->current_sms_mem2_storage = storage;
>
> In this patch I preferred to change this behaviour and setting default
> storage for SMS mem2 only if we are actually going to set
> this value on the modem.
>

That is a good change, yes.

> What do you think about it?
>

The same issue happens in mm_broadband_modem_lock_sms_storages();
where we may end up using "self->priv->current_sms_mem1_storage"
without checking it is UNKNOWN first. Could you also update that
method with the same logic? i.e. return an error if we end up there
with UNKNOWN in "current_sms_mem1_storage".

Also see comments below.

> ---
>  src/mm-broadband-modem.c       | 29 +++++++++++++++++++----------
>  src/mm-iface-modem-messaging.c |  2 +-
>  2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 17b3253..0bbf626 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5590,22 +5590,31 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
>      gchar *mem1_str;
>      gchar *mem_str;
>
> -    result = g_simple_async_result_new (G_OBJECT (self),
> -                                        callback,
> -                                        user_data,
> -                                        modem_messaging_set_default_storage);
> +    /* We provide the current sms storage for mem1 if not UNKNOWN */
> +    if (self->priv->current_sms_mem1_storage == MM_SMS_STORAGE_UNKNOWN) {
> +        g_simple_async_report_error_in_idle (G_OBJECT (self),
> +                                             callback,
> +                                             user_data,
> +                                             MM_CORE_ERROR,
> +                                             MM_CORE_ERROR_INVALID_ARGS,
> +                                             "Current SMS MEM1 storage is UNKNOWN (SIM busy?)");
> +        return;
> +    }
>
>      /* Set defaults as current */
>      self->priv->current_sms_mem2_storage = storage;
>
> -    /* We provide the current sms storage for mem1 if not UNKNOWN */
>      mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
> -
>      mem_str = g_ascii_strup (mm_sms_storage_get_string (storage), -1);
> -    cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"",
> -                           mem1_str ? mem1_str : "",
> -                           mem_str,
> -                           mem_str);
> +
> +    cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"", mem1_str, mem_str, mem_str);
> +
> +    result = g_simple_async_result_new (G_OBJECT (self),
> +                                        callback,
> +                                        user_data,
> +                                        modem_messaging_set_default_storage);
> +
> +

Just a single whiteline here.

>      mm_base_modem_at_command (MM_BASE_MODEM (self),
>                                cmd,
>                                3,
> diff --git a/src/mm-iface-modem-messaging.c b/src/mm-iface-modem-messaging.c
> index 0cff1f2..01a30f0 100644
> --- a/src/mm-iface-modem-messaging.c
> +++ b/src/mm-iface-modem-messaging.c
> @@ -789,7 +789,7 @@ set_default_storage_ready (MMIfaceModemMessaging *self,
>      GError *error = NULL;
>
>      if (!MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (self)->set_default_storage_finish (self, res, &error)) {
> -        mm_dbg ("Couldn't set default storage: '%s'", error->message);
> +        mm_warn ("Could not set default storage: '%s'", error->message);
>          g_error_free (error);
>      }
>
> --
> 2.9.3
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel



-- 
Aleksander
https://aleksander.es