[Merge] lp:~phablet-team/network-manager/lp1361864 into lp:~phablet-team/network-manager/vivid-phone-overlay
Alfonso Sanchez-Beato
alfonso.sanchez-beato at canonical.com
Mon Jul 6 08:00:12 UTC 2015
Review: Needs Fixing
See inline comments. Overall, looks good.
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);
This does not make much sense, as uuids will always be NULL at this point.
> +
> + list = contexts;
> + while (list) {
> + GHashTable *context = (GHashTable *) list->data;
A for loop would save the need for the "next" label
> +- 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;
Formatting error here
> ++ }
> +
> +- /* 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 you are checking for an allocation error, an else which prints an error would be good (nm_ofono_connection_new does not seem to do that)
> ++ 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;
Again, a for loop would be cleaner here
> ++ 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]);
The string array returned here is not being freed. Besides that, I do not see the need for the additional check for the IMSI, as the uuid is created using IMSI + context ID. And it is supposed to be, well, unique.
> ++
> ++ 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")) {
I think this comment is not right because of the break statement in line 272 of the file. Seems like the first "Preferred" wins. Which is fine nonetheless.
> ++ 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
Typo here
> ++ * 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;
Formatting
> +
> + 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