<div dir="ltr">Hi Aleksander,<div> </div><div>see below my reply Il giovedì 6 aprile 2017, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> ha scritto: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=100374" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=100374</a> > > --- > > 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? </blockquote><div> </div><div>I agree, the "step" thing is just for developer (myself, specifically :D), and I prefer the idea to use a separate patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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. </blockquote><div> </div><div>right</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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})\"" </blockquote><div> </div><div>I'm not sure it's really necessary. I'm still accepting everything (.*) until a string with an hex appears ([0-9...]) and I need the trailing double-quotes just to stop the parser, but I think it does not hurt to specify the beginning of the string :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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(). </blockquote><div> </div><div>I see. Ok, I can simplify it a bit</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > - /* 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. </blockquote><div> </div><div>Damn :/</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + 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; } </blockquote><div> </div><div>Indeed, I'll fix that</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + 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". </blockquote><div> </div><div>Ah, yes, that could happen.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + } 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 </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">nothing. </blockquote><div> </div><div>Ok</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > 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); > } > } > > </blockquote><div> <div style="font-family:arial,helvetica,sans-serif;display:inline" class="gmail_default">Ok, I'll be back with all the changes </div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -- Aleksander <a href="https://aleksander.es" target="_blank">https://aleksander.es</a> </blockquote></div> </div>