cinterion: simplify ^SWWAN response parsing

We get as input the ^SWWAN index we're interested in, and we loop
through the list of ^SWWAN lines looking for the one we need.

This updated helper method allows working with multi-line ^SWWAN
responses, e.g. given when more than one PDP context is active.

plugins/cinterion/mm-broadband-bearer-cinterion.c

@@ -86,49 +86,6 @@ get_usb_interface_config_index (MMPort  *data,
     return -1;
 }
 
-/*****************************************************************************/
-/* Common - Helper Functions*/
-
-static gint
-verify_connection_state_from_swwan_response (GList *result, GError **error)
-{
-    /* Returns 0 if SWWAN is connected, 1 if not connected, -1 on error
-     * for the bearer's target interface */
-
-    if (g_list_length(result) != 0) {
-        int first_result = GPOINTER_TO_INT(result->data);
-
-        /* Received an 'OK'(0) response */
-        if (first_result == 0)
-            return 1;
-        /* 1 || 3 result is the CID, given when that context is activated.
-         * TODO: Rework for dual sim connections. */
-        else if (first_result == 1 || first_result ==3)
-            return 0;
-        else {
-            for (; result; result = g_list_next(result))
-                mm_err ("Unknown SWWAN response data:%i", GPOINTER_TO_INT(result->data));
-
-            g_set_error (error,
-                         MM_CORE_ERROR,
-                         MM_CORE_ERROR_INVALID_ARGS,
-                         "Unparsable SWWAN response format.");
-
-            return -1;
-        }
-     }
-     else {
-        mm_err ("Unable to parse zero length SWWAN response.");
-
-        g_set_error (error,
-                     MM_CORE_ERROR,
-                     MM_CORE_ERROR_INVALID_ARGS,
-                     "Unparsable SWWAN response format.");
-
-        return -1;
-     }
-}
-
 /*****************************************************************************/
 /* Auth helpers */
 
