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

[Aleksander Morgado <aleksander lanedo com>]

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\"",
>                                  ctx->cid,
> +                               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,
>                                      ctx->primary,
> @@ -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->cid,
>                   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",
>                           pdp->cid);
>                   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 */
>                       break;

Cheers!

--
Aleksander