[PATCH v2] core: process SMS +CGMR response with regex
Dan Williams
dcbw at redhat.com
Tue Sep 15 15:26:06 PDT 2015
On Tue, 2015-09-15 at 20:15 +0000, Nick Stevens wrote:
> Variability in the response style from certain modems causes the parsing
> of the +CGMR response to fail. For example, the Telit HE910 inserts an
> empty string ("") in the second field of the response, causing the
> sscanf implementation to fail.
>
> This patch converts the parsing of the CGMR response to a regex that
> allows for more flexibility and robustness, and adds unit tests around
> the parsing call.
>
> Signed-off-by: Nick Stevens <Nick.Stevens at digi.com>
> ---
> V1->V2:
> * Moved parsing logic to mm-modem-helpers.[ch]
> * Added unit tests for CGMR parsing call
> - Generic case based on CGML test case
> - Telit-specific case that contains extra empty string in <alpha> field
>
> src/mm-broadband-modem.c | 17 ++++------
> src/mm-modem-helpers.c | 73 +++++++++++++++++++++++++++++++++++++++++-
> src/mm-modem-helpers.h | 5 +++
> src/mm-sms-part-3gpp.h | 2 --
> src/tests/test-modem-helpers.c | 64 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 148 insertions(+), 13 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index 9093db5..2013b78 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5603,8 +5603,7 @@ sms_part_ready (MMBroadbandModem *self,
> SmsPartContext *ctx)
> {
> MMSmsPart *part;
> - gint rv, status, tpdu_len;
> - gchar pdu[MM_SMS_PART_3GPP_MAX_PDU_LEN + 1];
> + MM3gppPduInfo *info;
> const gchar *response;
> GError *error = NULL;
>
> @@ -5622,19 +5621,16 @@ 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) {
> - error = g_error_new (MM_CORE_ERROR,
> - MM_CORE_ERROR_FAILED,
> - "Failed to parse CMGR response (parsed %d items)", rv);
> - mm_warn ("Couldn't retrieve SMS part: '%s'", error->message);
> + info = mm_3gpp_parse_cmgr_read_response (response, ctx->idx, &error);
> + if (!info) {
> + mm_warn ("Couldn't parse SMS part: '%s'",
> + error->message);
> g_simple_async_result_take_error (ctx->result, error);
> sms_part_context_complete_and_free (ctx);
> return;
> }
>
> - part = mm_sms_part_3gpp_new_from_pdu (ctx->idx, pdu, &error);
> + part = mm_sms_part_3gpp_new_from_pdu (info->index, info->pdu, &error);
> if (part) {
> mm_dbg ("Correctly parsed PDU (%d)", ctx->idx);
> mm_iface_modem_messaging_take_part (MM_IFACE_MODEM_MESSAGING (self),
> @@ -5648,6 +5644,7 @@ sms_part_ready (MMBroadbandModem *self,
> }
>
> /* All done */
> + mm_3gpp_pdu_info_free (info);
> g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> sms_part_context_complete_and_free (ctx);
> }
> diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c
> index d40082d..ee31942 100644
> --- a/src/mm-modem-helpers.c
> +++ b/src/mm-modem-helpers.c
> @@ -1161,6 +1161,77 @@ mm_3gpp_parse_cmgf_test_response (const gchar *reply,
>
> /*************************************************************************/
>
> +MM3gppPduInfo *
> +mm_3gpp_parse_cmgr_read_response (const gchar *reply,
> + guint index,
> + GError **error)
> +{
> + GRegex *r;
> + GMatchInfo *match_info = NULL;
> + GError *match_error = NULL;
> + MM3gppPduInfo *info = NULL;
> +
> + /* +CMGR: <stat>,<alpha>,<length>(whitespace)<pdu> */
> + /* The <alpha> and <length> fields are matched, but not currently used */
> + r = g_regex_new ("\\+CMGR:\\s*(\\d+)\\s*,([^,]*),\\s*(\\d+)\\s*([^\\r\\n]*)", 0, 0, error);
> + g_assert (r != NULL);
You could also do g_assert_no_error (error) before the g_assert(r) too.
> +
> + if (g_regex_match_full (r, reply, strlen (reply), 0, 0, &match_info, &match_error)) {
I think you should also just restructure the function with a 'goto' for
error handling to break out early. Yeah it's a goto, but it's OK in
patterns like this for error handling.
Also, I'd have local variables for the 'status' and 'pdu' bits so that
we don't allocate the returned 'info' until we know it's going to be
valid, that way we don't have to free it in the error cases at all.
It's usually best not to look too much at 'error'; eg a function
shouldn't care whether the caller passed in NULL or a valid pointer to
an error, but the patch has "if (*error) info = NULL;". We can avoid
that by using the goto stuff.
gint status;
gchar *pdu = NULL;
if (!g_regex_match_full (r, reply, strlen (reply), 0, 0, &match_info, &match_error)) {
if (match_error)
g_propagate_error (error, match_error);
else {
g_set_error (...);
}
goto out;
}
count = g_match_info_get_match_count(match_info);
if (count != 5) {
g_set_error (...);
goto out;
}
if (!mm_get_int_from_match_info (match_info, 1, &status)) {
g_set_error (...);
goto out;
}
pdu = mm_get_string_unquoted_from_match_info (match_info, 4);
if (!info->pdu) {
g_set_error (...);
goto out;
}
/* success */
info = g_new0 (MM3gppPduInfo, 1);
info->index = index;
info->status = status;
info->pdu = pdu;
out:
g_match_info_free (match_info);
g_regex_unref (r);
return info;
Rest looks OK to me...
Thanks!
Dan
> + gint count = 0;
> +
> + /* g_match_info_get_match_count includes match #0 */
> + count = g_match_info_get_match_count (match_info);
> + if (count != 5) {
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to match CMGR fields (matched %d) '%s'",
> + count,
> + reply);
> + } else {
> + info = g_new0 (MM3gppPduInfo, 1);
> + info->index = index;
> + if (!mm_get_int_from_match_info (match_info, 1, &info->status)) {
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to extract CMGR status field '%s'",
> + reply);
> + g_free(info);
> + } else {
> + info->pdu = mm_get_string_unquoted_from_match_info (match_info, 4);
> + if (!(info->pdu)) {
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to extract CMGR pdu field '%s'",
> + reply);
> + g_free(info);
> + }
> + }
> +
> + }
> + } else if (match_error) {
> + g_propagate_error (error, match_error);
> + } else {
> + g_set_error (error,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Failed to parse CMGR read result: response didn't match '%s'",
> + reply);
> + }
> +
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> +
> + if (*error)
> + info = NULL;
> +
> + return info;
> +}
> +
> +/*************************************************************************/
> +
> static MMSmsStorage
> storage_from_str (const gchar *str)
> {
> @@ -1658,7 +1729,7 @@ done:
>
> /*************************************************************************/
>
> -static void
> +void
> mm_3gpp_pdu_info_free (MM3gppPduInfo *info)
> {
> g_free (info->pdu);
> diff --git a/src/mm-modem-helpers.h b/src/mm-modem-helpers.h
> index 0ec59af..edc33f9 100644
> --- a/src/mm-modem-helpers.h
> +++ b/src/mm-modem-helpers.h
> @@ -181,10 +181,15 @@ typedef struct {
> gint status;
> gchar *pdu;
> } MM3gppPduInfo;
> +void mm_3gpp_pdu_info_free (MM3gppPduInfo *info);
> void mm_3gpp_pdu_info_list_free (GList *info_list);
> GList *mm_3gpp_parse_pdu_cmgl_response (const gchar *str,
> GError **error);
>
> +/* AT+CMGR (Read message) response parser */
> +MM3gppPduInfo *mm_3gpp_parse_cmgr_read_response (const gchar *reply,
> + guint index,
> + GError **error);
>
> /* Additional 3GPP-specific helpers */
>
> 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);
> diff --git a/src/tests/test-modem-helpers.c b/src/tests/test-modem-helpers.c
> index b493e6b..db8fc89 100644
> --- a/src/tests/test-modem-helpers.c
> +++ b/src/tests/test-modem-helpers.c
> @@ -196,6 +196,67 @@ test_cmgl_response_pantech_multiple (void *f, gpointer d)
> }
>
> /*****************************************************************************/
> +/* Test CMGR responses */
> +
> +static void
> +test_cmgr_response (const gchar *str,
> + const MM3gppPduInfo *expected)
> +{
> + MM3gppPduInfo *info;
> + GError *error = NULL;
> +
> + info = mm_3gpp_parse_cmgr_read_response (str, 0, &error);
> + g_assert_no_error (error);
> + g_assert (info != NULL);
> +
> + /* Ignore index, it is not included in CMGR response */
> + g_assert_cmpint (info->status, ==, expected->status);
> + g_assert_cmpstr (info->pdu, ==, expected->pdu);
> +}
> +
> +static void
> +test_cmgr_response_generic (void *f, gpointer d)
> +{
> + const gchar *str =
> + "+CMGR: 1,,147 07914306073011F00405812261F700003130916191314095C27"
> + "4D96D2FBBD3E437280CB2BEC961F3DB5D76818EF2F0381D9E83E06F39A8CC2E9FD372F"
> + "77BEE0249CBE37A594E0E83E2F532085E2F93CB73D0B93CA7A7DFEEB01C447F93DF731"
> + "0BD3E07CDCB727B7A9C7ECF41E432C8FC96B7C32079189E26874179D0F8DD7E93C3A0B"
> + "21B246AA641D637396C7EBBCB22D0FD7E77B5D376B3AB3C07";
> +
> + const MM3gppPduInfo expected = {
> + .index = 0,
> + .status = 1,
> + .pdu = "07914306073011F00405812261F700003130916191314095C27"
> + "4D96D2FBBD3E437280CB2BEC961F3DB5D76818EF2F0381D9E83E06F39A8CC2E9FD372F"
> + "77BEE0249CBE37A594E0E83E2F532085E2F93CB73D0B93CA7A7DFEEB01C447F93DF731"
> + "0BD3E07CDCB727B7A9C7ECF41E432C8FC96B7C32079189E26874179D0F8DD7E93C3A0B"
> + "21B246AA641D637396C7EBBCB22D0FD7E77B5D376B3AB3C07"
> + };
> +
> + test_cmgr_response (str, &expected);
> +}
> +
> +/* Telit HE910 places empty quotation marks in the <alpha> field and a CR+LF
> + * before the PDU */
> +static void
> +test_cmgr_response_telit (void *f, gpointer d)
> +{
> + const gchar *str =
> + "+CMGR: 0,\"\",50\r\n07916163838428F9040B916121021021F7000051905141642"
> + "20A23C4B0BCFD5E8740C4B0BCFD5E83C26E3248196687C9A0301D440DBBC3677918";
> +
> + const MM3gppPduInfo expected = {
> + .index = 0,
> + .status = 0,
> + .pdu = "07916163838428F9040B916121021021F7000051905141642"
> + "20A23C4B0BCFD5E8740C4B0BCFD5E83C26E3248196687C9A0301D440DBBC3677918"
> + };
> +
> + test_cmgr_response (str, &expected);
> +}
> +
> +/*****************************************************************************/
> /* Test COPS responses */
>
> static void
> @@ -2497,6 +2558,9 @@ int main (int argc, char **argv)
> g_test_suite_add (suite, TESTCASE (test_cmgl_response_pantech, NULL));
> g_test_suite_add (suite, TESTCASE (test_cmgl_response_pantech_multiple, NULL));
>
> + g_test_suite_add (suite, TESTCASE (test_cmgr_response_generic, NULL));
> + g_test_suite_add (suite, TESTCASE (test_cmgr_response_telit, NULL));
> +
> g_test_suite_add (suite, TESTCASE (test_supported_mode_filter, NULL));
>
> g_test_suite_add (suite, TESTCASE (test_supported_capability_filter, NULL));
More information about the ModemManager-devel
mailing list