[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