cinterion: make sure FALSE sets GError in parse_smong_response()

The g_regex_match_full() method may return FALSE without setting the
GError, so that case needs to be considered.

In addition to that, the following g_assert() was not doing what it
should have been, as it was comparing the address of the variable and
not the variable itself; rework the code to avoid that as well:
  g_assert (access_tech != MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN);

2 files changed, 48 insertions, 29 deletions

diff --git a/plugins/cinterion/mm-modem-helpers-cinterion.c b/plugins/cinterion/mm-modem-helpers-cinterion.c
index d397e423..67da6120 100644
--- a/plugins/cinterion/mm-modem-helpers-cinterion.c
+++ b/plugins/cinterion/mm-modem-helpers-cinterion.c
@@ -940,19 +940,23 @@ mm_cinterion_parse_sgauth_response (const gchar          *response,
 /*****************************************************************************/
 /* ^SMONG response parser */
 
-static MMModemAccessTechnology
-get_access_technology_from_smong_gprs_status (guint    gprs_status,
-                                              GError **error)
+static gboolean
+get_access_technology_from_smong_gprs_status (guint                     gprs_status,
+                                              MMModemAccessTechnology  *out,
+                                              GError                  **error)
 {
     switch (gprs_status) {
     case 0:
-        return MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN;
+        *out = MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN;
+        return TRUE;
     case 1:
     case 2:
-        return MM_MODEM_ACCESS_TECHNOLOGY_GPRS;
+        *out = MM_MODEM_ACCESS_TECHNOLOGY_GPRS;
+        return TRUE;
     case 3:
     case 4:
-        return MM_MODEM_ACCESS_TECHNOLOGY_EDGE;
+        *out = MM_MODEM_ACCESS_TECHNOLOGY_EDGE;
+        return TRUE;
     default:
         break;
     }
@@ -963,7 +967,7 @@ get_access_technology_from_smong_gprs_status (guint    gprs_status,
                  "Couldn't get network capabilities, "
                  "unsupported GPRS status value: '%u'",
                  gprs_status);
-    return MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN;
+    return FALSE;
 }
 
 gboolean
@@ -971,9 +975,10 @@ mm_cinterion_parse_smong_response (const gchar              *response,
                                    MMModemAccessTechnology  *access_tech,
                                    GError                  **error)
 {
-    GError     *inner_error = NULL;
-    GMatchInfo *match_info = NULL;
-    GRegex     *regex;
+    guint                  value = 0;
+    GError                *inner_error = NULL;
+    g_autoptr(GMatchInfo)  match_info = NULL;
+    g_autoptr(GRegex)      regex;
 
     /* The AT^SMONG command returns a cell info table, where the second
      * column identifies the "GPRS status", which is exactly what we want.
@@ -992,26 +997,21 @@ mm_cinterion_parse_smong_response (const gchar              *response,
                          0, NULL);
     g_assert (regex);
 
-    if (g_regex_match_full (regex, response, strlen (response), 0, 0, &match_info, &inner_error)) {
-        guint value = 0;
-
-        if (!mm_get_uint_from_match_info (match_info, 2, &value))
-            inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
-                                       "Couldn't read 'GPRS status' field from AT^SMONG response");
-        else if (access_tech)
-            *access_tech = get_access_technology_from_smong_gprs_status (value, &inner_error);
-    }
-
-    g_match_info_free (match_info);
-    g_regex_unref (regex);
+    g_regex_match_full (regex, response, strlen (response), 0, 0, &match_info, &inner_error);
 
     if (inner_error) {
+        g_prefix_error (&inner_error, "Failed to match AT^SMONG response: ");
         g_propagate_error (error, inner_error);
         return FALSE;
     }
 
-    g_assert (access_tech != MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN);
-    return TRUE;
+    if (!g_match_info_matches (match_info) || !mm_get_uint_from_match_info (match_info, 2, &value)) {
+        g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
+                     "Couldn't read 'GPRS status' field from AT^SMONG response");
+        return FALSE;
+    }
+
+    return get_access_technology_from_smong_gprs_status (value, access_tech, error);
 }
 
 /*****************************************************************************/
diff --git a/plugins/cinterion/tests/test-modem-helpers-cinterion.c b/plugins/cinterion/tests/test-modem-helpers-cinterion.c
index 8ea7d909..efd19b38 100644
--- a/plugins/cinterion/tests/test-modem-helpers-cinterion.c
+++ b/plugins/cinterion/tests/test-modem-helpers-cinterion.c
@@ -1134,6 +1134,7 @@ test_sind_response_simstatus (void)
 
 static void
 common_test_smong_response (const gchar             *response,
+                            gboolean                 success,
                             MMModemAccessTechnology  expected_access_tech)
 {
     GError                  *error = NULL;
@@ -1141,10 +1142,15 @@ common_test_smong_response (const gchar             *response,
     MMModemAccessTechnology  access_tech;
 
     res = mm_cinterion_parse_smong_response (response, &access_tech, &error);
-    g_assert_no_error (error);
-    g_assert (res == TRUE);
 
-    g_assert_cmpuint (access_tech, ==, expected_access_tech);
+    if (success) {
+        g_assert_no_error (error);
+        g_assert (res);
+        g_assert_cmpuint (access_tech, ==, expected_access_tech);
+    } else {
+        g_assert (error);
+        g_assert (!res);
+    }
 }
 
 static void
@@ -1155,7 +1161,7 @@ test_smong_response_tc63i (void)
         "GPRS Monitor\r\n"
         "BCCH  G  PBCCH  PAT MCC  MNC  NOM  TA      RAC                               # Cell #\r\n"
         "0073  1  -      -   262   02  2    00 01\r\n";
-    common_test_smong_response (response, MM_MODEM_ACCESS_TECHNOLOGY_GPRS);
+    common_test_smong_response (response, TRUE, MM_MODEM_ACCESS_TECHNOLOGY_GPRS);
 }
 
 static void
@@ -1167,7 +1173,19 @@ test_smong_response_other (void)
         "\r\n"
         "BCCH  G  PBCCH  PAT MCC  MNC  NOM  TA      RAC                              # Cell #\r\n"
         "  44  1  -      -   234   10  -    -       -                                             \r\n";
-    common_test_smong_response (response, MM_MODEM_ACCESS_TECHNOLOGY_GPRS);
+    common_test_smong_response (response, TRUE, MM_MODEM_ACCESS_TECHNOLOGY_GPRS);
+}
+
+static void
+test_smong_response_no_match (void)
+{
+    const gchar *response =
+        "\r\n"
+        "GPRS Monitor\r\n"
+        "\r\n"
+        "BCCH  K  PBCCH  PAT MCC  MNC  NOM  TA      RAC                              # Cell #\r\n"
+        "  44  1  -      -   234   10  -    -       -                                             \r\n";
+    common_test_smong_response (response, FALSE, MM_MODEM_ACCESS_TECHNOLOGY_UNKNOWN);
 }
 
 /*****************************************************************************/
@@ -1757,6 +1775,7 @@ int main (int argc, char **argv)
     g_test_add_func ("/MM/cinterion/sind/response/simstatus", test_sind_response_simstatus);
     g_test_add_func ("/MM/cinterion/smong/response/tc63i",    test_smong_response_tc63i);
     g_test_add_func ("/MM/cinterion/smong/response/other",    test_smong_response_other);
+    g_test_add_func ("/MM/cinterion/smong/response/no-match", test_smong_response_no_match);
     g_test_add_func ("/MM/cinterion/slcc/urc/empty",          test_slcc_urc_empty);
     g_test_add_func ("/MM/cinterion/slcc/urc/single",         test_slcc_urc_single);
     g_test_add_func ("/MM/cinterion/slcc/urc/multiple",       test_slcc_urc_multiple);