broadband-bearer: prefer unused CID to overwriting an existing one

There are two places where we look for unused CIDs:

 * First, while processing the list of existing contexts returned by
   CGDCONT?, we look for gaps in the used CIDs. E.g. if the first
   context has CID=1 and the next one has CID=3, we can definitely use
   the unused CID=2.

 * Then, while processing the response of CGDCONT=?, we try to see
   whether there is any empty CID available after the last existing
   one found. E.g. if the last existing context has CID=3 and the
   format tells us that the max allowed CID is 16, we can definitely
   use the unused CID=4.

In both these cases, we should prefer using such an unused CID found
to overwriting other CIDs that are already defined.

This logic will now overwrite existing CIDs only if there are no
unused CIDs, and the preference to overwrite is as follows:

 * If there is any existing context defined without an explicit APN,
   overwrite it.

 * Otherwise, overwrite the last existing CID found.

 * And in the worst case, when no list of contexts was loaded
   properly (e.g. some Android phones don't allow querying), fallback
   to overwriting CID=1.

1 files changed, 71 insertions, 45 deletions

diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
index b772c7a8..d93fa529 100644
--- a/src/mm-broadband-bearer.c
+++ b/src/mm-broadband-bearer.c
@@ -607,6 +607,7 @@ typedef struct {
     GCancellable      *cancellable;
     guint              cid;
     guint              max_cid;
+    guint              blank_cid;
     gboolean           use_existing_cid;
     MMBearerIpFamily   ip_family;
 } CidSelection3gppContext;
@@ -724,7 +725,7 @@ parse_cid_range (MMBaseModem              *modem,
 {
     GError *inner_error = NULL;
     GList  *formats, *l;
-    guint   cid;
+    guint   unused_cid;
 
     /* If cancelled, set result error */
     if (g_cancellable_is_cancelled (ctx->cancellable)) {
@@ -735,21 +736,20 @@ parse_cid_range (MMBaseModem              *modem,
 
     if (error) {
         mm_dbg ("Unexpected +CGDCONT error: '%s'", error->message);
-        mm_dbg ("Defaulting to CID=1");
-        ctx->cid = 1;
-        return TRUE;
+        goto out;
     }
 
     formats = mm_3gpp_parse_cgdcont_test_response (response, &inner_error);
     if (inner_error) {
         mm_dbg ("Error parsing +CGDCONT test response: '%s'", inner_error->message);
-        mm_dbg ("Defaulting to CID=1");
         g_error_free (inner_error);
-        ctx->cid = 1;
-        return TRUE;
+        goto out;
     }
 
-    cid = 0;
+    /* We need to look for the max allowed cid in the IP family we need, to see if
+     * we can use the next available CID */
+    unused_cid = 0;
+
     for (l = formats; l; l = g_list_next (l)) {
         MM3gppPdpContextFormat *format = l->data;
 
@@ -758,12 +758,11 @@ parse_cid_range (MMBaseModem              *modem,
             gchar *ip_family_str;
 
             ip_family_str = mm_bearer_ip_family_build_string_from_mask (format->pdp_type);
-            if (ctx->max_cid < format->max_cid) {
-                cid = ctx->max_cid + 1;
-                mm_dbg ("Using empty CID %u with PDP type '%s'", cid, ip_family_str);
-            } else {
-                cid = ctx->max_cid;
-                mm_dbg ("Re-using CID %u (max) with PDP type '%s'", cid, ip_family_str);
+            /* If the max existing CID found during CGDCONT? is below the max allowed
+             * CID, then we can use the next available CID because it's an unused one. */
+            if (ctx->max_cid && (ctx->max_cid < format->max_cid)) {
+                unused_cid = ctx->max_cid + 1;
+                mm_dbg ("Found an unused PDP context at CID %u", unused_cid);
             }
             g_free (ip_family_str);
             break;
@@ -772,12 +771,31 @@ parse_cid_range (MMBaseModem              *modem,
 
     mm_3gpp_pdp_context_format_list_free (formats);
 
-    if (cid == 0) {
-        mm_dbg ("Defaulting to CID=1");
-        cid = 1;
+    /* If an unused CID is found available after the last existing one, use it */
+    if (unused_cid) {
+        ctx->cid = unused_cid;
+        return TRUE;
+    }
+
+out:
+
+    /* Otherwise, rewrite a context defined with no APN, if any */
+    if (ctx->blank_cid) {
+        mm_dbg ("Rewriting first found context without APN set at CID %u", ctx->max_cid);
+        ctx->cid = ctx->blank_cid;
+        return TRUE;
     }
 
-    ctx->cid = cid;
+    /* Otherwise, rewrite the last existing one found */
+    if (ctx->max_cid) {
+        mm_dbg ("Rewriting last found context at CID %u", ctx->max_cid);
+        ctx->cid = ctx->max_cid;
+        return TRUE;
+    }
+
+    /* Otherwise, just fallback to CID=1 */
+    mm_dbg ("Defaulting to CID=1");
+    ctx->cid = 1;
     return TRUE;
 }
 
@@ -794,7 +812,8 @@ parse_pdp_list (MMBaseModem             *modem,
     GError *inner_error = NULL;
     GList *pdp_list;
     GList *l;
-    guint cid;
+    guint exact_cid;
+    guint unused_cid;
     guint prev_cid;
 
     /* If cancelled, set result error */
@@ -832,9 +851,6 @@ parse_pdp_list (MMBaseModem             *modem,
         return FALSE;
     }
 
-    cid = 0;
-    prev_cid = 0;
-
     /* Show all found PDP contexts in debug log */
     mm_dbg ("Found '%u' PDP contexts", g_list_length (pdp_list));
     for (l = pdp_list; l; l = g_list_next (l)) {
@@ -850,6 +866,10 @@ parse_pdp_list (MMBaseModem             *modem,
     }
 
     /* Look for the exact PDP context we want */
+    exact_cid = 0;
+    unused_cid = 0;
+    prev_cid  = 0;
+
     for (l = pdp_list; l; l = g_list_next (l)) {
         MM3gppPdpContext *pdp = l->data;
 
@@ -866,38 +886,32 @@ parse_pdp_list (MMBaseModem             *modem,
                 ip_family_str = mm_bearer_ip_family_build_string_from_mask (pdp->pdp_type);
                 mm_dbg ("Found PDP context with CID %u and PDP type %s for APN '%s'",
                         pdp->cid, ip_family_str, apn ? apn : "");
-                cid = pdp->cid;
-                ctx->use_existing_cid = TRUE;
+                exact_cid = pdp->cid;
                 g_free (ip_family_str);
                 /* In this case, stop searching */
                 break;
             }
 
-            /* If an open CID was not found yet and the previous CID is not (CID - 1),
-             * this means that (previous CID + 1) is a blank CID that can be used.
+            /* If an unused CID was not found yet and the previous CID is not (CID - 1),
+             * this means that (previous CID + 1) is an unused CID that can be used.
+             * This logic will allow us using unused CIDs that are available in the gaps
+             * between already defined contexts.
              */
-            if (prev_cid != G_MAXUINT && (prev_cid + 1) < pdp->cid) {
-                mm_dbg ("Found the first unused PDP context with CID %u", (prev_cid + 1));
-                cid = prev_cid + 1;
-
-                /* Set prev_cid to G_MAXUINT to show an open CID was found and only an
-                 * exact APN match would change the CID after
-                 */
-                prev_cid = G_MAXUINT;
-            } else {
-                /* PDP with no APN set? we may use that one if not exact match found */
-                if (!pdp->apn || !pdp->apn[0]) {
-                    mm_dbg ("Found PDP context with CID %u and no APN",
-                            pdp->cid);
-                    cid = pdp->cid;
-                }
+            if (!unused_cid && prev_cid && ((prev_cid + 1) < pdp->cid)) {
+                unused_cid = prev_cid + 1;
+                mm_dbg ("Found the first unused PDP context with CID %u", unused_cid);
+            }
+            /* PDP with no APN set? we may use that one if no exact match found */
+            else if ((!pdp->apn || !pdp->apn[0]) && !ctx->blank_cid) {
+                ctx->blank_cid = pdp->cid;
+                mm_dbg ("Found the first PDP context with no APN with CID %u", ctx->blank_cid);
             }
         }
 
         /* Update previous CID value to the current CID for use in the next loop,
-         * unless an unused CID was found
+         * unless an unused CID was already found.
          */
-        if (prev_cid != G_MAXUINT)
+        if (!unused_cid)
             prev_cid = pdp->cid;
 
         if (ctx->max_cid < pdp->cid)
@@ -905,11 +919,23 @@ parse_pdp_list (MMBaseModem             *modem,
     }
     mm_3gpp_pdp_context_list_free (pdp_list);
 
-    if (cid > 0) {
-        ctx->cid = cid;
+    /* Always prefer an exact match */
+    if (exact_cid) {
+        ctx->use_existing_cid = TRUE;
+        ctx->cid = exact_cid;
+        return TRUE;
+    }
+
+    /* Otherwise, try to use an empty CID detected in between the already
+     * defined contexts */
+    if (unused_cid) {
+        ctx->cid = unused_cid;
         return TRUE;
     }
 
+    /* And otherwise, fallback to running CGDCONT=? to look for the max
+     * range of CIDs and try to use the next one available after the last
+     * existing one. */
     return FALSE;
 }