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

[Carlo Lobrano]

Sure, I didn't see the other error. I'll update the patch


Il venerdì 24 marzo 2017, Aleksander Morgado <aleksander at aleksander.es> ha
scritto:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20170324/3229dbc5/attachment-0001.html>