Hey, Looks very well, but some minor things to fix, see comments below.
Thanks! On Tue, Mar 8, 2016 at 10:11 AM, <[email protected]> wrote: > From: Carlo Lobrano <[email protected]> > > - 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel -- Aleksander https://aleksander.es _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
