[PATCH] Fixed wrong MEM1 value passed to +CPMS

[Aleksander Morgado]

On Thu, Mar 23, 2017 at 2:29 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> Let mem1_str defaulting to NULL when current_sms_mem1_storage is
> MM_SMS_STORAGE_UNKNOWN
>
> modem_messaging_set_default_storage considers mem1 storage
> string valid if *not NULL*, but, previously, when we could not get
> current_sms_mem1_storage value right, mem1_str defaulted to "unknown"
> and that string was used in +CPMS command, resulting in an ERROR.
>

The current_sms_mem1_storage shouldn't be UNKNOWN unless:
  * init_current_storages() hasn't been run yet
  * init_current_storages () failed.

So why is current_sms_mem1_storage UNKNOWN? At which point did it
fail, if it did? Did it fail doing AT+CPMS? in
modem_messaging_init_current_storages()? Or is it that
set_default_storage() was called before init_current_storages() was
executed?

I agree that we shouldn't call any CPMS command passing "unknown" as
storage, but defaulting to "SM" isn't a good approach either. We
actually test which are the supported storages at some point
(modem_messaging_load_supported_storages()) to avoid assuming any of
them is always available.

If it is UNKNOWN because init_current_storages() failed, we should
also error out when trying to set_default_storage()

> ---
>  src/mm-broadband-modem.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 17b3253..776e6d1 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5587,7 +5587,7 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
>      MMBroadbandModem *self = MM_BROADBAND_MODEM (_self);
>      gchar *cmd;
>      GSimpleAsyncResult *result;
> -    gchar *mem1_str;
> +    gchar *mem1_str = NULL;
>      gchar *mem_str;
>
>      result = g_simple_async_result_new (G_OBJECT (self),
> @@ -5599,11 +5599,14 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
>      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);
> +    if (self->priv->current_sms_mem1_storage != MM_SMS_STORAGE_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 : "",
> +                           /* Empty value is not accepted and SM is the default one */
> +                           mem1_str ? mem1_str : "SM",
>                             mem_str,
>                             mem_str);
>      mm_base_modem_at_command (MM_BASE_MODEM (self),
> --
> 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