[Merge] lp:~phablet-team/network-manager/lp1361864 into lp:~phablet-team/network-manager/vivid-phone-overlay
Tony Espy
espy at canonical.com
Mon Jul 6 15:44:32 UTC 2015
Comments addressed, will re-push shortly after a quick sanity test.
Diff comments:
> === modified file 'debian/patches/series'
> --- debian/patches/lp1361864-add-ofono-preferred-contexts.patch 1970-01-01 00:00:00 +0000
> +++ debian/patches/lp1361864-add-ofono-preferred-contexts.patch 2015-07-06 00:07:47 +0000
> @@ -0,0 +1,527 @@
> +Index: network-manager-0.9.10.0/src/settings/plugins/ofono/plugin.c
> +===================================================================
> +--- network-manager-0.9.10.0.orig/src/settings/plugins/ofono/plugin.c
> ++++ network-manager-0.9.10.0/src/settings/plugins/ofono/plugin.c
> +@@ -110,63 +110,107 @@ ignore_cb ()
> + {
> + }
> +
> +-static
> ++static void
> + SCPluginOfono_parse_contexts (SCPluginOfono *self, GSList *contexts)
> + {
> + SCPluginOfonoPrivate *priv = SC_PLUGIN_OFONO_GET_PRIVATE (self);
> + GSList *list;
> ++ GList *keys;
> ++ GHashTable *uuids = NULL;
> + NMOfonoConnection *exported;
> +- NMSettingConnection *setting;
> ++ gboolean found = FALSE;
> ++ char *uuid;
> ++ const char *imsi;
> ++
> ++ if (uuids == NULL)
> ++ uuids = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
Fixed.
> +
> + list = contexts;
> + while (list) {
> + GHashTable *context = (GHashTable *) list->data;
> +- const char *id, *type, *name, *imsi;
> +- char *idstr, *uuid;
> ++ const char *id, *name;
> ++ char *idstr;
> +
> + id = g_hash_table_lookup (context, "ID");
> + imsi = g_hash_table_lookup (context, "IMSI");
> +- type = g_hash_table_lookup (context, "Type");
> + name = g_hash_table_lookup (context, "Name");
> +
> + idstr = g_strconcat ("/", imsi, "/", id, NULL);
> + uuid = nm_utils_uuid_generate_from_string (idstr);
> + g_free (idstr);
> +
> +- if(!strcmp ("internet", type)) {
> +- nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: found %s context '%s' (%s)",
> +- type, name, id);
> ++ g_hash_table_insert (uuids, g_strdup (uuid), NULL);
> +
> +- /* Ignore any connection for this block that was previously found */
> +- exported = g_hash_table_lookup (priv->connections, uuid);
> +- if (exported) {
> +- /*
> +- PLUGIN_PRINT("SCPlugin-Ofono", "deleting %s from connections", id);
> +- nm_settings_connection_delete (NM_SETTINGS_CONNECTION (exported), ignore_cb, NULL);
> +- g_hash_table_remove (priv->connections, id);
> +- */
> +- goto next;
> +- }
> ++ nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: found internet context '%s' (%s)",
> ++ name, id);
> ++
> ++ /* Ignore any connection for this block that was previously found */
> ++ exported = g_hash_table_lookup (priv->connections, uuid);
> ++ if (exported) {
> ++ nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: context '%s' (%s) already exported",
> ++ name, id);
> ++ goto next;
> ++ }
> +
> +- /* add the new connection */
> +- exported = nm_ofono_connection_new (context);
> ++ /* add the new connection */
> ++ exported = nm_ofono_connection_new (context);
> ++ if (exported) {
> ++ nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: adding %s (%s) to connections",
As nm_ofono_connection_new () just wraps a call to g_object_new (), and g_object () new can never fail, this check doesn't really accomplish anything. This check was in existing code, however I'll go ahead and remove.
> ++ name, uuid);
> +
> ++ /* FIXME: investigate if it's possible to set directly via g_object_* method... */
> + /* Lower disabled timer to 30s */
> + nm_settings_connection_set_reset_retries_timeout (NM_SETTINGS_CONNECTION (exported), 30);
> +
> +- setting = nm_connection_get_setting_connection (NM_CONNECTION (exported));
> +- g_object_set (setting, NM_SETTING_CONNECTION_AUTOCONNECT, TRUE, NULL);
> +- if (exported) {
> +- nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: adding %s (%s) to connections",
> +- name, uuid);
> +- g_hash_table_insert (priv->connections, g_strdup (uuid), exported);
> +- g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, exported);
> +- }
> ++ g_hash_table_insert (priv->connections, g_strdup (uuid), exported);
> ++ g_signal_emit_by_name (self, NM_SYSTEM_CONFIG_INTERFACE_CONNECTION_ADDED, exported);
> + }
> ++
> + next:
> + list = list->next;
> +- g_free (uuid);
> + }
> ++
> ++ /*
> ++ * Remove any connections that have the same IMSI
> ++ * as our current list of contexts *and* are not
> ++ * present in our current list ( ie. the context
> ++ * has been deleted ).
> ++ *
> ++ * TODO: note, could this be handled directly by
> ++ * the imsi_monitor??? If so, it gets rid of
> ++ * this loop. Doing so would require caching
> ++ * the preferred contexts for each IMSI
> ++ */
> ++
> ++ keys = g_hash_table_get_keys (priv->connections);
> ++
> ++ while (keys) {
> ++ char **context_id;
Changed.
> ++ const char *idstr;
> ++
> ++ uuid = (char *) keys->data;
> ++
> ++ found = g_hash_table_lookup_extended (uuids, uuid, NULL, NULL);
> ++ if (!found) {
> ++ exported = g_hash_table_lookup (priv->connections, uuid);
> ++ idstr = nm_connection_get_id (NM_CONNECTION (exported));
> ++ context_id = g_strsplit (idstr, "/", 0);
> ++ g_assert (context_id[2]);
Nice catch on the missing g_strfreev(); fixed.
As for the IMSI check, this required because of what I consider an overall design flaw. At startup, the ofono settings plugin reads every IMSI directory it finds, even if the SIM is no longer present in the phone. It creates system connections for every one of the contexts it finds. If I deleted every not-found connection in priv->connections, it would cause all connections to be removed, including potentially an active context if the user was editing the APNs for a SIM that wasn't currently active. Note, this was originally not allowed, but was changed to fix lp: #1388046.
> ++
> ++ if (g_strcmp0(imsi, context_id[1]) == 0) {
> ++
> ++ nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: removing (%s) from connections", idstr);
> ++
> ++ nm_settings_connection_signal_remove (NM_SETTINGS_CONNECTION (exported));
> ++ g_hash_table_remove (priv->connections, uuid);
> ++ }
> ++ }
> ++
> ++ keys = keys->next;
> ++ }
> ++
> ++ if (uuids)
> ++ g_hash_table_destroy (uuids);
> + }
> +
> + static gboolean
> +@@ -174,6 +218,7 @@ nm_ofono_read_imsi_contexts (SCPluginOfo
> + {
> + SCPluginOfonoPrivate *priv = SC_PLUGIN_OFONO_GET_PRIVATE (self);
> + GHashTable *context;
> ++ GHashTable *pref_context = NULL;
> + GSList *contexts = NULL;
> + GDir *imsi_dir;
> + const char *file;
> +@@ -223,20 +268,58 @@ nm_ofono_read_imsi_contexts (SCPluginOfo
> + continue;
> + }
> +
> +- nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: add context for %s", imsi);
> + context = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free);
> + g_hash_table_insert (context, "ID", g_strdup (groups[i]));
> + g_hash_table_insert (context, "IMSI", g_strdup (imsi));
> +
> + for (j = 0; keys[j]; j++) {
> +- g_hash_table_insert (context, keys[j],
> +- g_key_file_get_string (keyfile, groups[i], keys[j], NULL));
> ++ gchar *prop_value;
> ++
> ++ prop_value = g_key_file_get_string (keyfile, groups[i], keys[j], NULL);
> ++
> ++ /* FIXME: if file notify fires multiple times when 'pref' is updated,
> ++ * need someway to cache the new 'Pref' value, so subequent file changes
> ++ * are just ignored if 'Pref' hasn't changed...
> ++ */
> ++
> ++ if (!strcmp (keys[j], "Type") && strcmp (prop_value, "internet")) {
> ++
> ++ g_hash_table_destroy (context);
> ++ g_free (prop_value);
> ++
> ++ goto next_context;
> ++ }
> ++
> ++ /* If more than one context is 'Preferred', last one wins... */
> ++ if (!strcmp (keys[j], "Preferred") && !strcmp (prop_value, "true")) {
Fixed.
> ++ nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: '%s' - Preferred = 'true'", groups[i]);
> ++
> ++ pref_context = context;
> ++ }
> ++
> ++ g_hash_table_insert (context, keys[j], prop_value);
> ++
> + nm_log_info (LOGD_SETTINGS, "SCPlugin-Ofono: add property '%s': %s",
> +- keys[j],
> +- (char *) g_hash_table_lookup (context, keys[j]));
> ++ keys[j], prop_value);
> + }
> +
> +- contexts = g_slist_append (contexts, context);
> ++ if (pref_context != NULL) {
> ++ /*
> ++ * Preferred context found, free any contexts created
> ++ * before the preferred context was found.
> ++ */
> ++
> ++ if (contexts) {
> ++ g_slist_free_full (contexts, (GDestroyNotify) g_hash_table_destroy);
> ++ contexts = NULL;
> ++ }
> ++
> ++ contexts = g_slist_append (contexts, pref_context);
> ++ break;
> ++ } else
> ++ contexts = g_slist_append (contexts, context);
> ++next_context:
> ++ ;
> + }
> +
> + g_key_file_free (keyfile);
> +@@ -282,7 +365,7 @@ SCPluginOfono_should_ignore_imsi (const
> + return FALSE;
> + }
> +
> +-static
> ++static void
> + ofono_imsi_changed (GFileMonitor *monitor,
> + GFile *file,
> + GFile *other_file,
> +@@ -298,8 +381,8 @@ ofono_imsi_changed (GFileMonitor *monito
> + path = g_file_get_path (file);
> +
> + /* If this isn't about a "gprs" file we don't want to know */
> +- if (g_strrstr (path, "gprs") == NULL)
> +- goto out_imsi;
> ++ if (g_strrstr (path, "gprs") == NULL)
> ++ goto out_imsi;
> +
> + switch (event_type) {
> + case G_FILE_MONITOR_EVENT_DELETED:
> +@@ -320,6 +403,7 @@ ofono_imsi_changed (GFileMonitor *monito
> + g_free (imsi);
> + break;
> + default:
> ++ nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: unexpected event type '%d'", (int) event_type);
> + break;
> + }
> +
> +@@ -329,7 +413,65 @@ out_imsi:
> + return;
> + }
> +
> +-static
> ++static gboolean
> ++add_gprs_file_watch(SCPluginOfono *self,
> ++ const char *imsi)
> ++{
> ++
> ++ SCPluginOfonoPrivate *priv = SC_PLUGIN_OFONO_GET_PRIVATE (self);
> ++ gchar *path;
> ++ GFile *config_path;
> ++ GFileMonitor *imsi_monitor;
> ++ gulong id;
> ++ const char *id_str;
> ++ gboolean result = FALSE;
> ++
> ++ id_str = g_hash_table_lookup (priv->ofono_imsi_monitor_ids, imsi);
> ++ if (id_str != NULL) {
> ++ nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: file_monitor already exists for %s", imsi);
> ++ goto done;
> ++ }
> ++
> ++ path = g_strdup_printf (OFONO_CONFIG_DIR "/%s", imsi);
> ++
> ++ /*
> ++ * TODO: an optimizztion woudld be to add only monitor the directory
> ++ * if /<IMSI>/gprs doesn't yet exist. Otherwise, a regular file
Fixed.
> ++ * monitor could be used, cutting down the times NM gets notified
> ++ * for changes to other ofono settings files...
> ++ */
> ++
> ++ config_path = g_file_new_for_path (path);
> ++ imsi_monitor = g_file_monitor_directory (config_path,
> ++ G_FILE_MONITOR_NONE,
> ++ NULL, NULL);
> ++
> ++ g_object_unref (config_path);
> ++ g_free (path);
> ++
> ++ if (imsi_monitor) {
> ++ nm_log_info (LOGD_SETTINGS, "SCPluginOfono: watching file changes for %s", imsi);
> ++ id = g_signal_connect (imsi_monitor, "changed",
> ++ G_CALLBACK (ofono_imsi_changed),
> ++ self);
> ++ g_hash_table_insert (priv->ofono_imsi_monitors,
> ++ g_strdup (imsi),
> ++ g_object_ref (imsi_monitor));
> ++ g_hash_table_insert (priv->ofono_imsi_monitor_ids,
> ++ g_strdup (imsi),
> ++ (gpointer) id);
> ++ g_object_unref (imsi_monitor);
> ++
> ++ result = TRUE;
> ++ } else {
> ++ nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: couldn't create file monitor for %s.", imsi);
> ++ }
> ++
> ++done:
> ++ return result;
> ++}
> ++
> ++static void
> + ofono_dir_changed (GFileMonitor *monitor,
> + GFile *file,
> + GFile *other_file,
> +@@ -346,7 +488,8 @@ ofono_dir_changed (GFileMonitor *monitor
> + GError *error = NULL;
> +
> + imsi = g_file_get_basename (file);
> +- if (SCPluginOfono_should_ignore_imsi (imsi))
> ++
> ++ if (SCPluginOfono_should_ignore_imsi (imsi))
> + goto out_ofono;
Fixed.
> +
> + switch (event_type) {
> +@@ -369,38 +512,22 @@ ofono_dir_changed (GFileMonitor *monitor
> + case G_FILE_MONITOR_EVENT_CREATED:
> + case G_FILE_MONITOR_EVENT_CHANGED:
> + case G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT:
> +- /* Add watches for the IMSI directories too */
> +- if (g_strrstr (imsi, "gprs") == NULL) {
> +- path = g_strdup_printf (OFONO_CONFIG_DIR "/%s", imsi);
> +- config_path = g_file_new_for_path (path);
> +- imsi_monitor = g_file_monitor_directory (config_path,
> +- G_FILE_MONITOR_NONE,
> +- NULL, NULL);
> +- g_object_unref (config_path);
> +- g_free (path);
> +-
> +- if (imsi_monitor) {
> +- nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: watching file changes for %s", imsi);
> +- id = g_signal_connect (monitor, "changed",
> +- G_CALLBACK (ofono_imsi_changed),
> +- self);
> +- g_hash_table_insert (priv->ofono_imsi_monitors,
> +- g_strdup (imsi),
> +- g_object_ref (imsi_monitor));
> +- g_hash_table_insert (priv->ofono_imsi_monitor_ids,
> +- g_strdup (imsi),
> +- (gpointer) id);
> +- g_object_unref (imsi_monitor);
> +- }
> +- }
> +
> +- res = nm_ofono_read_imsi_contexts (self, imsi, &error);
> ++ /* TODO: is this a valid test? If only new dirs and/or files created in
> ++ * the config dir ( /var/lib/ofono ), and not it's sub-dirs, then if
> ++ * there's no trailing slash, then it's a new IMSI directory. Also
> ++ * determine if writes to gprs files cause CHANGE events in the top-level
> ++ * dir.... */
> +
> +- if (!res) {
> +- nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: an error occured while reading "
> +- "contexts for IMSI %s", imsi);
> ++ /* Add watches for the IMSI directories too */
> ++ if ((g_strrstr (imsi, "gprs") == NULL) && add_gprs_file_watch (self, imsi)) {
> ++
> ++ res = nm_ofono_read_imsi_contexts (self, imsi, &error);
> ++ if (!res)
> ++ nm_log_warn (LOGD_SETTINGS, "SCPluginOfono: an error occured while reading "
> ++ "contexts for IMSI %s", imsi);
> + }
> +- break;
> ++
> + default:
> + break;
> + }
> +@@ -411,7 +538,7 @@ out_ofono:
> + return;
> + }
> +
> +-static
> ++static void
> + SCPluginOfono_read_context_files (SCPluginOfono *self)
> + {
> + SCPluginOfonoPrivate *priv = SC_PLUGIN_OFONO_GET_PRIVATE (self);
> +@@ -422,33 +549,46 @@ SCPluginOfono_read_context_files (SCPlug
> + gboolean res = FALSE;
> + GError *error = NULL;
> +
> +- config = g_dir_open (OFONO_CONFIG_DIR, 0, NULL);
> +- while ((imsi = g_dir_read_name (config)) != NULL) {
> +-
> +- if (SCPluginOfono_should_ignore_imsi (imsi))
> +- continue;
> +-
> +- res = nm_ofono_read_imsi_contexts (self, imsi, &error);
> +-
> +- if (error && error->message)
> +- nm_log_warn (LOGD_SETTINGS, "SCPlugin-Ofono: %s", error->message);
> +- }
> +-
> + /* Hook up a GFileMonitor to watch for new context directories being created.
> + * This is in case ofono's provisioning plugin hasn't run when NM and the
> + * ofono settings plugin are started, and to pick up new created contexts.
> + */
> + config_path = g_file_new_for_path (OFONO_CONFIG_DIR);
> + if (g_file_query_exists (config_path, NULL)) {
> ++
> + monitor = g_file_monitor_directory (config_path, G_FILE_MONITOR_NONE,
> + NULL, NULL);
> +
> + if (monitor) {
> +- priv->ofono_dir_monitor_id = g_signal_connect (monitor, "changed",
> +- G_CALLBACK (ofono_dir_changed), self);
> + priv->ofono_dir_monitor = monitor;
> ++ } else {
> ++ nm_log_warn (LOGD_SETTINGS, "SCPlugin-Ofono: couldn't create dir monitor");
> ++ goto done;
> + }
> ++ } else {
> ++ nm_log_warn (LOGD_SETTINGS, "SCPlugin-Ofono: file doesn't exist: /var/lib/ofono");
> ++ goto done;
> ++ return;
> ++ }
> ++
> ++ config = g_dir_open (OFONO_CONFIG_DIR, 0, NULL);
> ++ while ((imsi = g_dir_read_name (config)) != NULL) {
> ++
> ++ if (SCPluginOfono_should_ignore_imsi (imsi))
> ++ continue;
> ++
> ++ res = nm_ofono_read_imsi_contexts (self, imsi, &error);
> ++
> ++ if (error && error->message)
> ++ nm_log_warn (LOGD_SETTINGS, "SCPlugin-Ofono: %s", error->message);
> ++
> ++ /* TODO: could go into read_imsi_contexts? */
> ++ add_gprs_file_watch(self, imsi);
> + }
> ++
> ++ priv->ofono_dir_monitor_id = g_signal_connect (monitor, "changed",
> ++ G_CALLBACK (ofono_dir_changed), self);
> ++done:
> + g_object_unref (config_path);
> + }
> +
> +Index: network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
> +===================================================================
> +--- network-manager-0.9.10.0.orig/src/devices/wwan/nm-modem-ofono.c
> ++++ network-manager-0.9.10.0/src/devices/wwan/nm-modem-ofono.c
> +@@ -88,8 +88,6 @@ ip_string_to_network_address (const gcha
> + return TRUE;
> + }
> +
> +-static void ofono_read_contexts (NMModemOfono *self);
> +-
> + static void
> + update_modem_state (NMModemOfono *self)
> + {
> +@@ -299,7 +297,6 @@ handle_subscriber_identity(NMModemOfono
> + nm_log_info (LOGD_MB, "GetPropsDone: 'SubscriberIdentity': %s", priv->imsi);
> +
> + priv->imsi = g_strdup (value_str);
> +- ofono_read_contexts (self);
> + update_modem_state (self);
> + }
> + }
> +@@ -351,55 +348,6 @@ ofono_sim_properties_changed (DBusGProxy
> + }
> +
> + static void
> +-ofono_read_imsi_contexts_done (DBusGProxy *proxy,
> +- DBusGProxyCall *call_id,
> +- gpointer user_data)
> +-{
> +- NMModemOfono *self = NM_MODEM_OFONO (user_data);
> +- NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
> +- GError *error = NULL;
> +-
> +- nm_log_dbg (LOGD_MB, "in %s", __func__);
> +-
> +- if (!dbus_g_proxy_end_call (proxy, call_id, &error, G_TYPE_INVALID)) {
> +- nm_log_warn (LOGD_MB, "failed notify settings plugin of a new context: (%d) %s",
> +- error ? error->code : -1,
> +- error && error->message ? error->message : "(unknown)");
> +- return;
> +- }
> +-}
> +-
> +-static void
> +-ofono_read_contexts (NMModemOfono *self)
> +-{
> +- NMModemOfonoPrivate *priv = NM_MODEM_OFONO_GET_PRIVATE (self);
> +- DBusGConnection *bus;
> +- DBusGProxy *settings_proxy;
> +-
> +- nm_log_dbg (LOGD_MB, "in %s", __func__);
> +-
> +- if (priv->imsi == NULL) {
> +- nm_log_warn (LOGD_MB, "No 'SubscriberIdentity', so can't read ofono GPRS contexts");
> +- return;
> +- }
> +-
> +- bus = nm_dbus_manager_get_connection (priv->dbus_mgr);
> +- settings_proxy = dbus_g_proxy_new_for_name (bus,
> +- "com.canonical.NMOfono",
> +- "/com/canonical/NMOfono",
> +- "com.canonical.NMOfono");
> +-
> +- if (settings_proxy)
> +- dbus_g_proxy_begin_call_with_timeout (settings_proxy,
> +- "ReadImsiContexts", ofono_read_imsi_contexts_done,
> +- self, NULL, 20000,
> +- G_TYPE_STRING, priv->imsi,
> +- G_TYPE_INVALID);
> +- else
> +- nm_log_warn (LOGD_MB, "could not get proxy to the oFono Settings plugin.");
> +-}
> +-
> +-static void
> + ofono_context_added (DBusGProxy *proxy,
> + const char *path,
> + GValue *prop,
> +@@ -408,8 +356,6 @@ ofono_context_added (DBusGProxy *proxy,
> + NMModemOfono *self = NM_MODEM_OFONO (user_data);
> +
> + nm_log_dbg (LOGD_MB, "context %s added", path);
> +-
> +- ofono_read_contexts (self);
> + }
> +
> + static void
--
https://code.launchpad.net/~phablet-team/network-manager/lp1361864/+merge/263855
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/network-manager/vivid-phone-overlay.
More information about the Ubuntu-reviews
mailing list