[PATCH v2] Fix Bug 93135 +CPMS emtpy parameter support
Aleksander Morgado
aleksander at aleksander.es
Wed Mar 9 09:59:05 UTC 2016
Hey,
Looks very well, but some minor things to fix, see comments below.
Thanks!
On Tue, Mar 8, 2016 at 10:11 AM, <c.lobrano at gmail.com> wrote:
> From: Carlo Lobrano <c.lobrano at gmail.com>
>
> - Add new async virtual method init_current_storages to
> MMIfaceModemMessaging
> - Add logic of init_current_storages to MMBroadbandModem
> - Add step "INIT_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 | 91 +++++++++++++++++++++++++++++++++++++++++-
> src/mm-iface-modem-messaging.c | 37 +++++++++++++++++
> src/mm-iface-modem-messaging.h | 11 +++++
> src/mm-modem-helpers.c | 76 +++++++++++++++++++++++++++++++++++
> src/mm-modem-helpers.h | 10 +++++
> src/tests/test-modem-helpers.c | 35 ++++++++++++++++
> 6 files changed, 259 insertions(+), 1 deletion(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 6fc0663..24f2108 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5242,6 +5242,77 @@ modem_messaging_load_supported_storages (MMIfaceModemMessaging *self,
> }
>
> /*****************************************************************************/
> +/* Load current SMS storages (Messaging interface) */
> +
> +static gboolean
> +modem_messaging_init_current_storages_finish (MMIfaceModemMessaging *_self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), 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);
Whitespace before parenthesis missing.
> + 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));
Whitespace before parenthesis missing.
> + mm_dbg (" mem2 (write/send) storages: '%s'",
> + mm_common_build_sms_storages_string(&mem2, 1));
Whitespace before parenthesis missing.
> + }
> +
> + g_simple_async_result_complete (simple);
> + g_object_unref (simple);
> +}
> +
> +static void
> +modem_messaging_init_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_init_current_storages);
> +
> + /* Check support storages */
> + mm_base_modem_at_command (MM_BASE_MODEM (self),
> + "+CPMS?",
> + 3,
> + TRUE,
> + (GAsyncReadyCallback)cpms_query_ready,
> + result);
> +}
> +
> +/*****************************************************************************/
> /* Lock/unlock SMS storage (Messaging interface implementation helper)
> *
> * The basic commands to work with SMS storages play with AT+CPMS and three
> @@ -5382,6 +5453,15 @@ mm_broadband_modem_lock_sms_storages (MMBroadbandModem *self,
> 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);
> + }
> }
>
> /* We don't touch 'mem3' here */
> @@ -5446,6 +5526,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 +5537,14 @@ modem_messaging_set_default_storage (MMIfaceModemMessaging *_self,
> /* 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);
> +
mem1_str is leaking; should be g_free-d before the function exits.
> 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 ? mem1_str : "",
> + mem_str,
> + mem_str);
> mm_base_modem_at_command (MM_BASE_MODEM (self),
> cmd,
> 3,
> @@ -10390,6 +10477,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->init_current_storages = modem_messaging_init_current_storages;
> + iface->init_current_storages_finish = modem_messaging_init_current_storages_finish;
> }
>
> static void
> diff --git a/src/mm-iface-modem-messaging.c b/src/mm-iface-modem-messaging.c
> index d2ef6e8..2bc782f 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_INIT_CURRENT_STORAGES,
> INITIALIZATION_STEP_LAST
> } InitializationStep;
>
> @@ -1213,6 +1214,30 @@ check_support_ready (MMIfaceModemMessaging *self,
> }
>
> static void
> +init_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)->init_current_storages_finish (
> + self,
> + res,
> + &error)) {
> + mm_dbg ("Couldn't load current storages: '%s'", error->message);
s/load/initialize
> + g_error_free (error);
> + } else {
> + mm_dbg ("Current storages loaded");
> + }
s/loaded/initialized
> +
> + /* 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_INIT_CURRENT_STORAGES:
> + if (MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->init_current_storages &&
> + MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->init_current_storages_finish) {
> + MM_IFACE_MODEM_MESSAGING_GET_INTERFACE (ctx->self)->init_current_storages (
> + ctx->self,
> + (GAsyncReadyCallback)init_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..9609241 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
> + */
The previous comment should state what the step does, something like
"initializes state of the storages" or something, no need to specify
what mem1/mem2/mem3 are because we're not returning them here.
> + void (* init_current_storages) (MMIfaceModemMessaging *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data);
> + gboolean (*init_current_storages_finish) (MMIfaceModemMessaging *self,
> + GAsyncResult *res,
> + GError **error);
>
> /* 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..74e5546 100644
> --- a/src/mm-modem-helpers.c
> +++ b/src/mm-modem-helpers.c
> @@ -1507,6 +1507,82 @@ 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]"
> +
> +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);
> + }
> +
> + g_free (str);
> +
> + 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..ddd25af 100644
> --- a/src/tests/test-modem-helpers.c
> +++ b/src/tests/test-modem-helpers.c
> @@ -2070,6 +2070,40 @@ 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 = 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);
> + g_assert(ret);
> + 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);
> + }
> +}
> +
> /*****************************************************************************/
> /* Test CNUM responses */
>
> @@ -2821,6 +2855,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