[PATCH] core: process SMS +CGMR response with regex

[Aleksander Morgado]

Hey,

On Tue, Sep 8, 2015 at 10:41 PM, Stevens, Nick <Nick.Stevens at digi.com> wrote:
> Variability in the response style from certain modems would cause the parsing
> of the +CGMR response to fail. For example, the Telit HE910 inserts an empty
> string ("") in the second field of the response, which caused the previous
> sscanf implementation to fail.
>
> This patch converts the parsing of the CGMR response to a regex that allows
> for more flexibility and robustness.
>
> Signed-off-by: Nick Stevens <Nick.Stevens at digi.com>

That's a good one. Could you move the logic to mm-modem-helpers.[ch]
and write some unit tests in test/test-modem-helpers.c ?

> ---
>  src/mm-broadband-modem.c | 63 +++++++++++++++++++++++++++++++++++++++++-------
>  src/mm-sms-part-3gpp.h   |  2 --
>  2 files changed, 54 insertions(+), 11 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 085d4d7..a6346f1 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5614,10 +5614,12 @@ sms_part_ready (MMBroadbandModem *self,
>                  SmsPartContext *ctx)
>  {
>      MMSmsPart *part;
> -    gint rv, status, tpdu_len;
> -    gchar pdu[MM_SMS_PART_3GPP_MAX_PDU_LEN + 1];
> +    gint matches, status, tpdu_len;
> +    gchar *pdu;
>      const gchar *response;
>      GError *error = NULL;
> +    GRegex *r = NULL;
> +    GMatchInfo *match_info = NULL;
>
>      /* Always always always unlock mem1 storage. Warned you've been. */
>      mm_broadband_modem_unlock_sms_storages (self, TRUE, FALSE);
> @@ -5633,16 +5635,51 @@ sms_part_ready (MMBroadbandModem *self,
>          return;
>      }
>
> -    rv = sscanf (response, "+CMGR: %d,,%d %" G_STRINGIFY (MM_SMS_PART_3GPP_MAX_PDU_LEN) "s",
> -                 &status, &tpdu_len, pdu);
> -    if (rv != 3) {
> +    /* +CMGR: <status>,(ignored),<tpdu_len>(whitespace)<data> */
> +    r = g_regex_new ("\\+CMGR:\\s*(\\d+)\\s*,[^,]*,\\s*(\\d+)\\s*([^\\r\\n]*)",
> +                     0, 0, NULL);
> +    g_assert (r);
> +
> +    if (!g_regex_match_full (r, response, strlen (response), 0, 0, &match_info, NULL)) {
>          error = g_error_new (MM_CORE_ERROR,
>                               MM_CORE_ERROR_FAILED,
> -                             "Failed to parse CMGR response (parsed %d items)", rv);
> +                             "Failed to parse CMGR response");
>          mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> -        g_simple_async_result_take_error (ctx->result, error);
> -        sms_part_context_complete_and_free (ctx);
> -        return;
> +        goto error;
> +    }
> +
> +    matches = g_match_info_get_match_count (match_info);
> +    if (matches != 4) {
> +        error = g_error_new (MM_CORE_ERROR,
> +                             MM_CORE_ERROR_FAILED,
> +                             "Failed to match entire CMGR response (count %d)", matches);
> +        mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> +        goto error;
> +    }
> +
> +    if (!mm_get_int_from_match_info (match_info, 1, &status)) {
> +        error = g_error_new (MM_CORE_ERROR,
> +                             MM_CORE_ERROR_FAILED,
> +                             "Failed to convert status");
> +        mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> +        goto error;
> +    }
> +
> +    if (!mm_get_int_from_match_info (match_info, 2, &tpdu_len)) {
> +        error = g_error_new (MM_CORE_ERROR,
> +                             MM_CORE_ERROR_FAILED,
> +                             "Failed to convert tpdu_len");
> +        mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> +        goto error;
> +    }
> +
> +    pdu = mm_get_string_unquoted_from_match_info (match_info, 3);
> +    if (!pdu) {
> +        error = g_error_new (MM_CORE_ERROR,
> +                             MM_CORE_ERROR_FAILED,
> +                             "Failed to get PDU");
> +        mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> +        goto error;
>      }
>
>      part = mm_sms_part_3gpp_new_from_pdu (ctx->idx, pdu, &error);
> @@ -5659,8 +5696,16 @@ sms_part_ready (MMBroadbandModem *self,
>      }
>
>      /* All done */
> +    g_regex_unref(r);
>      g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
>      sms_part_context_complete_and_free (ctx);
> +
> +    return;
> +
> +error:
> +    g_regex_unref(r);
> +    g_simple_async_result_take_error (ctx->result, error);
> +    sms_part_context_complete_and_free (ctx);
>  }
>
>  static void
> diff --git a/src/mm-sms-part-3gpp.h b/src/mm-sms-part-3gpp.h
> index 82709a2..a21d734 100644
> --- a/src/mm-sms-part-3gpp.h
> +++ b/src/mm-sms-part-3gpp.h
> @@ -22,8 +22,6 @@
>
>  #include "mm-sms-part.h"
>
> -#define MM_SMS_PART_3GPP_MAX_PDU_LEN 344
> -
>  MMSmsPart *mm_sms_part_3gpp_new_from_pdu  (guint index,
>                                             const gchar *hexpdu,
>                                             GError **error);
> --
> 2.5.0
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel



-- 
Aleksander
https://aleksander.es