[PATCH] Fix Bug 93135 +CPMS emtpy parameter not always supported
Carlo Lobrano
c.lobrano at gmail.com
Mon Mar 7 16:13:20 UTC 2016
Hi Aleksander,
thanks for the quick reply. My comments are below, even if I mostly agree
with your observations
On 7 March 2016 at 16:15, Aleksander Morgado <aleksander at aleksander.es>
wrote:
> 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 :)
>
I will
>
> 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?
>
>
I'm actually considering now the fact that I need a mem1_str only when
mem2_str is not NULL.
I could keep the "if (mem1 != MM_SMS_STORAGE_UNKNOWN)" as it was before,
and do something like this
if (mem2 != MM_SMS_STORAGE_UNKNOWN) {
...
mem2_str = g_ascii_strup (mm_sms_storage_get_string
(self->priv->current_sms_mem2_storage), -1);
if (mem1 == MM_SMS_STORAGE_UNKNOWN) {
/* Verbose explanation */
mem1_str = g_ascii_strup(mm_sms_storage_get_string
(self->priv->current_sms_mem1_storage), -1);
}
}
This way the change will effect only the cases when mem2 only is to be
locked and the verbose explanation should be more clear to understand.
What do you think?
>
> > 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.
>
I'll rephrase that. I just wanted to say that we will use the current
value of mem1.
>
> > + 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?
>
init_current_storages it's fine for me.
>
>
> > /* 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 :)
>
I think it's a good way to express what one expects to get from the regex,
but that's just an opinion :)
>
> > +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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160307/1996c0eb/attachment-0001.html>
More information about the ModemManager-devel
mailing list