Re: [MM] [PATCH] bearer: allow specifying default IP family for bearers



Thanks. Submitted a revised patch.

- Ben


On Mon, Apr 22, 2013 at 12:42 AM, Aleksander Morgado <aleksander lanedo com> wrote:
On 04/22/2013 03:50 AM, Ben Chan wrote:
> This patch adds a 'bearer-default-ip-family' property to MMBearer, which
> specifies the default IP family to use for a bearer when no explicit
> value is given via the simple connect properties. The default IP family
> is set to IPv4 in MMBearer but can be overridden by a MMBearer subclass,
> which allows a modem plugin to specify an appropriate default value.

The logic to use the new property was added only in the
MMBroadbandBearer subclass; all the other available MMBearer subclasses
(QMI, MBIM) should also get modified to use this new default value.

Some more comments below.


> ---
>  libmm-glib/mm-bearer-properties.c |  8 +-------
>  src/mm-bearer.c                   | 25 +++++++++++++++++++++++++
>  src/mm-bearer.h                   |  2 ++
>  src/mm-broadband-bearer.c         | 25 +++++++++++++++++++++----
>  4 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/libmm-glib/mm-bearer-properties.c b/libmm-glib/mm-bearer-properties.c
> index 6526ed0..2f59fee 100644
> --- a/libmm-glib/mm-bearer-properties.c
> +++ b/libmm-glib/mm-bearer-properties.c
> @@ -674,13 +674,7 @@ mm_bearer_properties_init (MMBearerProperties *self)
>      self->priv->allow_roaming = TRUE;
>      self->priv->rm_protocol = MM_MODEM_CDMA_RM_PROTOCOL_UNKNOWN;
>      self->priv->allowed_auth = MM_BEARER_ALLOWED_AUTH_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 = MM_BEARER_IP_FAMILY_IPV4;
> +    self->priv->ip_type = MM_BEARER_IP_FAMILY_UNKNOWN;
>  }
>
>  static void
> diff --git a/src/mm-bearer.c b/src/mm-bearer.c
> index 9e96bde..d047e11 100644
> --- a/src/mm-bearer.c
> +++ b/src/mm-bearer.c
> @@ -57,6 +57,7 @@ enum {
>      PROP_MODEM,
>      PROP_STATUS,
>      PROP_CONFIG,
> +    PROP_DEFAULT_IP_FAMILY,
>      PROP_LAST
>  };
>
> @@ -73,6 +74,8 @@ struct _MMBearerPrivate {
>      MMBearerStatus status;
>      /* Configuration of the bearer */
>      MMBearerProperties *config;
> +    /* Default IP family of this bearer */
> +    MMBearerIpFamily default_ip_family;
>
>      /* Cancellable for connect() */
>      GCancellable *connect_cancellable;
> @@ -844,6 +847,12 @@ mm_bearer_get_config (MMBearer *self)
>              NULL);
>  }
>
> +MMBearerIpFamily
> +mm_bearer_get_default_ip_family (MMBearer *self)
> +{
> +    return self->priv->default_ip_family;
> +}
> +
>  /*****************************************************************************/
>
>  static void
> @@ -969,6 +978,9 @@ set_property (GObject *object,
>              g_variant_unref (dictionary);
>          break;
>      }
> +    case PROP_DEFAULT_IP_FAMILY:
> +        self->priv->default_ip_family = g_value_get_enum (value);
> +        break;
>      default:
>          G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>          break;
> @@ -999,6 +1011,9 @@ get_property (GObject *object,
>      case PROP_CONFIG:
>          g_value_set_object (value, self->priv->config);
>          break;
> +    case PROP_DEFAULT_IP_FAMILY:
> +        g_value_set_enum (value, self->priv->default_ip_family);
> +        break;
>      default:
>          G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
>          break;
> @@ -1015,6 +1030,7 @@ mm_bearer_init (MMBearer *self)
>      self->priv->status = MM_BEARER_STATUS_DISCONNECTED;
>      self->priv->reason_3gpp = CONNECTION_FORBIDDEN_REASON_NONE;
>      self->priv->reason_cdma = CONNECTION_FORBIDDEN_REASON_NONE;
> +    self->priv->default_ip_family = MM_BEARER_IP_FAMILY_IPV4;
>
>      /* Set defaults */
>      mm_gdbus_bearer_set_interface (MM_GDBUS_BEARER (self), NULL);
> @@ -1111,6 +1127,15 @@ mm_bearer_class_init (MMBearerClass *klass)
>                               MM_TYPE_BEARER_PROPERTIES,
>                               G_PARAM_READWRITE);
>      g_object_class_install_property (object_class, PROP_CONFIG, properties[PROP_CONFIG]);
> +
> +    properties[PROP_DEFAULT_IP_FAMILY] =
> +        g_param_spec_enum (MM_BEARER_DEFAULT_IP_FAMILY,
> +                           "Bearer default IP family",
> +                           "IP family to use for this bearer when no IP family is specified",
> +                           MM_TYPE_BEARER_IP_FAMILY,
> +                           MM_BEARER_IP_FAMILY_IPV4,
> +                           G_PARAM_READWRITE);
> +    g_object_class_install_property (object_class, PROP_DEFAULT_IP_FAMILY, properties[PROP_DEFAULT_IP_FAMILY]);
>  }
>
>  /*****************************************************************************/
> diff --git a/src/mm-bearer.h b/src/mm-bearer.h
> index c617297..cc71bfd 100644
> --- a/src/mm-bearer.h
> +++ b/src/mm-bearer.h
> @@ -58,6 +58,7 @@ typedef struct _MMBearerPrivate MMBearerPrivate;
>  #define MM_BEARER_MODEM      "bearer-modem"
>  #define MM_BEARER_STATUS     "bearer-status"
>  #define MM_BEARER_CONFIG     "bearer-config"
> +#define MM_BEARER_DEFAULT_IP_FAMILY "bearer-deafult-ip-family"
>
>  typedef enum { /*< underscore_name=mm_bearer_status >*/
>      MM_BEARER_STATUS_DISCONNECTED,
> @@ -103,6 +104,7 @@ const gchar        *mm_bearer_get_path    (MMBearer *bearer);
>  MMBearerStatus      mm_bearer_get_status  (MMBearer *bearer);
>  MMBearerProperties *mm_bearer_peek_config (MMBearer *self);
>  MMBearerProperties *mm_bearer_get_config  (MMBearer *self);
> +MMBearerIpFamily mm_bearer_get_default_ip_family (MMBearer *self);
>
>
>  void     mm_bearer_connect        (MMBearer *self,
> diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
> index f380289..ebe48ea 100644
> --- a/src/mm-broadband-bearer.c
> +++ b/src/mm-broadband-bearer.c
> @@ -68,6 +68,18 @@ mm_broadband_bearer_get_3gpp_cid (MMBroadbandBearer *self)
>      return self->priv->cid;
>  }
>
> +static MMBearerIpFamily
> +mm_broadband_bearer_get_ip_family (MMBroadbandBearer *self)
> +{
> +    MMBearerIpFamily ip_family;
> +
> +    ip_family = mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (self)));
> +    if (ip_family != MM_BEARER_IP_FAMILY_UNKNOWN)
> +        return ip_family;
> +
> +    return mm_bearer_get_default_ip_family (MM_BEARER (self));
> +}

