Sure, good point! At least for the error codes it's easy, I'll think something for the hex with the retry value as well. I'll update the patch
Carlo Il lunedì 3 aprile 2017, Dan Williams <d...@redhat.com> ha scritto: > On Mon, 2017-04-03 at 17:01 +0200, Carlo Lobrano wrote: > > Updated 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 > > > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=100374 > > --- > > > > As a side note, I observed that sometimes the modem replies with > > error > > code > > > > 6A86 - Incorrect parameters > > > > when #QSS: 3 has not been received yet. This seems to be a modem's > > bug > > because the very same request is accepted as correct when issued > > later, > > namely when the SIM is ready. > > > > --- > > plugins/telit/mm-modem-helpers-telit.c | 96 > > ++++++++++++++++++++--- > > plugins/telit/tests/test-mm-modem-helpers-telit.c | 43 +++++++--- > > 2 files changed, 113 insertions(+), 26 deletions(-) > > > > diff --git a/plugins/telit/mm-modem-helpers-telit.c > > b/plugins/telit/mm-modem-helpers-telit.c > > index c8c1f4b..cc8cf0a 100644 > > --- a/plugins/telit/mm-modem-helpers-telit.c > > +++ b/plugins/telit/mm-modem-helpers-telit.c > > @@ -121,45 +121,115 @@ mm_telit_parse_csim_response (const guint > > step, > > { > > GRegex *r = NULL; > > GMatchInfo *match_info = NULL; > > - gchar *retries_hex_str; > > - guint retries; > > + gchar *error_code = NULL; > > + gchar *retries_hex_str = NULL; > > + gint rtv = -1; > > > > + /* Check for error codes */ > > + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*6300\"", G_REGEX_RAW, > > 0, NULL); > > + if (g_regex_match (r, response, 0, &match_info)) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "SIM verification failed"); > > + goto out; > > + } > > Can we do a single regex, capture the 4-hex-character response as part > of the regex, run g_ascii_strtoull() on it to get a uint, and then use > switch() to compare to known error codes instead? > > Dan > > > + g_match_info_free (match_info); > > + match_info = NULL; > > + g_regex_unref (r); > > + r = NULL; > > + > > + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*69(.*)\"", > > G_REGEX_RAW, 0, NULL); > > + if (g_regex_match (r, response, 0, &match_info) && > > + match_info != NULL && > > + g_match_info_matches (match_info)) { > > + error_code = mm_get_string_unquoted_from_match_info > > (match_info, 1); > > + > > + if (error_code == NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "Could not parse CSIM error code in > > response '%s'", > > + response); > > + } else if (strstr (error_code, "83") != NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "SIM authentication method blocked"); > > + } else if (strstr (error_code, "84") != NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "SIM reference data invalidated"); > > + } else { > > + g_set_error (error, MM_CORE_ERROR, > > MM_CORE_ERROR_FAILED, > > + "Unknown error code '69%s'", error_code); > > + } > > + > > + goto out; > > + } > > + > > + g_match_info_free (match_info); > > + match_info = NULL; > > + g_regex_unref (r); > > + r = NULL; > > + > > + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*6A(.*)\"", > > G_REGEX_RAW, 0, NULL); > > + if (g_regex_match (r, response, 0, &match_info) && > > + match_info != NULL && > > + g_match_info_matches (match_info)) { > > + error_code = mm_get_string_unquoted_from_match_info > > (match_info, 1); > > + > > + if (error_code == NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "Could not parse CSIM error code in > > response '%s'", > > + response); > > + } else if (strstr (error_code, "86") != NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "Incorrect parameters in SIM request"); > > + } else if (strstr (error_code, "88") != NULL) { > > + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, > > + "SIM reference data not found"); > > + } else { > > + g_set_error (error, MM_CORE_ERROR, > > MM_CORE_ERROR_FAILED, > > + "Unknown error code '6A%s'", error_code); > > + } > > + > > + goto out; > > + } > > + > > + g_match_info_free (match_info); > > + match_info = NULL; > > + g_regex_unref (r); > > + r = NULL; > > + > > + /* No errors found. Get SIM retries */ > > r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*63C(.*)\"", > > G_REGEX_RAW, 0, NULL); > > > > 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; > > + 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; > > + goto out; > > } > > > > retries_hex_str = mm_get_string_unquoted_from_match_info > > (match_info, 1); > > g_assert (NULL != retries_hex_str); > > > > /* convert hex value to uint */ > > - if (sscanf (retries_hex_str, "%x", &retries) != 1) { > > + if (sscanf (retries_hex_str, "%x", &rtv) != 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; > > + goto out; > > } > > > > g_free (retries_hex_str); > > + > > +out: > > + g_free (error_code), > > g_match_info_free (match_info); > > g_regex_unref (r); > > > > - return retries; > > + return rtv; > > } > > #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/tests/test-mm-modem-helpers-telit.c > > b/plugins/telit/tests/test-mm-modem-helpers-telit.c > > index 40e3628..4907ce5 100644 > > --- a/plugins/telit/tests/test-mm-modem-helpers-telit.c > > +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c > > @@ -28,36 +28,46 @@ > > typedef struct { > > gchar *response; > > gint result; > > + gchar *error_message; > > } CSIMResponseTest; > > > > static CSIMResponseTest valid_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,\"xxxx63C1\"", 1, NULL}, > > + {"+CSIM:8,\"xxxx63CA\"", 10, NULL}, > > + {"+CSIM:8,\"xxxx63CF\"", 15, NULL}, > > /* The parser accepts spaces */ > > - {"+CSIM:8,\"xxxx63C1\"", 1}, > > - {"+CSIM:8, \"xxxx63C1\"", 1}, > > - {"+CSIM: 8, \"xxxx63C1\"", 1}, > > - {"+CSIM: 8, \"xxxx63C1\"", 1}, > > + {"+CSIM:8,\"xxxx63C1\"", 1, NULL}, > > + {"+CSIM:8, \"xxxx63C1\"", 1, NULL}, > > + {"+CSIM: 8, \"xxxx63C1\"", 1, NULL}, > > + {"+CSIM: 8, \"xxxx63C1\"", 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}, > > + {"+CSIM: 4, \"63CF\"", 15, NULL}, > > /* the parser expect a quotation mark at the end > > * of the response, but not at the begin */ > > - {"+CSIM: 10, 63CF\"", 15}, > > + {"+CSIM: 4, 63CF\"", 15, NULL}, > > + {"+CSIM: 4, 63CF\"", 15, NULL}, > > + /* SIM 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, \"69XX\"", -1, "Unknown error code '69XX'"}, > > + {"+CSIM: 4, \"6A86\"", -1, "Incorrect parameters in SIM > > request"}, > > + {"+CSIM: 4, \"6A88\"", -1, "SIM reference data not found"}, > > + {"+CSIM: 4, \"6AXX\"", -1, "Unknown error code '6AXX'"}, > > { NULL, -1} > > }; > > > > static CSIMResponseTest invalid_csim_response_test_list [] = { > > /* Test missing final quotation mark */ > > - {"+CSIM: 8, xxxx63CF", -1}, > > + {"+CSIM: 8, xxxx63CF", -1, NULL}, > > /* Negative test for substring "63Cx" */ > > - {"+CSIM: 4, xxxx73CF\"", -1}, > > + {"+CSIM: 4, xxxx73CF\"", -1, NULL}, > > /* Test missing first argument */ > > - {"+CSIM:xxxx63CF\"", -1}, > > + {"+CSIM:xxxx63CF\"", -1, NULL}, > > { NULL, -1} > > }; > > > > @@ -73,7 +83,14 @@ test_mm_telit_parse_csim_response (void) > > 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); > > + if (valid_csim_response_test_list[i].error_message == NULL) > > { > > + g_assert_no_error (error); > > + } else { > > + g_assert_cmpstr (error->message, ==, > > valid_csim_response_test_list[i].error_message); > > + g_error_free (error); > > + error = NULL; > > + } > > + > > g_assert_cmpint (res, ==, > > valid_csim_response_test_list[i].result); > > } > > >
_______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
