Hi Aleksander, see below my reply
Il giovedì 6 aprile 2017, Aleksander Morgado <aleksan...@aleksander.es> ha scritto: > 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? > > I agree, the "step" thing is just for developer (myself, specifically :D), and I prefer the idea to use a separate patch. > > > 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. > right > > > 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})\"" > 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 :) > > > > > 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(). > I see. Ok, I can simplify it a bit > > > 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. > Damn :/ > > > + 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; > } > Indeed, I'll fix that > > > + > > + 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". > Ah, yes, that could happen. > > > + } 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. > Ok > > > 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<B > ands4G>[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_lis > t[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); > > } > > } > > > > > Ok, I'll be back with all the changes > > > -- > Aleksander > https://aleksander.es >
_______________________________________________ ModemManager-devel mailing list ModemManager-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
