[PATCH v3] Fixed wrong MEM1 value passed to +CPMS
Aleksander Morgado
aleksander at aleksander.es
Mon Mar 27 11:22:37 UTC 2017
Hey Carlo,
On 27/03/17 12:19, Carlo Lobrano 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.
>
See comments below.
> ---
>
> src/mm-broadband-modem.c | 79 ++++++++++++++++++++++++++----------------
> src/mm-iface-modem-messaging.c | 2 +-
> 2 files changed, 50 insertions(+), 31 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 302fc3d..c15f18d 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -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,55 @@ 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_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().
> + 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);
>
Whitespace missing before the parenthesis.
> + /* 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 +5588,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?)");
Same here with the error message text, maybe something like "cannot set
default storage when current mem1 storage is unknown". And I'd also
avoid the "(SIM busy?)" thing, as that is just an assumption that may
not always be true.
> + 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);
> +
> +
Single whiteline here, not two.
> 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);
> }
>
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list