[PATCH v3] telit: add error_code recognition to +CSIM parser
Aleksander Morgado
aleksander at aleksander.es
Thu Apr 6 19:18:46 UTC 2017
Hey Carlo,
On 04/04/17 14:55, 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: 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.
>
See comments inline below.
> ---
> plugins/telit/mm-broadband-modem-telit.c | 6 +-
> plugins/telit/mm-modem-helpers-telit.c | 93 ++++++++++++++++-------
> plugins/telit/mm-modem-helpers-telit.h | 3 +-
> plugins/telit/tests/test-mm-modem-helpers-telit.c | 72 +++++++++---------
> 4 files changed, 105 insertions(+), 69 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index cce0229..f316e30 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);
I don't think we should be printing the "step number", especially not
via warning. This information would be useful if instead we gave the
actual lock name associated to the step, though.
As the csim_query_ready() is being reused for multiple locks, we could
have an array of strings specifying which lock is being processed at
each step, e.g.:
static const gchar *step_lock_names[LOAD_UNLOCK_RETRIES_STEP_LAST] = {
[LOAD_UNLOCK_RETRIES_STEP_PIN] = "PIN",
[LOAD_UNLOCK_RETRIES_STEP_PUK] = "PUK",
[LOAD_UNLOCK_RETRIES_STEP_PIN2] = "PIN2",
[LOAD_UNLOCK_RETRIES_STEP_PUK2] = "PUK2",
};
What do you think? I know this issue was already there, but just spotted
it :) Maybe handled in a separate new patch better?
> 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);
Same here with the step number.
> 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..cb3ff24 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,92 @@ 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;
> -
> - r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*63C(.*)\"", G_REGEX_RAW, 0, NULL);
> + GRegex *r = NULL;
> + gchar *str_code = NULL;
> + gint retries = -1;
> + guint64 hex_code = 0x0;
>
> + r = g_regex_new ("\\+CSIM:\\s*[0-9]+,\\s*.*([0-9a-fA-F]{4})\"", G_REGEX_RAW, 0, NULL);
The regex is matching the trailing double-quotes; why not the leading
ones as well? E.g.:
"\\+CSIM:\\s*[0-9]+,\\s*\".*([0-9a-fA-F]{4})\""
> 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;
> + "Could not recognize response '%s'", response);
> + goto out;
> }
>
> - if (!g_match_info_matches (match_info)) {
> + if (match_info == NULL || !g_match_info_matches (match_info)) {
match_info is always created by g_regex_match(), so no need to check if
it is == NULL here. Actually, you could also ignore the g_regex_match()
return value all together if you're afterwards running
g_match_info_matches().
> 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;
> + "Could not parse response '%s'", response);
> + goto out;
> + }
> +
> + str_code = mm_get_string_unquoted_from_match_info (match_info, 1);
> + if (str_code == NULL) {
> + g_set_error (error, 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);
This is a good change; mm_get_string_unquoted_from_match_info() may be
returning NULL if the matched string is empty, so the g_assert() was a
bit too much.
> + errno = 0;
> + hex_code = g_ascii_strtoull (str_code, NULL, 16);
> + if (hex_code == G_MAXUINT64 && errno == ERANGE) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not recognize expected hex code in response '%s'", response);
> + goto out;
> + }
And this one may be too much, as we really just matched a 4-digit hex
string with the regex, so the chances of a failure here are 0. But
anyway, it's safer in this way in case the regex changes in the future,
so good.
>
> - /* 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) {
A whitespace before the "(" is missing.
> + case 0x6300:
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "SIM verification failed");
> + break;
Instead of "break;" just "goto out;" is perfectly fine here and in all
the other cases.
> + case 0x6983:
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "SIM authentication method blocked");
> + break;
> + case 0x6984:
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "SIM reference data invalidated");
> + break;
> + case 0x6A86:
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Incorrect parameters in SIM request");
> + break;
> + case 0x6A88:
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "SIM reference data not found");
> + break;
> + default:
> + break;
> + }
> +
> + if (g_error_matches (*error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED)) {
> + goto out;
> + }
This isn't good practice, you shouldn't assume that the caller passes a
valid GError address, the method should also accept NULL as GError
address so that the caller ignores it.
A better approach would be to have a "GError *inner_error;" declared by
yourself inside the method, use g_error_new() to create the GError, and
then, before returning -1 in the method, do something like:
...
if (inner_error) {
g_propagate_error (error, inner_error);
return -1;
}
g_assert (retries >= 0);
return retries;
}
> +
> + if (hex_code < MM_TELIT_MIN_SIM_RETRY_HEX || hex_code > MM_TELIT_MAX_SIM_RETRY_HEX) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "SIM retries out of bound in response '%s'", response);
If we get a value out of bounds, wouldn't it really be another error we
don't know about (and therefore not handled in the switch above)? Maybe
we could just:
g_set_error (..., "Unknown error returned: 0x%04x", hex_code);
i.e. instead of saying "out of bounds".
> + } else {
> + retries = (gint)(hex_code - MM_TELIT_MIN_SIM_RETRY_HEX);
> }
>
> - g_free (retries_hex_str);
> +out:
> + if (str_code != NULL)
> + g_free (str_code);
No need to check the pointer being != NULL before calling g_free();
g_free() (as also does free()) accepts the NULL pointer and will just do
nothing.
> g_match_info_free (match_info);
> g_regex_unref (r);
>
> 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..8b7a808 100644
> --- a/plugins/telit/tests/test-mm-modem-helpers-telit.c
> +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c
> @@ -28,66 +28,64 @@
> 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,\"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: 10, \"63CF\"", 15, NULL},
> /* 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},
> + * of the response, but not at the beginning */
> + {"+CSIM: 4, 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: out of bound */
> + {"+CSIM: 4, \"63BF\"", -1, "SIM retries out of bound in response '+CSIM: 4, \"63BF\"'"},
> + {"+CSIM: 4, \"63D0\"", -1, "SIM retries out of bound in response '+CSIM: 4, \"63D0\"'"},
> + /* Test error: missing final quotation mark */
> + {"+CSIM: 8, xxxx63CF", -1, "Could not recognize response '+CSIM: 8, xxxx63CF'"},
> + /* Test error: missing first argument */
> + {"+CSIM:xxxx63CF\"", -1, "Could not recognize response '+CSIM:xxxx63CF\"'"},
> { NULL, -1}
I know this wasn't part of the patch, but I would remove this last
{NULL, -1} element, and then use G_N_ELEMENTS() when iterating below.
Given that you're merging two arrays together, this may be a good moment
to do that change as well.
> };
>
> 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);
> -
> - 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) {
> + for (i = 0; csim_response_test_list[i].response != NULL; i++) {
E.g.:
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);
> +
> + 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_error_free (error);
> error = NULL;
Also not part of the changes you're doing but let's use this to free the
error and reset the pointer to NULL at the same time:
g_clear_error (&error);
> }
> +
> + g_assert_cmpint (res, ==, csim_response_test_list[i].result);
> }
> }
>
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list