@@ -255,8 +212,8 @@ swwan_connect_check_status_ready (MMBaseModem        *modem,
                                   Connect3gppContext *ctx)
 {
     const gchar  *response;
-    GList       *response_parsed = NULL;
     GError       *error = NULL;
+    MMSwwanState  state;
 
     response = mm_base_modem_at_command_full_finish (modem, res, &error);
     if (!response) {
@@ -265,22 +222,24 @@ swwan_connect_check_status_ready (MMBaseModem        *modem,
         return;
     }
 
-    /* Error if parsing SWWAN read response fails */
+    state = mm_cinterion_parse_swwan_response (response,
-    if (!mm_cinterion_parse_swwan_response (response, &response_parsed, &error)) {
+                                               usb_interface_configs[ctx->usb_interface_config_index].pdp_context,
+                                               &error);
+    if (state == MM_SWWAN_STATE_UNKNOWN) {
         g_simple_async_result_take_error (ctx->result, error);
         connect_3gpp_context_complete_and_free (ctx);
         return;
     }
 
-    /* Check parsed SWWAN reponse to see if we are now connected */
+    if (state == MM_SWWAN_STATE_DISCONNECTED) {
-    if (verify_connection_state_from_swwan_response (response_parsed, &error)) {
+        g_simple_async_result_set_error (ctx->result, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
-        g_simple_async_result_take_error (ctx->result, error);
+                                         "CID %u is reported disconnected",
+                                         usb_interface_configs[ctx->usb_interface_config_index].pdp_context);
         connect_3gpp_context_complete_and_free (ctx);
-        g_list_free (response_parsed);
         return;
     }
 
-    g_list_free (response_parsed);
+    g_assert (state == MM_SWWAN_STATE_CONNECTED);
 
     /* Go to next step */
     ctx->step++;
@@ -576,8 +535,8 @@ swwan_disconnect_check_status_ready (MMBaseModem           *modem,
                                      Disconnect3gppContext *ctx)
 {
     const gchar  *response;
-    GList       *response_parsed = NULL;
     GError       *error = NULL;
+    MMSwwanState  state;
 
     response = mm_base_modem_at_command_full_finish (modem, res, &error);
     if (error) {
@@ -586,23 +545,26 @@ swwan_disconnect_check_status_ready (MMBaseModem           *modem,
         return;
     }
 
-    /* Return if the SWWAN read threw an error or parsing it fails */
+    state = mm_cinterion_parse_swwan_response (response,
-    if (!mm_cinterion_parse_swwan_response (response, &response_parsed, &error)) {
+                                               usb_interface_configs[ctx->usb_interface_config_index].pdp_context,
-        g_simple_async_result_take_error (ctx->result, error);
+                                               &error);
+
+    if (state == MM_SWWAN_STATE_CONNECTED) {
+        g_simple_async_result_set_error (ctx->result, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
+                                         "CID %u is reported connected",
+                                         usb_interface_configs[ctx->usb_interface_config_index].pdp_context);
         disconnect_3gpp_context_complete_and_free (ctx);
         return;
     }
 
-    /* Check parsed SWWAN reponse to see if we are now disconnected */
+    if (state == MM_SWWAN_STATE_UNKNOWN) {
-    if (verify_connection_state_from_swwan_response (response_parsed, &error) != 1) {
+        /* Assume disconnected */
-        g_prefix_error (&error, "Disconnection attempt failed: ");
+        mm_dbg ("couldn't get CID %u status, assume disconnected: %s",
-        g_simple_async_result_take_error (ctx->result, error);
+                usb_interface_configs[ctx->usb_interface_config_index].pdp_context,
-        disconnect_3gpp_context_complete_and_free (ctx);
+                error->message);
-        g_list_free (response_parsed);
+        g_error_free (error);
-        return;
+    } else
-    }
+        g_assert (state == MM_SWWAN_STATE_DISCONNECTED);
-
-    g_list_free (response_parsed);
 
     /* Go on to next step */
     ctx->step++;

plugins/cinterion/mm-modem-helpers-cinterion.c

@@ -489,8 +489,15 @@ mm_cinterion_parse_sind_response (const gchar *response,
 
 /*****************************************************************************/
 /* ^SWWAN read parser
+ *
  * Description: Parses <cid>, <state>[, <WWAN adapter>] or CME ERROR from SWWAN.
- * Return: False if error occured while trying to parse SWWAN.
+ *
+ * The method returns a MMSwwanState with the connection status of a single
+ * PDP context, the one being queried via the cid given as input.
+ *
+ * Note that we use CID for matching because the WWAN adapter field is optional
+ * it seems.
+ *
  *     Read Command
  *         AT^SWWAN?
  *         Response(s)
@@ -505,61 +512,60 @@ mm_cinterion_parse_sind_response (const gchar *response,
  *         ^SWWAN: 3,1,1   - 3rd PDP Context, Activated, First WWAN Adaptor
  *         +CME ERROR: ?   -
  */
-
+MMSwwanState
-gboolean
 mm_cinterion_parse_swwan_response (const gchar  *response,
-                                   GList **result,
+                                   guint         cid,
                                    GError      **error)
 {
-    if (*error)
+    GRegex       *r;
-        return FALSE;
+    GMatchInfo   *match_info;
+    GError       *inner_error = NULL;
+    MMSwwanState  state;
 
-    if (!response) {
+    g_assert (response);
-        g_set_error (error,
+
-                     MM_CORE_ERROR,
+    /* If no WWAN connection is active, then ^SWWAN read command just returns OK
-                     MM_CORE_ERROR_FAILED,
+     * (which we receive as an empty string) */
-                     "Recieved NULL from SWWAN response.");
+    if (!response[0])
-        return FALSE;
+        return MM_SWWAN_STATE_DISCONNECTED;
+
+    if (!g_str_has_prefix (response, "^SWWAN:")) {
+        g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
+                     "Couldn't parse ^SWWAN response: '%s'", response);
+        return MM_SWWAN_STATE_UNKNOWN;
     }
 
-    /* Parse for [^SWWAN: <cid>, <state>[, <WWAN adapter>]] */
+    r = g_regex_new ("\\^SWWAN:\\s*(\\d+),\\s*(\\d+)(?:,\\s*(\\d+))?(?:\\r\\n)?",
-    if (strcasestr (response, "SWWAN")) {
+                     G_REGEX_DOLLAR_ENDONLY | G_REGEX_RAW, 0, NULL);
-        gint matched;
+    g_assert (r != NULL);
-        guint cid, state, wwan_adapter;
-        matched = sscanf (response, "^SWWAN: %d,%d,%d",
-                          &cid,
-                          &state,
-                          &wwan_adapter);
 
-        if (matched != 3 ||
+    state = MM_SWWAN_STATE_UNKNOWN;
-            cid < 1 || cid > 16 ||
+    g_regex_match_full (r, response, strlen (response), 0, 0, &match_info, &inner_error);
-            state < 0 || state > 1 ||
+    while (!inner_error && g_match_info_matches (match_info)) {
-            wwan_adapter < 1 || wwan_adapter > 2) {
+        guint read_state;
-            g_set_error (error,
+        guint read_cid;
-                         MM_CORE_ERROR,
+
-                         MM_CORE_ERROR_FAILED,
+        if (!mm_get_uint_from_match_info (match_info, 1, &read_cid))
-                         "Invalid format for SWWAN response: '%s'",
+            mm_warn ("Couldn't read cid in ^SWWAN response: '%s'", response);
-                         response);
+        else if (!mm_get_uint_from_match_info (match_info, 2, &read_state))
-            return FALSE;
+            mm_warn ("Couldn't read state in ^SWWAN response: '%s'", response);
+        else if (read_cid == cid) {
+            if (read_state == MM_SWWAN_STATE_CONNECTED || read_state == MM_SWWAN_STATE_DISCONNECTED) {
+                state = (MMSwwanState) read_state;
+                break;
+            }
+            mm_warn ("Invalid state read in ^SWWAN response: %u", read_state);
         }
 
-        *result = g_list_append (*result, GINT_TO_POINTER(cid));
+        g_match_info_next (match_info, &inner_error);
-        *result = g_list_append (*result, GINT_TO_POINTER(state));
-        *result = g_list_append (*result, GINT_TO_POINTER(wwan_adapter));
-    }
-    /* TODO: It'd be nice to get 'OK' from response so we don't have to assume that
-     * zero length response means 'OK' or am I doing it wrong?... */
-    else if (!g_utf8_strlen (response, 100))
-        *result = g_list_append (*result, GINT_TO_POINTER(0));
-    else {
-        g_set_error (error,
-                     MM_CORE_ERROR,
-                     MM_CORE_ERROR_FAILED,
-                     "Could not parse SWWAN response: '%s'",
-                     response);
-        return FALSE;
     }
 
+    g_match_info_free (match_info);
+    g_regex_unref (r);
 
-    return TRUE;
+    if (state == MM_SWWAN_STATE_UNKNOWN)
+        g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
+                     "No state returned for CID %u", cid);
+
+    return state;
 }

plugins/cinterion/mm-modem-helpers-cinterion.h

@@ -67,7 +67,15 @@ gboolean mm_cinterion_parse_sind_response (const gchar *response,
 
 /*****************************************************************************/
 /* ^SWWAN response parser */
-gboolean mm_cinterion_parse_swwan_response (const gchar *response,
+
-                                        GList **result,
+typedef enum {
+    MM_SWWAN_STATE_UNKNOWN      = -1,
+    MM_SWWAN_STATE_DISCONNECTED =  0,
+    MM_SWWAN_STATE_CONNECTED    =  1,
+} MMSwwanState;
+
+MMSwwanState mm_cinterion_parse_swwan_response (const gchar  *response,
+                                                guint         swwan_index,
                                                 GError      **error);
+
 #endif  /* MM_MODEM_HELPERS_CINTERION_H */

plugins/cinterion/tests/test-modem-helpers-cinterion.c

@@ -327,142 +327,158 @@ test_cnmi_phs8 (void)
     g_array_unref (expected_mt);
     g_array_unref (expected_bm);
     g_array_unref (expected_ds);
-    g_array_unref (expected_bfr);}
+    g_array_unref (expected_bfr);
+}
 
-static void
+/*****************************************************************************/
-test_swwan_parser (const GArray *expected_cid,
+/* Test ^SWWAN read */
-                   const GArray *expected_state,
+
-                   const GArray *expected_adapter,
+#define SWWAN_TEST_MAX_CIDS 2
-                   const gchar *at_cmd,
+
-                   const gboolean test_for_errors)
+typedef struct {
+    guint        cid;
+    MMSwwanState state;
+} PdpContextState;
+
+typedef struct {
+    const gchar     *response;
+    PdpContextState  expected_items[SWWAN_TEST_MAX_CIDS];
+    gboolean         skip_test_other_cids;
+} SwwanTest;
+
+/* Note: all tests are based on checking CIDs 2 and 3 */
+static const SwwanTest swwan_tests[] = {
+    /* No active PDP context reported (all disconnected) */
     {
-    GError *error = NULL;
+        .response = "",
-    gboolean res = TRUE;
+        .expected_items = {
-    gchar *response;
+            { .cid = 2, .state = MM_SWWAN_STATE_DISCONNECTED },
-    guint i, j, k;
+            { .cid = 3, .state = MM_SWWAN_STATE_DISCONNECTED }
-
+        },
-    g_assert (expected_cid != NULL);
+        /* Don't test other CIDs because for those we would also return
-    g_assert (expected_state != NULL);
+         * DISCONNECTED, not UNKNOWN. */
-    g_assert (expected_adapter != NULL);
+        .skip_test_other_cids = TRUE
-
+    },
-    /* For each expected_cid */
+    /* Single PDP context active (short version without interface index) */
-    for (i = 0; i < expected_cid->len; i++) {
+    {
-        /* For each expected_state */
+        .response = "^SWWAN: 3,1\r\n",
-        for (j = 0; j < expected_state->len; j++) {
+        .expected_items = {
-            /* For each expected_adapter */
+            { .cid = 2, .state = MM_SWWAN_STATE_UNKNOWN   },
-            for (k = 0; k < expected_adapter->len; k++) {
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED }
-                GList *response_parsed = NULL;
-
-                /* Build a unique at_cmd string */
-                response = g_strdup_printf ("%s: %i,%i,%i\r\n\r\n",
-                                            at_cmd,
-                                            g_array_index (expected_cid, guint, i),
-                                            g_array_index (expected_state, guint, j),
-                                            g_array_index (expected_adapter, guint, k));
-
-                /* and send it to the parser */
-                res = mm_cinterion_parse_swwan_response (response,
-                                                         &response_parsed,
-                                                         &error);
-
-                if (test_for_errors) {
-                    /* There should be errors raised */
-                    g_assert (res == FALSE);
-                    g_assert (error != NULL);
-
-                    /* reset the error's everytime */
-                    res = TRUE;
-                    g_clear_error (&error);
-                }
-                else {
-                    /* The parsed values we get back should match the AT string we sent */
-                    g_assert (g_array_index (expected_cid, guint, i) ==
-                              GPOINTER_TO_INT(g_list_nth_data (response_parsed, 0)));
-                    g_assert (g_array_index (expected_state, guint, j) ==
-                              GPOINTER_TO_INT(g_list_nth_data (response_parsed, 1)));
-                    g_assert (g_array_index (expected_adapter, guint, k) ==
-                              GPOINTER_TO_INT(g_list_nth_data (response_parsed, 2)));
-
-                    /* and there should be no errors raised */
-                    g_assert (res == TRUE);
-                    g_assert_no_error (error);
-                }
-
-                g_list_free(response_parsed);
         }
+    },
+    /* Single PDP context active (long version with interface index) */
+    {
+        .response = "^SWWAN: 3,1,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_UNKNOWN   },
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED }
         }
+    },
+    /* Single PDP context inactive (short version without interface index) */
+    {
+        .response = "^SWWAN: 3,0\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_UNKNOWN      },
+            { .cid = 3, .state = MM_SWWAN_STATE_DISCONNECTED }
+        }
+    },
+    /* Single PDP context inactive (long version with interface index) */
+    {
+        .response = "^SWWAN: 3,0,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_UNKNOWN      },
+            { .cid = 3, .state = MM_SWWAN_STATE_DISCONNECTED }
+        }
+    },
+    /* Multiple PDP contexts active (short version without interface index) */
+    {
+        .response = "^SWWAN: 2,1\r\n^SWWAN: 3,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_CONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED }
+        }
+    },
+    /* Multiple PDP contexts active (long version with interface index) */
+    {
+        .response = "^SWWAN: 2,1,3\r\n^SWWAN: 3,1,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_CONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED }
+        }
+    },
+    /* Multiple PDP contexts inactive (short version without interface index) */
+    {
+        .response = "^SWWAN: 2,0\r\n^SWWAN: 3,0\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_DISCONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_DISCONNECTED }
+        }
+    },
+    /* Multiple PDP contexts inactive (long version with interface index) */
+    {
+        .response = "^SWWAN: 2,0,3\r\n^SWWAN: 3,0,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_DISCONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_DISCONNECTED }
+        }
+    },
+    /* Multiple PDP contexts active/inactive (short version without interface index) */
+    {
+        .response = "^SWWAN: 2,0\r\n^SWWAN: 3,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_DISCONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED    }
+        }
+    },
+    /* Multiple PDP contexts active/inactive (long version with interface index) */
+    {
+        .response = "^SWWAN: 2,0,3\r\n^SWWAN: 3,1,1\r\n",
+        .expected_items = {
+            { .cid = 2, .state = MM_SWWAN_STATE_DISCONNECTED },
+            { .cid = 3, .state = MM_SWWAN_STATE_CONNECTED    }
         }
     }
+};
 
 static void
 test_swwan_pls8 (void)
 {
-    GArray *good_cid;
+    MMSwwanState  read_state;
-    GArray *good_state;
+    GError       *error = NULL;
-    GArray *good_adapter;
-    GArray *bad_cid;
-    GArray *bad_state;
-    GArray *bad_adapter;
     guint         i;
-    guint val;
 
-    /* AT^SWWAN=? -> '^SWWAN: (0,1),(1-16),(1,2)' */
+    /* Base tests for successful responses */
-    /* Setup array with good SWWAN values */
+    for (i = 0; i < G_N_ELEMENTS (swwan_tests); i++) {
-    good_cid = g_array_sized_new (FALSE, FALSE, sizeof (guint), 16);
+        guint j;
-    for (i = 1; i < 17; i++)
-        val = i, g_array_append_val (good_cid, val);
 
-    good_state = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2);
+        /* Query for the expected items (CIDs 2 and 3) */
-    val = 0, g_array_append_val (good_state, val);
+        for (j = 0; j < SWWAN_TEST_MAX_CIDS; j++) {
-    val = 1, g_array_append_val (good_state, val);
+            read_state = mm_cinterion_parse_swwan_response (swwan_tests[i].response, swwan_tests[i].expected_items[j].cid, &error);
-
+            if (swwan_tests[i].expected_items[j].state == MM_SWWAN_STATE_UNKNOWN) {
-    good_adapter = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2);
+                g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
-    val = 1, g_array_append_val (good_adapter, val);
+                g_clear_error (&error);
-    val = 2, g_array_append_val (good_adapter, val);
+            } else
-
+                g_assert_no_error (error);
-    /* and test */
+            g_assert_cmpint (read_state, ==, swwan_tests[i].expected_items[j].state);
-    test_swwan_parser (good_cid,
-                       good_state,
-                       good_adapter,
-                       "^SWWAN",
-                       FALSE);
-
-    /* Setup array with bad SWWAN values */
-    bad_cid = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2);
-    val = -1, g_array_append_val (bad_cid, val);
-    val = 17, g_array_append_val (bad_cid, val);
-
-    bad_state = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2);
-    val = -1, g_array_append_val (bad_state, val);
-    val = 2, g_array_append_val (bad_state, val);
-
-    bad_adapter = g_array_sized_new (FALSE, FALSE, sizeof (guint), 2);
-    val = -1, g_array_append_val (bad_adapter, val);
-    val = 0, g_array_append_val (bad_adapter, val);
-    val = 3, g_array_append_val (bad_adapter, val);
-
-    /* and test */
-    test_swwan_parser (bad_cid,
-                       bad_state,
-                       bad_adapter,
-                       "^SWWAN",
-                       TRUE);
-
-    /* and again with a bad cmd */
-    test_swwan_parser (bad_cid,
-                       bad_state,
-                       bad_adapter,
-                       "^GARBAGE",
-                       TRUE);
-
-
-    g_array_unref (good_cid);
-    g_array_unref (good_state);
-    g_array_unref (good_adapter);
-    g_array_unref (bad_cid);
-    g_array_unref (bad_state);
-    g_array_unref (bad_adapter);
         }
+
+        /* Query for a CID which isn't replied (e.g. 12) */
+        if (!swwan_tests[i].skip_test_other_cids) {
+            read_state = mm_cinterion_parse_swwan_response (swwan_tests[i].response, 12, &error);
+            g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
+            g_assert_cmpint (read_state, ==, MM_SWWAN_STATE_UNKNOWN);
+            g_clear_error (&error);
+        }
+    }
+
+    /* Additional tests for errors */
+    read_state = mm_cinterion_parse_swwan_response ("^GARBAGE", 2, &error);
+    g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
+    g_assert_cmpint (read_state, ==, MM_SWWAN_STATE_UNKNOWN);
+    g_clear_error (&error);
+}
+
 /*****************************************************************************/
 /* Test ^SIND responses */