[PATCH v4] Fixed wrong MEM1 value passed to +CPMS
Carlo Lobrano
c.lobrano at gmail.com
Mon Mar 27 15:50:29 UTC 2017
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.
>> + "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.
Sorry for missing the whitespace twice. Btw, do you have any automatic tool for checking the coding rules or just a good eye? :D
---
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
More information about the ModemManager-devel
mailing list