<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Pushed to git master. I also pushed a follow up commit to use mm_get_uint_from_hex_str(): <a href="https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=d09bc8baaa9fe93a72bb715530b1403a7a81c891" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=d09bc8baaa9fe93a72bb715530b1403a7a81c891</a> Thanks!</div></blockquote><div> <div style="font-family:arial,helvetica,sans-serif;display:inline" class="gmail_default">Awesome, thanks to you!</div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On 19 April 2017 at 10:46, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 19/04/17 10:00, Carlo Lobrano wrote: > - Refactored mm_telit_parse_csim_response in order to correctly > recognize the following +CSIM error codes: > * 6300 - Verification failed > * 6983 - Authentication method blocked > * 6984 - Reference data invalidated > * 6A86 - Incorrect parameters > * 6A88 - Reference data not found > > - Updated correspondent tests. > - Finally, some minor changes in other files for better error logging > > Fixes: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=100374" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=100374</a> Pushed to git master. I also pushed a follow up commit to use mm_get_uint_from_hex_str(): <a href="https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=d09bc8baaa9fe93a72bb715530b1403a7a81c891" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=d09bc8baaa9fe93a72bb715530b1403a7a81c891</a> Thanks! <div><div class="h5"> > --- > plugins/telit/mm-broadband-modem-telit.c | 6 +- > plugins/telit/mm-modem-helpers-telit.c | 98 ++++++++++++++++------- > plugins/telit/mm-modem-helpers-telit.h | 3 +- > plugins/telit/tests/test-mm-modem-helpers-telit.c | 73 ++++++++--------- > 4 files changed, 104 insertions(+), 76 deletions(-) > > diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c > index 8b87310..d154fc6 100644 > --- a/plugins/telit/mm-broadband-modem-telit.c > +++ b/plugins/telit/mm-broadband-modem-telit.c > @@ -541,13 +541,13 @@ csim_query_ready (MMBaseModem *self, > response = mm_base_modem_at_command_finish (self, res, &error); > > if (!response) { > - mm_warn ("No respose for step %d: %s", ctx->step, error->message); > + mm_warn ("load unlock retries: no respose for step %d: %s", ctx->step, error->message); > g_error_free (error); > goto next_step; > } > > - if ( (unlock_retries = mm_telit_parse_csim_response (ctx->step, response, &error)) < 0) { > - mm_warn ("Parse error in step %d: %s.", ctx->step, error->message); > + if ( (unlock_retries = mm_telit_parse_csim_response (response, &error)) < 0) { > + mm_warn ("load unlock retries: parse error in step %d: %s.", ctx->step, error->message); > g_error_free (error); > goto next_step; > } > diff --git a/plugins/telit/mm-modem-helpers-telit.c b/plugins/telit/mm-modem-helpers-telit.c > index c8c1f4b..1dd2542 100644 > --- a/plugins/telit/mm-modem-helpers-telit.c > +++ b/plugins/telit/mm-modem-helpers-telit.c > @@ -17,6 +17,7 @@ > #include <stdlib.h> > #include <string.h> > #include <stdio.h> > +#include <errno.h> > > #include <ModemManager.h> > #define _LIBMM_INSIDE_MMCLI > @@ -113,54 +114,91 @@ mm_telit_get_band_flag (GArray *bands_array, > > /*****************************************************************************/ > /* +CSIM response parser */ > +#define MM_TELIT_MIN_SIM_RETRY_HEX 0x63C0 > +#define MM_TELIT_MAX_SIM_RETRY_HEX 0x63CF > > gint > -mm_telit_parse_csim_response (const guint step, > - const gchar *response, > +mm_telit_parse_csim_response (const gchar *response, > GError **error) > { > - GRegex *r = NULL; > GMatchInfo *match_info = NULL; > - gchar *retries_hex_str; > - guint retries; > + GRegex *r = NULL; > + gchar *str_code = NULL; > + gint retries = -1; > + guint64 hex_code = 0x0; > + GError *inner_error = NULL; > > - r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*63C(.*)\"", G_REGEX_RAW, 0, NULL); > + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*\".*([0-9a-fA-F]{4})\"", G_REGEX_RAW, 0, NULL); > + g_regex_match (r, response, 0, &match_info); > > - if (!g_regex_match (r, response, 0, &match_info)) { > - g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > - "Could not parse reponse '%s'", response); > - g_match_info_free (match_info); > - g_regex_unref (r); > - return -1; > + if (!g_match_info_matches (match_info)) { > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "Could not recognize +CSIM response '%s'", response); > + goto out; > } > > - if (!g_match_info_matches (match_info)) { > - g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > - "Could not find matches in response '%s'", response); > - g_match_info_free (match_info); > - g_regex_unref (r); > - return -1; > + str_code = mm_get_string_unquoted_from_match_info (match_info, 1); > + if (str_code == NULL) { > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "Could not find expected string code in response '%s'", response); > + goto out; > } > > - retries_hex_str = mm_get_string_unquoted_from_match_info (match_info, 1); > - g_assert (NULL != retries_hex_str); > + errno = 0; > + hex_code = g_ascii_strtoull (str_code, NULL, 16); > + if (hex_code == G_MAXUINT64 && errno == ERANGE) { > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "Could not recognize expected hex code in response '%s'", response); > + goto out; > + } > > - /* convert hex value to uint */ > - if (sscanf (retries_hex_str, "%x", &retries) != 1) { > - g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > - "Could not get retry value from match '%s'", > - retries_hex_str); > - g_match_info_free (match_info); > - g_regex_unref (r); > - return -1; > + switch (hex_code) { > + case 0x6300: > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "SIM verification failed"); > + goto out; > + case 0x6983: > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "SIM authentication method blocked"); > + goto out; > + case 0x6984: > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "SIM reference data invalidated"); > + goto out; > + case 0x6A86: > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "Incorrect parameters in SIM request"); > + goto out; > + case 0x6A88: > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "SIM reference data not found"); > + goto out; > + default: > + break; > } > > - g_free (retries_hex_str); > - g_match_info_free (match_info); > + if (hex_code < MM_TELIT_MIN_SIM_RETRY_HEX || hex_code > MM_TELIT_MAX_SIM_RETRY_HEX) { > + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > + "Unknown error returned '0x%04lx'", hex_code); > + goto out; > + } > + > + retries = (gint)(hex_code - MM_TELIT_MIN_SIM_RETRY_HEX); > + > +out: > g_regex_unref (r); > + g_match_info_free (match_info); > + g_free (str_code); > + > + if (inner_error) { > + g_propagate_error (error, inner_error); > + return -1; > + } > > + g_assert (retries >= 0); > return retries; > } > + > #define SUPP_BAND_RESPONSE_REGEX "#BND:\\s*\\((?P<Bands2G>[0-9\\-,]*)\\)(,\\s*\\((?P<Bands3G>[0-9\\-,]*)\\))?(,\\s*\\((?P<Bands4G>[0-9\\-,]*)\\))?" > #define CURR_BAND_RESPONSE_REGEX "#BND:\\s*(?P<Bands2G>\\d+)(,\\s*(?P<Bands3G>\\d+))?(,\\s*(?P<Bands4G>\\d+))?" > > diff --git a/plugins/telit/mm-modem-helpers-telit.h b/plugins/telit/mm-modem-helpers-telit.h > index 5f0e880..b693732 100644 > --- a/plugins/telit/mm-modem-helpers-telit.h > +++ b/plugins/telit/mm-modem-helpers-telit.h > @@ -61,8 +61,7 @@ typedef struct { > } TelitToMMBandMap; > > /* +CSIM response parser */ > -gint mm_telit_parse_csim_response (const guint step, > - const gchar *response, > +gint mm_telit_parse_csim_response (const gchar *response, > GError **error); > > typedef enum { > diff --git a/plugins/telit/tests/test-mm-modem-helpers-telit.c b/plugins/telit/tests/test-mm-modem-helpers-telit.c > index 40e3628..3008010 100644 > --- a/plugins/telit/tests/test-mm-modem-helpers-telit.c > +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c > @@ -28,66 +28,57 @@ > typedef struct { > gchar *response; > gint result; > + gchar *error_message; > } CSIMResponseTest; > > -static CSIMResponseTest valid_csim_response_test_list [] = { > +static CSIMResponseTest csim_response_test_list [] = { > /* The parser expects that 2nd arg contains > * substring "63Cx" where x is an HEX string > * representing the retry value */ > - {"+CSIM:8,\"xxxx63C1\"", 1}, > - {"+CSIM:8,\"xxxx63CA\"", 10}, > - {"+CSIM:8,\"xxxx63CF\"", 15}, > + {"+CSIM:8,\"000063C1\"", 1, NULL}, > + {"+CSIM:8,\"000063CA\"", 10, NULL}, > + {"+CSIM:8,\"000063CF\"", 15, NULL}, > /* The parser accepts spaces */ > - {"+CSIM:8,\"xxxx63C1\"", 1}, > - {"+CSIM:8, \"xxxx63C1\"", 1}, > - {"+CSIM: 8, \"xxxx63C1\"", 1}, > - {"+CSIM: 8, \"xxxx63C1\"", 1}, > + {"+CSIM:8, \"000063C1\"", 1, NULL}, > + {"+CSIM: 8, \"000063C1\"", 1, NULL}, > + {"+CSIM: 8, \"000063C1\"", 1, NULL}, > /* the parser expects an int as first argument (2nd arg's length), > * but does not check if it is correct */ > - {"+CSIM: 10, \"63CF\"", 15}, > - /* the parser expect a quotation mark at the end > - * of the response, but not at the begin */ > - {"+CSIM: 10, 63CF\"", 15}, > - { NULL, -1} > -}; > - > -static CSIMResponseTest invalid_csim_response_test_list [] = { > - /* Test missing final quotation mark */ > - {"+CSIM: 8, xxxx63CF", -1}, > - /* Negative test for substring "63Cx" */ > - {"+CSIM: 4, xxxx73CF\"", -1}, > - /* Test missing first argument */ > - {"+CSIM:xxxx63CF\"", -1}, > - { NULL, -1} > + {"+CSIM: 10, \"63CF\"", 15, NULL}, > + /* Valid +CSIM Error codes */ > + {"+CSIM: 4, \"6300\"", -1, "SIM verification failed"}, > + {"+CSIM: 4, \"6983\"", -1, "SIM authentication method blocked"}, > + {"+CSIM: 4, \"6984\"", -1, "SIM reference data invalidated"}, > + {"+CSIM: 4, \"6A86\"", -1, "Incorrect parameters in SIM request"}, > + {"+CSIM: 4, \"6A88\"", -1, "SIM reference data not found"}, > + /* Test error: missing first argument */ > + {"+CSIM:000063CF\"", -1, "Could not recognize +CSIM response '+CSIM:000063CF\"'"}, > + /* Test error: missing quotation mark */ > + {"+CSIM: 8, 000063CF", -1, "Could not recognize +CSIM response '+CSIM: 8, 000063CF'"}, > + /* Test generic error */ > + {"+CSIM: 4, \"63BF\"", -1, "Unknown error returned '0x63bf'"}, > + {"+CSIM: 4, \"63D0\"", -1, "Unknown error returned '0x63d0'"} > }; > > static void > test_mm_telit_parse_csim_response (void) > { > - const gint step = 1; > guint i; > gint res; > GError* error = NULL; > > - /* Test valid responses */ > - for (i = 0; valid_csim_response_test_list[i].response != NULL; i++) { > - res = mm_telit_parse_csim_response (step, valid_csim_response_test_list[i].response, &error); > - > - g_assert_no_error (error); > - g_assert_cmpint (res, ==, valid_csim_response_test_list[i].result); > - } > - > - /* Test invalid responses */ > - for (i = 0; invalid_csim_response_test_list[i].response != NULL; i++) { > - res = mm_telit_parse_csim_response (step, invalid_csim_response_test_list[i].response, &error); > + for (i = 0; i < G_N_ELEMENTS (csim_response_test_list); i++) { > + res = mm_telit_parse_csim_response (csim_response_test_list[i].response, &error); > > - g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED); > - g_assert_cmpint (res, ==, invalid_csim_response_test_list[i].result); > - > - if (NULL != error) { > - g_error_free (error); > - error = NULL; > + if (csim_response_test_list[i].error_message == NULL) { > + g_assert_no_error (error); > + } else { > + g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED); > + g_assert_cmpstr (error->message, ==, csim_response_test_list[i].error_message); > + g_clear_error (&error); > } > + > + g_assert_cmpint (res, ==, csim_response_test_list[i].result); > } > } > > -- </div></div>Aleksander <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </blockquote></div> </div>