On Tue, Sep 15, 2015 at 05:26:06PM -0500, Dan Williams wrote:
> 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.stev...@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.
>
I am going to go with Aleksander's later suggestion to just pass NULL to the
g_regex_new - checking the error here isn't adding anything.
> > +
> > + 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.
That's kind of funny, because I actually had structured v1 of the patch with
gotos. When I moved the code to mm-modem-helpers, I noticed that many of the
parse functions seemed to avoid goto (mm-3gpp_parse_cgdcont_read_response as
an arbitrary example), so I was trying to match the style of that module. I'm
definitely okay switching to gotos for this patch, so long as we're okay with
having the different style. Thoughts?
>
> 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
Thanks for the review!
Nick
> > + 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));
>
>
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