I wouldn't have a method like this, it's a bit misleading. Instead,
better just compute the actual ip_family value to use once, and keep it
in the DetailedConnectContext. Also, mm_log() about the default IP type
being used when UNKNOWN is given in the Bearer Properties.


> +
>  /*****************************************************************************/
>  /* Detailed connect context, used in both CDMA and 3GPP sequences */
>
> @@ -772,7 +784,7 @@ find_cid_ready (MMBaseModem *modem,
>          return;
>
>      /* Initialize PDP context with our APN */
> -    pdp_type = mm_3gpp_get_pdp_type_from_ip_family (mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self))));
> +    pdp_type = mm_3gpp_get_pdp_type_from_ip_family (mm_broadband_bearer_get_ip_family (ctx->self));
>      if (!pdp_type) {
>          g_simple_async_result_set_error (ctx->result,
>                                           MM_CORE_ERROR,
> @@ -838,6 +850,9 @@ parse_cid_range (MMBaseModem *modem,
>                       G_REGEX_DOLLAR_ENDONLY | G_REGEX_RAW,
>                       0, &inner_error);
>      if (r) {
> +        MMBearerIpFamily ip_family;
> +
> +        ip_family = mm_broadband_bearer_get_ip_family (ctx->self);
>          g_regex_match_full (r, response, strlen (response), 0, 0, &match_info, &inner_error);
>          cid = 0;
>          while (!inner_error &&
> @@ -847,8 +862,7 @@ parse_cid_range (MMBaseModem *modem,
>
>              pdp_type = g_match_info_fetch (match_info, 3);
>
> -            if (mm_3gpp_get_ip_family_from_pdp_type (pdp_type) ==
> -                mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self)))) {
> +            if (mm_3gpp_get_ip_family_from_pdp_type (pdp_type) == ip_family) {
>                  gchar *max_cid_range_str;
>                  guint max_cid_range;
>
> @@ -900,6 +914,7 @@ parse_pdp_list (MMBaseModem *modem,
>      GList *pdp_list;
>      GList *l;
>      guint cid;
> +    MMBearerIpFamily ip_family;
>
>      /* If cancelled, set result error */
>      if (detailed_connect_context_set_error_if_cancelled (ctx, result_error))
> @@ -938,11 +953,13 @@ parse_pdp_list (MMBaseModem *modem,
>                  pdp->apn ? pdp->apn : "");
>      }
>
> +    ip_family = mm_broadband_bearer_get_ip_family (ctx->self);
> +
>      /* Look for the exact PDP context we want */
>      for (l = pdp_list; l; l = g_list_next (l)) {
>          MM3gppPdpContext *pdp = l->data;
>
> -        if (pdp->pdp_type == mm_bearer_properties_get_ip_type (mm_bearer_peek_config (MM_BEARER (ctx->self)))) {
> +        if (pdp->pdp_type == ip_family) {
>              /* 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",
>


--
Aleksander



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