[PATCH] Fix Bug 93135 +CPMS emtpy parameter not always supported
Aleksander Morgado
aleksander at aleksander.es
Mon Mar 7 15:15:43 UTC 2016
Hey,
>From now on, please implement all async calls using the newer GTask,
instead of GSimpleAsyncResult. No need to rework this patch for that,
just take it into consideration for future patches :)
See comments below.
On Mon, Mar 7, 2016 at 3:04 PM, <c.lobrano at gmail.com> wrote:
> From: Carlo Lobrano <c.lobrano at gmail.com>
>
> - Add new async virtual method load_current_storages to
> MMIfaceModemMessaging
> - Add logic of load_current_storages to MMBroadbandModem
> - Add step "LOAD_CURRENT_STORAGES" in MMIfaceModemMessaging
> initialization in order to load and store current SMS
> storages for mem1 and mem2.
> - Add usage of current sms storage value for mem1 in place
> of an empty string parameter when the command AT+CPMS
> is used.
> ---
> src/mm-broadband-modem.c | 89 +++++++++++++++++++++++++++++++++++++++++-
> src/mm-iface-modem-messaging.c | 37 ++++++++++++++++++
> src/mm-iface-modem-messaging.h | 11 ++++++
> src/mm-modem-helpers.c | 77 ++++++++++++++++++++++++++++++++++++
> src/mm-modem-helpers.h | 10 +++++
> src/tests/test-modem-helpers.c | 36 +++++++++++++++++
> 6 files changed, 258 insertions(+), 2 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 6fc0663..08c8f13 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5242,6 +5242,82 @@ modem_messaging_load_supported_storages (MMIfaceModemMessaging *self,
> }
>
> /*****************************************************************************/
> +/* Load current SMS storages (Messaging interface) */
> +
> +static gboolean
> +modem_messaging_load_current_storages_finish (MMIfaceModemMessaging *_self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + if (g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error))
> + return FALSE;
> +
> + return TRUE;
Just "return !g_simple_async_result_propagate_error (...);"
> +}
> +
> +static void
> +cpms_query_ready (MMBroadbandModem *self,
> + GAsyncResult *res,
> + GSimpleAsyncResult *simple)
> +{
> + const gchar *response;
> + GError *error = NULL;
> + MMSmsStorage mem1;
> + MMSmsStorage mem2;
> +
> + response = mm_base_modem_at_command_finish (MM_BASE_MODEM(self), res, &error);
> + if (error) {
> + g_simple_async_result_take_error (simple, error);
> + g_simple_async_result_complete (simple);
> + g_object_unref (simple);
> + return;
> + }
> +
> + /* Parse reply */
> + if (!mm_3gpp_parse_cpms_query_response (response,
> + &mem1,
> + &mem2,
> + &error)) {
> + g_simple_async_result_take_error (simple, error);
> + } else {
> + self->priv->current_sms_mem1_storage = mem1;
> + self->priv->current_sms_mem2_storage = mem2;
> +
> + mm_dbg ("Current storages loaded:");
> + mm_dbg (" mem1 (list/read/delete) storages: '%s'",
> + mm_common_build_sms_storages_string(&mem1, 1));
> + mm_dbg (" mem2 (write/send) storages: '%s'",
> + mm_common_build_sms_storages_string(&mem2, 1));
> + }
> +
> + g_simple_async_result_complete (simple);
> + g_object_unref (simple);
> +}
> +
> +static void
> +modem_messaging_load_current_storages (MMIfaceModemMessaging *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GSimpleAsyncResult *result;
> +
> + result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> + modem_messaging_load_current_storages);
> +
> + /* Check support storages */
> + mm_base_modem_at_command (MM_BASE_MODEM (self),
> + "+CPMS?",
> + 3,
> + TRUE,
> + (GAsyncReadyCallback)cpms_query_ready,
> + result);
> +}
> +
Too many empty lines here, just one enough.
> +
> +
> +/*****************************************************************************/
> /* Lock/unlock SMS storage (Messaging interface implementation helper)
> *
> * The basic commands to work with SMS storages play with AT+CPMS and three
> @@ -5373,8 +5449,9 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self,
> 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);
> }
> + /* We always provide a non empty string parameter as mem1 */
> + mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
>
This is a bit confusing actually. The fact that we're passing the mem1
value when e.g. trying to lock mem2 doesn't mean that we can be
changing the mem1 while some other async logic got it locked before. I
think your code is ok, but I spent 30 mins convinced that it wasn't,
I'm not 100% sure yet :) The key point is that If mem1 is locked for
an operation, no other operation that wants to lock mem1 can be run
(ok); but we can still run operations that want to lock mem2 only (and
in that case we're just re-sending the already locked mem1 value). Can
you try to explain that in the comment?
> if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
> ctx->mem2_locked = TRUE;
> @@ -5446,6 +5523,7 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
> MMBroadbandModem *self = MM_BROADBAND_MODEM (_self);
> gchar *cmd;
> GSimpleAsyncResult *result;
> + gchar *mem1_str;
> gchar *mem_str;
>
> result = g_simple_async_result_new (G_OBJECT (self),
> @@ -5456,8 +5534,13 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
> /* Set defaults as current */
> self->priv->current_sms_mem2_storage = storage;
>
> + /* Some modems do no support empty string as parameter.
> + * Here we keep the value or Memr as the one get with
> + * load_current_storages step */
Not sure I undestand the last sentence.
> + mem1_str = g_ascii_strup (mm_sms_storage_get_string (self->priv->current_sms_mem1_storage), -1);
> +
Oh, hum... I thought this was going to be telit-only.
Given that the new async method you wrote is optional (e.g. subclasses
can set it as NULL, and we also don't care if it fails), we still have
the possibility of seeing UNKNOWN in "current_sms_mem1_storage". I
would change the logic so that, if we see UNKNOWN in mem1, we use the
old method of providing the empty string.
> mem_str = g_ascii_strup (mm_sms_storage_get_string (storage), -1);
> - cmd = g_strdup_printf ("+CPMS=\"\",\"%s\",\"%s\"", mem_str, mem_str);
> + cmd = g_strdup_printf ("+CPMS=\"%s\",\"%s\",\"%s\"", mem1_str, mem_str, mem_str);
> mm_base_modem_at_command (MM_BASE_MODEM (self),
> cmd,
> 3,
> @@ -10390,6 +10473,8 @@ iface_modem_messaging_init (MMIfaceModemMessaging *iface)
> iface->cleanup_unsolicited_events = modem_messaging_cleanup_unsolicited_events;
> iface->cleanup_unsolicited_events_finish = modem_messaging_setup_cleanup_unsolicited_events_finish;
> iface->create_sms = modem_messaging_create_sms;
> + iface->load_current_storages = modem_messaging_load_current_storages;
> + iface->load_current_storages_finish = modem_messaging_load_current_storages_finish;
> }
>
> static void
> diff --git a/src/mm-iface-modem-messaging.c b/src/mm-iface-modem-messaging.c
> index d2ef6e8..5058569 100644
> --- a/src/mm-iface-modem-messaging.c
> +++ b/src/mm-iface-modem-messaging.c
> @@ -1057,6 +1057,7 @@ typedef enum {
> INITIALIZATION_STEP_CHECK_SUPPORT,
> INITIALIZATION_STEP_FAIL_IF_UNSUPPORTED,
> INITIALIZATION_STEP_LOAD_SUPPORTED_STORAGES,
> + INITIALIZATION_STEP_LOAD_CURRENT_STORAGES,
> INITIALIZATION_STEP_LAST
> } InitializationStep;
>
> @@ -1213,6 +1214,30 @@ check_support_ready (MMIfaceModemMessaging *self,
> }
>
> static void
> +load_current_storages_ready (MMIfaceModemMessaging *self,
> + GAsyncResult *res,
> + InitializationContext *ctx)
> +{
> + StorageContext *storage_ctx;
> + GError *error = NULL;
> +
> + storage_ctx = get_storage_context (self);
> + if (!MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (self)->load_current_storages_finish (
> + self,
> + res,
> + &error)) {
> + mm_dbg ("Couldn't load current storages: '%s'", error->message);
> + g_error_free (error);
> + } else {
> + mm_dbg ("Current storages loaded");
> + }
> +
> + /* Go on to next step */
> + ctx->step++;
> + interface_initialization_step (ctx);
> +}
> +
> +static void
> interface_initialization_step (InitializationContext *ctx)
> {
> /* Don't run new steps if we're cancelled */
> @@ -1284,6 +1309,18 @@ interface_initialization_step (InitializationContext *ctx)
> /* Fall down to next step */
> ctx->step++;
>
> + case INITIALIZATION_STEP_LOAD_CURRENT_STORAGES:
> + if (MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->load_current_storages &&
> + MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->load_current_storages_finish) {
> + MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->load_current_storages (
> + ctx->self,
> + (GAsyncReadyCallback)load_current_storages_ready,
> + ctx);
> + return;
> + }
> + /* Fall down to next step */
> + ctx->step++;
> +
> case INITIALIZATION_STEP_LAST:
> /* We are done without errors! */
>
> diff --git a/src/mm-iface-modem-messaging.h b/src/mm-iface-modem-messaging.h
> index c27e100..0a289df 100644
> --- a/src/mm-iface-modem-messaging.h
> +++ b/src/mm-iface-modem-messaging.h
> @@ -62,6 +62,17 @@ struct _MMIfaceModemMessaging {
> GArray **mem2,
> GArray **mem3,
> GError **error);
> + /* Load current storages for...
> + * mem1: listing/reading/deleting
> + * mem2: writing/sending
> + * mem3: receiving
> + */
> + void (* load_current_storages) (MMIfaceModemMessaging *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data);
> + gboolean (*load_current_storages_finish) (MMIfaceModemMessaging *self,
> + GAsyncResult *res,
> + GError **error);
>
Humm.. given that we're not returning the storage values for
mem1/mem2, maybe we should rename the method to
"init_current_storages" or something like that. The key point here is
to make it clear that this is not a async method to load something
that we're exposing in the DBus API, but an async method which does
some internal initialization in the actual implementation. What do you
think?
> /* Set default storage (async) */
> void (* set_default_storage) (MMIfaceModemMessaging *self,
> diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c
> index 58394ee..3c64263 100644
> --- a/src/mm-modem-helpers.c
> +++ b/src/mm-modem-helpers.c
> @@ -1507,6 +1507,83 @@ mm_3gpp_parse_cpms_test_response (const gchar *reply,
> return FALSE;
> }
>
> +/**********************************************************************
> + * AT+CPMS?
> + * +CPMS: <memr>,<usedr>,<totalr>,<memw>,<usedw>,<totalw>, <mems>,<useds>,<totals>
> + */
> +
> +#define CPMS_QUERY_REGEX "\\+CPMS:\\s*\"(?P<memr>.*)\",[0-9]+,[0-9]+,\"(?P<memw>.*)\",[0-9]+,[0-9]+,\"(?P<mems>.*)\",[0-9]+,[0-9]"
> +
Named fields in regex, not sure if I love it or hate it :)
> +gboolean
> +mm_3gpp_parse_cpms_query_response (const gchar *reply,
> + MMSmsStorage *memr,
> + MMSmsStorage *memw,
> + GError **error)
> +{
> + GRegex *r = NULL;
> + gboolean ret = FALSE;
> + GMatchInfo *match_info = NULL;
> +
> + r = g_regex_new (CPMS_QUERY_REGEX, G_REGEX_RAW, 0, NULL);
> +
> + g_assert(r);
> +
> + if (!g_regex_match (r, reply, 0, &match_info)) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not parse CPMS query reponse '%s'", reply);
> + goto end;
> + }
> +
> + if (!g_match_info_matches(match_info)) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not find matches in CPMS query reply '%s'", reply);
> + goto end;
> + }
> +
> + if (!mm_3gpp_get_cpms_storage_match (match_info, "memr", memr, error)) {
> + goto end;
> + }
> +
> + if (!mm_3gpp_get_cpms_storage_match (match_info, "memw", memw, error)) {
> + goto end;
> + }
> +
> + ret = TRUE;
> +
> +end:
> + if (r != NULL)
> + g_regex_unref (r);
> +
> + if (match_info != NULL)
> + g_match_info_free (match_info);
> +
> + return ret;
> +}
> +
> +gboolean
> +mm_3gpp_get_cpms_storage_match (GMatchInfo *match_info,
> + const gchar *match_name,
> + MMSmsStorage *storage,
> + GError **error)
> +{
> + gboolean ret = TRUE;
> + gchar *str = NULL;
> +
> + str = g_match_info_fetch_named(match_info, match_name);
> + if (str == NULL || str[0] == '\0') {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not find '%s' from CPMS reply", match_name);
> + ret = FALSE;
> + } else {
> + *storage = storage_from_str (str);
> + }
> +
> + if (str != NULL)
> + g_free (str);
No need the if(); g_free(NULL) is perfectly valid.
> +
> + return ret;
> +}
> +
> /*************************************************************************/
>
> gboolean
> diff --git a/src/mm-modem-helpers.h b/src/mm-modem-helpers.h
> index 975a493..476b315 100644
> --- a/src/mm-modem-helpers.h
> +++ b/src/mm-modem-helpers.h
> @@ -158,6 +158,16 @@ gboolean mm_3gpp_parse_cpms_test_response (const gchar *reply,
> GArray **mem2,
> GArray **mem3);
>
> +/* AT+CPMS? (Current SMS storage) response parser */
> +gboolean mm_3gpp_parse_cpms_query_response (const gchar *reply,
> + MMSmsStorage *mem1,
> + MMSmsStorage *mem2,
> + GError** error);
> +gboolean mm_3gpp_get_cpms_storage_match (GMatchInfo *match_info,
> + const gchar *match_name,
> + MMSmsStorage *storage,
> + GError **error);
> +
> /* AT+CSCS=? (Supported charsets) response parser */
> gboolean mm_3gpp_parse_cscs_test_response (const gchar *reply,
> MMModemCharset *out_charsets);
> diff --git a/src/tests/test-modem-helpers.c b/src/tests/test-modem-helpers.c
> index db84f01..837317f 100644
> --- a/src/tests/test-modem-helpers.c
> +++ b/src/tests/test-modem-helpers.c
> @@ -2070,6 +2070,41 @@ test_cpms_response_empty_fields (void *f, gpointer d)
> g_array_unref (mem3);
> }
>
> +typedef struct {
> + const gchar *query;
> + MMSmsStorage mem1_want;
> + MMSmsStorage mem2_want;
> +} CpmsQueryTest;
> +
> +CpmsQueryTest cpms_query_test[] = {
> + {"+CPMS: \"ME\",1,100,\"MT\",5,100,\"TA\",1,100", 2, 3},
> + {"+CPMS: \"SM\",100,100,\"SR\",5,10,\"TA\",1,100", 1, 4},
> + {"+CPMS: \"XX\",100,100,\"BM\",5,10,\"TA\",1,100", 0, 5},
> + {"+CPMS: \"XX\",100,100,\"YY\",5,10,\"TA\",1,100", 0, 0},
> + {NULL, 0, 0}
> +};
> +
> +static void
> +test_cpms_query_response (void *f, gpointer d) {
> + MMSmsStorage mem1;
> + MMSmsStorage mem2;
> + gboolean ret;
> + GError *error;
GError must always be initialized to NULL before using it:
GError *error = NULL;
> + int i;
> +
> + for (i = 0; cpms_query_test[i].query != NULL; i++){
> + ret = mm_3gpp_parse_cpms_query_response (cpms_query_test[i].query,
> + &mem1,
> + &mem2,
> + &error);
> +
No need for this whiteline here.
> + g_assert(ret);
I'd include:
g_assert_no_error (error);
> + g_assert_cmpuint (cpms_query_test[i].mem1_want, ==, mem1);
> + g_assert_cmpuint (cpms_query_test[i].mem2_want, ==, mem2);
> + }
> +
No need for this whiteline here.
> +}
> +
> /*****************************************************************************/
> /* Test CNUM responses */
>
> @@ -2821,6 +2856,7 @@ int main (int argc, char **argv)
> g_test_suite_add (suite, TESTCASE (test_cpms_response_mixed, NULL));
> g_test_suite_add (suite, TESTCASE (test_cpms_response_mixed_spaces, NULL));
> g_test_suite_add (suite, TESTCASE (test_cpms_response_empty_fields, NULL));
> + g_test_suite_add (suite, TESTCASE (test_cpms_query_response, NULL));
>
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_single, NULL));
> g_test_suite_add (suite, TESTCASE (test_cgdcont_test_response_multiple, NULL));
> --
> 2.1.4
>
> _______________________________________________
> 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