Re: [RFC v0] ofono: Only register network when APN is set

[Jussi Kukkonen <jussi.kukkonen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4639 bytes --]

On Tue, Feb 28, 2012 at 11:19 AM, Daniel Wagner <wagi@monom.org> wrote:
> From: Daniel Wagner <daniel.wagner@bmw-carit.de>
>
> We should now show a network without an APN.
> ---
> I have not tested this one. But something like this should do
> the trick. Maybe someone with deeper knowledge on the APN
> behavior could explain under which circumstances the APN is set, e.g.
> see the netreg vs apn setting in this patch. Not sure if this is correct.
>
> cheers,
> daniel

Thanks Daniel. This sort of works after some fixes but it looks like
there are still issues if modem or context properties change. I'll
have a go a fixing it later today. I'm including my initial comments
below for reference (just in case I don't manage to fix them).

Also while I remember: cm_context_added() does a lookup on modem_hash
when it probably shoud use context_hash. I'll include this in the
patches.

>  plugins/ofono.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index d87d7b6..c92c3cc 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -105,6 +105,9 @@ enum ofono_api {
>  * the plugin about IP configuration through the updating the context's
>  * properties.
>  *
> + * The network is only registered at the core when the AccessPointName
> + * has been set.
> + *
>  * CDMA working flow:
>  *
>  * When a new modem appears, the plugin always powers it up. This
> @@ -172,6 +175,7 @@ struct modem_data {
>        /* ConnectionContext Interface */
>        connman_bool_t active;
>        connman_bool_t set_active;
> +       char *apn;

probably makes sense to have this in network_context -- easier to keep
them in sync if e.g. context disappears.

>
>        /* SimManager Interface */
>        char *imsi;
> @@ -1105,6 +1109,10 @@ static int add_cm_context(struct modem_data *modem, const char *context_path,
>                        dbus_message_iter_get_basic(&value, &active);
>
>                        DBG("%s Active %d", modem->path, active);
> +               } else if (g_str_equal(key, "AccessPointName") == TRUE) {
> +                       dbus_message_iter_get_basic(&value, &modem->apn);

copying needed.

> +
> +                       DBG("%s AccessPointName %s", modem->path, modem->apn);
>                }
>
>                dbus_message_iter_next(dict);
> @@ -1180,6 +1188,19 @@ static gboolean context_changed(DBusConnection *connection,
>                        set_connected(modem);
>                else
>                        set_disconnected(modem);
> +       } else if (g_str_equal(key, "AccessPointName") == TRUE) {
> +               g_free(modem->apn);
> +
> +               dbus_message_iter_get_basic(&value, &modem->apn);
> +
> +               DBG("%s AccessPointName %s", modem->path, modem->apn);

copy needed as well

> +
> +               if (has_interface(modem->interfaces,
> +                                       OFONO_API_NETREG) == TRUE &&
> +                               modem->network != NULL) {
> +                       DBG("Register network at core");
> +                       add_network(modem);
> +               }

* if a network exists and apn is empty -> remove_network()
* if a network does not exist and netreg iface is supported  and apn
is not empty -> add_network()

also, probably need to call set_connected() if Active is set?

>        }
>
>        return TRUE;
> @@ -1518,6 +1539,9 @@ static void netreg_properties_reply(struct modem_data *modem,
>                return;
>        }
>
> +       if (modem->apn == NULL)
> +               return;
> +

This doesn't actually work as the 'empty' apn is "". Now that I think
about it, it would be safer and easier to just save a boolean
apn_is_valid...

>        add_network(modem);
>
>        if (modem->active == TRUE)
> @@ -2187,6 +2211,7 @@ static void remove_modem(gpointer data)
>        g_free(modem->serial);
>        g_free(modem->name);
>        g_free(modem->imsi);
> +       g_free(modem->apn);
>        g_free(modem->path);
>
>        g_free(modem);
> --
> 1.7.9.48.g85da4d
>
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono