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 <[email protected]> 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, <[email protected]> wrote: > > From: Carlo Lobrano <[email protected]> > > > > - 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 > > [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
