[PATCH 2/2] move +CRSM parsing to mm_3gpp_parse_crsm_response; add test cases
Aleksander Morgado
aleksander at aleksander.es
Sat Feb 13 12:14:13 UTC 2016
Hey Thomas,
A consolidated +CRSM reader, that was nice to see :)
See comments below.
On Fri, Feb 12, 2016 at 12:43 PM, Thomas Sailer
<sailer at sailer.dynip.lugs.ch> wrote:
> From: Thomas Sailer <t.sailer at alumni.ethz.ch>
>
> Signed-off-by: Thomas Sailer <t.sailer at alumni.ethz.ch>
> ---
> src/mm-base-sim.c | 120 +++++++++++++++++++++--------------------
> src/mm-modem-helpers.c | 47 ++++++++++++++++
> src/mm-modem-helpers.h | 8 +++
> src/tests/test-modem-helpers.c | 54 +++++++++++++++++++
> 4 files changed, 172 insertions(+), 57 deletions(-)
>
> diff --git a/src/mm-base-sim.c b/src/mm-base-sim.c
> index 104e7f8..ff3eb09 100644
> --- a/src/mm-base-sim.c
> +++ b/src/mm-base-sim.c
> @@ -944,23 +944,26 @@ static gchar *
> parse_iccid (const gchar *response,
> GError **error)
> {
> - gchar buf[21];
> - const gchar *str;
> - gint sw1;
> - gint sw2;
> - gboolean success = FALSE;
> -
> - memset (buf, 0, sizeof (buf));
> - str = mm_strip_tag (response, "+CRSM:");
> - if (sscanf (str, "%d,%d,\"%20c\"", &sw1, &sw2, (char *) &buf) == 3)
> - success = TRUE;
> - else {
> - /* May not include quotes... */
> - if (sscanf (str, "%d,%d,%20c", &sw1, &sw2, (char *) &buf) == 3)
> - success = TRUE;
> + GError *inner_error = NULL;
> + guint sw1 = 0;
> + guint sw2 = 0;
> + gchar *hex = 0;
> + gboolean success;
> +
> + success = mm_3gpp_parse_crsm_response (response,
> + &sw1,
> + &sw2,
> + &hex,
> + &inner_error);
> +
> + if (inner_error) {
> + g_propagate_error (error, inner_error);
> + g_prefix_error (error, "Could not parse the CRSM response");
> + return NULL;
> }
>
> if (!success) {
There shouldn't be 2 different checks for inner_error and !success;
those handle the same condition: the method failed. See comments
below.
> + g_free (hex);
And if method fails, no other allocated output should be returned, so
this free shouldn't be here.
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> @@ -972,8 +975,11 @@ parse_iccid (const gchar *response,
> (sw1 == 0x91) ||
> (sw1 == 0x92) ||
> (sw1 == 0x9f)) {
> - return mm_3gpp_parse_iccid (buf, error);
> + gchar *ret = mm_3gpp_parse_iccid (hex, error);
Variables are declared at the beginning of the context, then
whiteline, then method calls, e.g.:
{
gchar *ret;
ret = mm_3gpp_parse_iccid (hex, error);
g_free (hex);
return ret;
}
> + g_free (hex);
> + return ret;
> } else {
> + g_free (hex);
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> @@ -1101,21 +1107,26 @@ static guint
> parse_mnc_length (const gchar *response,
> GError **error)
> {
> - gint sw1;
> - gint sw2;
> - gboolean success = FALSE;
> - gchar hex[51];
> -
> - memset (hex, 0, sizeof (hex));
> - if (sscanf (response, "+CRSM:%d,%d,\"%50c\"", &sw1, &sw2, (char *) &hex) == 3)
> - success = TRUE;
> - else {
> - /* May not include quotes... */
> - if (sscanf (response, "+CRSM:%d,%d,%50c", &sw1, &sw2, (char *) &hex) == 3)
> - success = TRUE;
> + GError *inner_error = NULL;
> + guint sw1 = 0;
> + guint sw2 = 0;
> + gchar *hex = 0;
> + gboolean success;
> +
> + success = mm_3gpp_parse_crsm_response (response,
> + &sw1,
> + &sw2,
> + &hex,
> + &inner_error);
> +
> + if (inner_error) {
> + g_propagate_error (error, inner_error);
> + g_prefix_error (error, "Could not parse the CRSM response");
> + return 0;
> }
>
> if (!success) {
> + g_free (hex);
Same here, see below.
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> @@ -1131,15 +1142,6 @@ parse_mnc_length (const gchar *response,
> guint32 mnc_len;
> gchar *bin;
>
> - /* Make sure the buffer is only hex characters */
> - while (buflen < sizeof (hex) && hex[buflen]) {
> - if (!isxdigit (hex[buflen])) {
> - hex[buflen] = 0x0;
> - break;
> - }
> - buflen++;
> - }
Looks like we're not doing this check in the new code? I think we
should do it in mm_3gpp_parse_crsm_response() before returning
success. If we check the hex string read and it doesn't have hex chars
for some reason, we free hex and return FALSE with error set.
> -
> /* Convert hex string to binary */
> bin = mm_utils_hexstr2bin (hex, &buflen);
> if (!bin || buflen < 4) {
> @@ -1149,9 +1151,12 @@ parse_mnc_length (const gchar *response,
> "SIM returned malformed response '%s'",
> hex);
> g_free (bin);
> + g_free (hex);
> return 0;
> }
>
> + g_free (hex);
> +
> /* MNC length is byte 4 of this SIM file */
> mnc_len = bin[3] & 0xFF;
> if (mnc_len == 2 || mnc_len == 3) {
> @@ -1168,6 +1173,7 @@ parse_mnc_length (const gchar *response,
> return 0;
> }
>
> + g_free (hex);
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> @@ -1238,21 +1244,26 @@ static gchar *
> parse_spn (const gchar *response,
> GError **error)
> {
> - gint sw1;
> - gint sw2;
> - gboolean success = FALSE;
> - gchar hex[51];
> -
> - memset (hex, 0, sizeof (hex));
> - if (sscanf (response, "+CRSM:%d,%d,\"%50c\"", &sw1, &sw2, (char *) &hex) == 3)
> - success = TRUE;
> - else {
> - /* May not include quotes... */
> - if (sscanf (response, "+CRSM:%d,%d,%50c", &sw1, &sw2, (char *) &hex) == 3)
> - success = TRUE;
> + GError *inner_error = NULL;
> + guint sw1 = 0;
> + guint sw2 = 0;
> + gchar *hex = 0;
> + gboolean success;
> +
> + success = mm_3gpp_parse_crsm_response (response,
> + &sw1,
> + &sw2,
> + &hex,
> + &inner_error);
> +
> + if (inner_error) {
> + g_propagate_error (error, inner_error);
> + g_prefix_error (error, "Could not parse the CRSM response");
> + return NULL;
> }
>
> if (!success) {
> + g_free (hex);
Same here, see below.
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> @@ -1268,15 +1279,6 @@ parse_spn (const gchar *response,
> gchar *bin;
> gchar *utf8;
>
> - /* Make sure the buffer is only hex characters */
> - while (buflen < sizeof (hex) && hex[buflen]) {
> - if (!isxdigit (hex[buflen])) {
> - hex[buflen] = 0x0;
> - break;
> - }
> - buflen++;
> - }
> -
> /* Convert hex string to binary */
> bin = mm_utils_hexstr2bin (hex, &buflen);
> if (!bin) {
> @@ -1285,9 +1287,12 @@ parse_spn (const gchar *response,
> MM_CORE_ERROR_FAILED,
> "SIM returned malformed response '%s'",
> hex);
> + g_free (hex);
> return NULL;
> }
>
> + g_free (hex);
> +
> /* Remove the FF filler at the end */
> while (buflen > 1 && bin[buflen - 1] == (char)0xff)
> buflen--;
> @@ -1298,6 +1303,7 @@ parse_spn (const gchar *response,
> return utf8;
> }
>
> + g_free (hex);
> g_set_error (error,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c
> index bb7837e..a06d311 100644
> --- a/src/mm-modem-helpers.c
> +++ b/src/mm-modem-helpers.c
> @@ -1333,6 +1333,53 @@ done:
> return info;
> }
>
> +/*****************************************************************************/
> +
> +/* AT+CRSM response parser */
> +gboolean
> +mm_3gpp_parse_crsm_response (const gchar *reply,
> + guint *sw1,
> + guint *sw2,
> + gchar **hex,
> + GError **error)
> +{
> + GRegex *r;
> + GMatchInfo *match_info;
> + GError *inner_error = NULL;
> + gboolean success = FALSE;
> +
> + g_assert (sw1 != NULL);
> + g_assert (sw2 != NULL);
> + g_assert (hex != NULL);
> +
> + if (!reply || !g_str_has_prefix (reply, "+CRSM:")) {
> + g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED, "Missing +CRSM prefix");
> + return FALSE;
> + }
> +
> + r = g_regex_new ("\\+CRSM:\\s*(\\d+)\\s*,\\s*(\\d+)\\s*,\\s*\"?([0-9a-fA-F]+)\"?",
> + G_REGEX_RAW, 0, &inner_error);
> + g_assert (r != NULL);
> +
> + g_regex_match_full (r, reply, strlen (reply), 0, 0, &match_info, &inner_error);
> + if (!inner_error && g_match_info_matches (match_info)) {
> + success = mm_get_uint_from_match_info (match_info, 1, sw1) &&
> + mm_get_uint_from_match_info (match_info, 2, sw2);
> + if (success)
> + *hex = mm_get_string_unquoted_from_match_info (match_info, 3);
> + }
> +
> + g_match_info_free (match_info);
> + g_regex_unref (r);
> +
> + if (inner_error) {
> + g_propagate_error (error, inner_error);
> + g_prefix_error (error, "Couldn't properly parse +CRSM. ");
> + }
> +
> + return success;
> +}
> +
One of the usual compromises when writing code that uses GError to
return errors is that if an error is returned, no other allocated
output will be returned (and therefore caller doesn't need to consider
that). In this specific case:
* If method returns FALSE, the output error must be set, and hex
won't be allocated. we don't care about sw1 and sw2 because they're
not extra allocations, they can be ignored.
* If method returns TRUE, the output error must not be set, sw1 and
sw2 will be set and hex will be allocated and set.
In your implementation, I think you could do something like this (plus
the hex validity check?):
{
...
*hex = NULL;
*sw1 = NULL;
*sw2 = NULL;
if (!g_regex_match_full (r, reply, strlen (reply), 0, 0, &match_info, NULL))
goto done;
if (!mm_get_uint_from_match_info (match_info, 1, sw1))
goto done;
if (!mm_get_uint_from_match_info (match_info, 2, sw2))
goto done;
*hex = mm_get_string_unquoted_from_match_info (match_info, 3);
done:
g_match_info_free (match_info);
g_regex_unref (r);
if (*hex == NULL) {
g_set_error (error,
MM_CORE_ERROR,
MM_CORE_ERROR_FAILED,
"Failed to parse CMGF query result '%s'",
reply);
return FALSE;
}
return TRUE;
}
> /*************************************************************************/
>
> static MMSmsStorage
> diff --git a/src/mm-modem-helpers.h b/src/mm-modem-helpers.h
> index 3be7c7b..975a493 100644
> --- a/src/mm-modem-helpers.h
> +++ b/src/mm-modem-helpers.h
> @@ -203,6 +203,14 @@ MM3gppPduInfo *mm_3gpp_parse_cmgr_read_response (const gchar *reply,
> guint index,
> GError **error);
>
> +
> +/* AT+CRSM response parser */
> +gboolean mm_3gpp_parse_crsm_response (const gchar *reply,
> + guint *sw1,
> + guint *sw2,
> + gchar **hex,
> + GError **error);
> +
> /* Additional 3GPP-specific helpers */
>
> MMModem3gppFacility mm_3gpp_acronym_to_facility (const gchar *str);
> diff --git a/src/tests/test-modem-helpers.c b/src/tests/test-modem-helpers.c
> index 4dc5662..db0cef9 100644
> --- a/src/tests/test-modem-helpers.c
> +++ b/src/tests/test-modem-helpers.c
> @@ -2621,6 +2621,58 @@ test_cclk_response (void)
> }
> }
>
> +
> +/*****************************************************************************/
> +/* Test +CRSM responses */
> +
> +typedef struct {
> + const gchar *str;
> + gboolean ret;
> + guint sw1;
> + guint sw2;
> + gchar *hex;
> +} CrsmTest;
> +
> +static const CrsmTest crsm_tests[] = {
> + { "+CRSM: 144, 0, 0054485552415941FFFFFFFFFFFFFFFFFF", TRUE, 144, 0, "0054485552415941FFFFFFFFFFFFFFFFFF" },
> + { "+CRSM: 144, 0,0054485552415941FFFFFFFFFFFFFFFFFF", TRUE, 144, 0, "0054485552415941FFFFFFFFFFFFFFFFFF" },
> + { "+CRSM: 144, 0, \"0054485552415941FFFFFFFFFFFFFFFFFF\"", TRUE, 144, 0, "0054485552415941FFFFFFFFFFFFFFFFFF" },
> + { "+CRSM: 144, 0,\"0054485552415941FFFFFFFFFFFFFFFFFF\"", TRUE, 144, 0, "0054485552415941FFFFFFFFFFFFFFFFFF" },
> + { NULL, FALSE, 0, 0, NULL }
> +};
> +
> +static void
> +test_crsm_response (void)
> +{
> + guint i;
> +
> + for (i = 0; crsm_tests[i].str; i++) {
> + GError *error = NULL;
> + guint sw1 = 0;
> + guint sw2 = 0;
> + gchar *hex = 0;
> + gboolean ret;
> +
> + ret = mm_3gpp_parse_crsm_response (crsm_tests[i].str,
> + &sw1,
> + &sw2,
> + &hex,
> + &error);
> +
> + g_assert (ret == crsm_tests[i].ret);
> + g_assert (ret == (error ? FALSE : TRUE));
> +
> + g_clear_error (&error);
> +
> + g_assert (sw1 == crsm_tests[i].sw1);
> + g_assert (sw2 == crsm_tests[i].sw2);
> +
> + g_assert_cmpstr (crsm_tests[i].hex, ==, hex);
> +
> + g_free(hex);
> + }
> +}
> +
> /*****************************************************************************/
>
> void
> @@ -2794,6 +2846,8 @@ int main (int argc, char **argv)
>
> g_test_suite_add (suite, TESTCASE (test_cclk_response, NULL));
>
> + g_test_suite_add (suite, TESTCASE (test_crsm_response, NULL));
> +
> result = g_test_run ();
>
> reg_test_data_free (reg_data);
> --
> 2.5.0
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list