Re: [PATCH] [MM 0.7] Derive PDP type from the ip-type bearer property

Hi Tore,

This patch makes it possible to use MM to set up PDP contexts with
PDP types other than 'IP', which is particularly useful when trying
to use the 'IPV6' or 'IPV4V6' PDP types defined in recent 3GPP specs.

If ip-type isn't specified, 'IP' will be used by default, due to the
fact that modem support for the 'IPV4V6' type is still rather scarce.

The patch applies to Aleksander's 'bearer-properties' branch. It has
been tested using mmcli in this fashion:

mmcli -m 0 --simple-connect=apn=internet # default to 'IP'
mmcli -m 0 --simple-connect=apn=internet,ip-type=IP
mmcli -m 0 --simple-connect=apn=internet,ip-type=IPV6
mmcli -m 0 --simple-connect=apn=internet,ip-type=IPV4V6
  libmm-common/mm-bearer-properties.c |    9 +++++++--
  src/mm-broadband-bearer.c           |   21 ++++++++++++---------
  2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/libmm-common/mm-bearer-properties.c b/libmm-common/mm-bearer-properties.c
index 9afa07f..2097834 100644
--- a/libmm-common/mm-bearer-properties.c
+++ b/libmm-common/mm-bearer-properties.c
@@ -85,7 +85,6 @@ mm_bearer_properties_set_ip_type (MMBearerProperties *self,
      g_return_if_fail (MM_IS_BEARER_PROPERTIES (self));

-    g_free (self->priv->ip_type);

self->ip_type needs to be allocated in heap always or never. The original approach was to have it always in heap, and therefore always set with a g_strdup() or similar; and that then requires to free the memory with g_free() or the program will be leaking memory. So you shouldn't remove the g_free() here.

      self->priv->ip_type = g_strdup (ip_type);

@@ -473,6 +472,13 @@ mm_bearer_properties_init (MMBearerProperties *self)
      /* Some defaults */
      self->priv->allow_roaming = TRUE;
      self->priv->rm_protocol = MM_MODEM_CDMA_RM_PROTOCOL_UNKNOWN;
+    /* At some point in the future, this default should probably be changed
+     * to IPV4V6. However, presently support for this PDP type is rare. An
+     * even better approach would likely be to query which PDP types the
+     * modem supports (using AT+CGDCONT=?), and set the default accordingly
+     */
+    self->priv->ip_type = "IP";

I'm guessing that you removed the g_free() before because it was crashing afterwards when you tried to g_free() this "IP" string that you're setting here. In order to avoid that, g_free() should receive always a string allocated in heap:
 self->priv->ip_type = g_strdup ("IP");


  static void
@@ -483,7 +489,6 @@ finalize (GObject *object)
      g_free (self->priv->apn);
      g_free (self->priv->user);
      g_free (self->priv->password);
-    g_free (self->priv->ip_type);

Same here, don't remove it.

      g_free (self->priv->number);

      G_OBJECT_CLASS (mm_bearer_properties_parent_class)->finalize (object);
diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
index 9f8c4aa..126de7e 100644
--- a/src/mm-broadband-bearer.c
+++ b/src/mm-broadband-bearer.c
@@ -758,8 +758,9 @@ find_cid_ready (MMBaseModem *modem,

      /* Initialize PDP context with our APN */
      ctx->cid = g_variant_get_uint32 (result);
-    command = g_strdup_printf ("+CGDCONT=%u,\"IP\",\"%s\"",
+    command = g_strdup_printf ("+CGDCONT=%u,\"%s\",\"%s\"",
+                               mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self))),
                                 mm_bearer_properties_get_apn (mm_bearer_peek_config (MM_BEARER (ctx->self))));

There's some misalignment here, probably coming from the previous source code; could you fix that while you're at it?

      mm_base_modem_at_command_full (ctx->modem,
@@ -818,8 +819,7 @@ parse_cid_range (MMBaseModem *modem,

              pdp_type = g_match_info_fetch (match_info, 3);

-            /* TODO: What about PDP contexts of type "IPV6"? */
-            if (g_str_equal (pdp_type, "IP")) {
+            if (g_str_equal (pdp_type, mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self))))) {
                  gchar *max_cid_range_str;
                  guint max_cid_range;

@@ -905,21 +905,24 @@ parse_pdp_list (MMBaseModem *modem,
                  pdp->pdp_type ? pdp->pdp_type : "",
                  pdp->apn ? pdp->apn : "");
-        if (g_str_equal (pdp->pdp_type, "IP")) {
+        if (g_str_equal (pdp->pdp_type, mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self))))) {
              /* 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",
                  cid = pdp->cid;
              } else {
-                const gchar *apn;
+                const gchar *apn, *pdp_type;

                  apn = mm_bearer_properties_get_apn (mm_bearer_peek_config (MM_BEARER (ctx->self)));
+                pdp_type = mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self)));
                  if (apn&&
-                    g_str_equal (pdp->apn, apn)) {
-                    /* Found a PDP context with the same CID, we'll use it. */
-                    mm_dbg ("Found PDP context with CID %u for APN '%s'",
-                            pdp->cid, pdp->apn);
+                    g_str_equal (pdp->apn, apn)&&
+                    pdp_type&&
+                    g_str_equal (pdp->pdp_type, pdp_type)) {

It seems you're comparing pdp->pdp_type with the pdp_type retrieved from the bearer config twice here (you already did it some lines before in the previous g_str_equal()). So no need for the pdp_type variable all together.

+                    /* Found a PDP context with the same CID and PDP type, we'll use it. */
+                    mm_dbg ("Found PDP context with CID %u and PDP type %s for APN '%s'",
+                            pdp->cid, pdp->pdp_type, pdp->apn);
                      cid = pdp->cid;
                      /* In this case, stop searching */



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]