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

Aleksander Morgado aleksander at aleksander.es
Tue Mar 28 09:06:07 UTC 2017


Hey

On Mon, Mar 27, 2017 at 5:50 PM, Carlo Lobrano <c.lobrano at gmail.com> wrote:
> In functions
>
> - mm_broadband_modem_lock_sms_storages
> - modem_messaging_set_default_storage
>
> return with error when using current_sms_mem1_storage as +CPMS value, but
> current_sms_mem1_storage value is UNKNOWN.
>
> ---
>
>>> +        g_simple_async_report_error_in_idle (
>>
>>report_error_in_idle() shouldn't be used when there is already a
>>GSimpleAsyncResult created in the context. You should instead:
>>
>>g_simple_async_result_set_error (ctx->result, MM_CORE_ERROR...);
>>
>>> +                G_OBJECT (self),
>>> +                callback,
>>> +                user_data,
>>> +                MM_CORE_ERROR,
>>> +                MM_CORE_ERROR_RETRY,
>>> +                "Unknown SMS mem1 storage is allowed only if the current sms mem1 storage value is not unknown.");
>>
>>This message is a bit hard to get... maybe something like "cannot lock
>>mem2 storage alone when current mem1 storage is unknown"?
>>
>>> +        lock_sms_storages_context_complete_and_free (ctx);
>>
>>And as we're completing the GSimpleAsyncResult directly in the
>>mm_broadband_modem_lock_sms_storages() method, the
>>lock_sms_storages_context_complete_and_free() method should be updated
>>so that the completion is done in idle (i.e.
>>g_simple_async_result_complete_in_idle().
>
> Hope this is right this time. I used "list_call_context_complete_and_free" as example.
>

Yes, it's ok now.

>
>>> +                "Unknown SMS mem1 storage is allowed only if the current sms mem1 storage value is not unknown.");
>>
>>This message is a bit hard to get... maybe something like "cannot lock
>>mem2 storage alone when current mem1 storage is unknown"?
>
> I wasn't totally happy with them myself. I accept your suggestions.
>

Good!

> Sorry for missing the whitespace twice. Btw, do you have any automatic tool for checking the coding rules or just a good eye? :D
>

Nah, I just review the patch with "git show" once I apply it :)

I've fixed up a minor typo in a comment (s/previoulsy/previously) and
pushed it to git master and mm-1-6.

Thanks!

> ---
>  src/mm-broadband-modem.c       | 77 +++++++++++++++++++++++++-----------------
>  src/mm-iface-modem-messaging.c |  2 +-
>  2 files changed, 47 insertions(+), 32 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 302fc3d..59bffe5 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5419,7 +5419,7 @@ typedef struct {
>  static void
>  lock_sms_storages_context_complete_and_free (LockSmsStoragesContext *ctx)
>  {
> -    g_simple_async_result_complete (ctx->result);
> +    g_simple_async_result_complete_in_idle (ctx->result);
>      g_object_unref (ctx->result);
>      g_object_unref (ctx->self);
>      g_slice_free (LockSmsStoragesContext, ctx);
> @@ -5459,7 +5459,7 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self,
>                                        gpointer user_data)
>  {
>      LockSmsStoragesContext *ctx;
> -    gchar *cmd;
> +    gchar *cmd = NULL;
>      gchar *mem1_str = NULL;
>      gchar *mem2_str = NULL;
>
> @@ -5488,45 +5488,52 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self,
>                                               user_data,
>                                               mm_broadband_modem_lock_sms_storages);
>
> +    /* Some modems may not support empty string parameters, then if mem1 is
> +     * UNKNOWN, and current sms mem1 storage has a valid value, we send again
> +     * the already locked mem1 value in place of an empty string.
> +     * This way we also avoid to confuse the environment of other sync operation
> +     * that have potentially locked mem1 previoulsy.
> +     */
>      if (mem1 != MM_SMS_STORAGE_UNKNOWN) {
>          ctx->mem1_locked = TRUE;
>          ctx->previous_mem1 = self->priv->current_sms_mem1_storage;
> -        self->priv->mem1_storage_locked = TRUE;
> +
>          self->priv->current_sms_mem1_storage = mem1;
> -        mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
> +        self->priv->mem1_storage_locked = TRUE;
> +    } else if (self->priv->current_sms_mem1_storage != MM_SMS_STORAGE_UNKNOWN) {
> +        mm_dbg ("Given sms mem1 storage is unknown. Using current sms mem1 storage value '%s' instead",
> +                mm_sms_storage_get_string (self->priv->current_sms_mem1_storage));
> +    } else {
> +        g_simple_async_result_set_error (ctx->result,
> +                                         MM_CORE_ERROR,
> +                                         MM_CORE_ERROR_RETRY,
> +                                        "Cannot lock mem2 storage alone when current mem1 storage is unknown");
> +        lock_sms_storages_context_complete_and_free (ctx);
> +        return;
>      }
> +    mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
>
>      if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
>          ctx->mem2_locked = TRUE;
>          ctx->previous_mem2 = self->priv->current_sms_mem2_storage;
> +
>          self->priv->mem2_storage_locked = TRUE;
>          self->priv->current_sms_mem2_storage = mem2;
> -        mem2_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem2_storage), -1);
>
> -        if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
> -            /* Some modems may not support empty string parameters. Then if mem1 is
> -             * UNKNOWN, we send again the already locked mem1 value in place of an
> -             * empty string. This way we also avoid to confuse the environment of
> -             * other async operation that have potentially locked mem1 previoulsy.
> -             * */
> -            mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
> -        }
> +        mem2_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem2_storage), -1);
>      }
>
> -    /* We don't touch 'mem3' here */
> +    g_assert (mem1_str != NULL);
>
> +    /* We don't touch 'mem3' here */
>      mm_dbg ("Locking SMS storages to: mem1 (%s), mem2 (%s)...",
> -            mem1_str ? mem1_str : "none",
> +            mem1_str,
>              mem2_str ? mem2_str : "none");
>
>      if (mem2_str)
> -        cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"",
> -                               mem1_str ? mem1_str : "",
> -                               mem2_str);
> -    else if (mem1_str)
> -        cmd = g_strdup_printf ("+CPMS=\"%s\"", mem1_str);
> +        cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\"", mem1_str, mem2_str);
>      else
> -        g_assert_not_reached ();
> +        cmd = g_strdup_printf ("+CPMS=\"%s\"", mem1_str);
>
>      mm_base_modem_at_command (MM_BASE_MODEM (self),
>                                cmd,
> @@ -5578,22 +5585,30 @@ 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,
> +                                             "Cannot set default storage when current mem1 storage is unknown");
> +        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);
> +
>      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


More information about the ModemManager-devel mailing list