hostname: cache hostname-manager's hostname property

A property preferably only emits a notify-changed signal when
the value actually changes and it caches the value (so that
between property-changed signals the value is guaranteed not to change).

NMSettings and NMManager both already cache the hostname, because
NMHostnameManager didn't guarantee this basic concept.

Implement it and rely on it from NMSettings and NMPolicy.
And remove the copy of the property from NMManager.

Move the call for nm_dispatcher_call_hostname() from NMHostnameManager
to NMManager. Note that NMPolicy also has a call to the dispatcher
when set-transient-hostname returns. This should be cleaned up later.

6 files changed, 107 insertions, 131 deletions

diff --git a/src/nm-hostname-manager.c b/src/nm-hostname-manager.c
index 31132082b5..40f3351a69 100644
--- a/src/nm-hostname-manager.c
+++ b/src/nm-hostname-manager.c
@@ -37,7 +37,6 @@
 #include "nm-core-internal.h"
 
 #include "NetworkManagerUtils.h"
-#include "nm-dispatcher.h"
 
 /*****************************************************************************/
 
@@ -70,7 +69,7 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMHostnameManager,
 );
 
 typedef struct {
-	char *value;
+	char *current_hostname;
 	GFileMonitor *monitor;
 	GFileMonitor *dhcp_monitor;
 	gulong monitor_id;
@@ -189,13 +188,13 @@ hostname_is_dynamic (void)
 
 /* Returns an allocated string which the caller owns and must eventually free */
 char *
-nm_hostname_manager_get_hostname (NMHostnameManager *self)
+nm_hostname_manager_read_hostname (NMHostnameManager *self)
 {
 	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (self);
 	char *hostname = NULL;
 
 	if (priv->hostnamed_proxy) {
-		hostname = g_strdup (priv->value);
+		hostname = g_strdup (priv->current_hostname);
 		goto out;
 	}
 
@@ -216,7 +215,7 @@ nm_hostname_manager_get_hostname (NMHostnameManager *self)
 out:
 	if (hostname && !hostname[0]) {
 		g_free (hostname);
-		hostname = NULL;
+		return NULL;
 	}
 
 	return hostname;
@@ -224,6 +223,60 @@ out:
 
 /*****************************************************************************/
 
+const char *
+nm_hostname_manager_get_hostname (NMHostnameManager *self)
+{
+	g_return_val_if_fail (NM_IS_HOSTNAME_MANAGER (self), NULL);
+	return NM_HOSTNAME_MANAGER_GET_PRIVATE (self)->current_hostname;
+}
+
+static void
+_set_hostname_take (NMHostnameManager *self, char *hostname)
+{
+	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (self);
+
+	_LOGI ("hostname changed from %s%s%s to %s%s%s",
+	       NM_PRINT_FMT_QUOTED (priv->current_hostname, "\"", priv->current_hostname, "\"", "(none)"),
+	       NM_PRINT_FMT_QUOTED (hostname, "\"", hostname, "\"", "(none)"));
+
+	g_free (priv->current_hostname);
+	priv->current_hostname = hostname;
+	_notify (self, PROP_HOSTNAME);
+}
+
+static void
+_set_hostname (NMHostnameManager *self, const char *hostname)
+{
+	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (self);
+
+	hostname = nm_str_not_empty (hostname);
+	if (!nm_streq0 (hostname, priv->current_hostname))
+		_set_hostname_take (self, g_strdup (hostname));
+}
+
+static void
+_set_hostname_read (NMHostnameManager *self)
+{
+	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (self);
+	char *hostname;
+
+	if (priv->hostnamed_proxy) {
+		/* read-hostname returns the current hostname with hostnamed. */
+		return;
+	}
+
+	hostname = nm_hostname_manager_read_hostname (self);
+
+	if (nm_streq0 (hostname, priv->current_hostname)) {
+		g_free (hostname);
+		return;
+	}
+
+	_set_hostname_take (self, hostname);
+}
+
+/*****************************************************************************/
+
 typedef struct {
 	char *hostname;
 	NMHostnameManagerSetHostnameCb cb;
@@ -413,35 +466,13 @@ nm_hostname_manager_validate_hostname (const char *hostname)
 }
 
 static void
-hostname_maybe_changed (NMHostnameManager *settings)
-{
-	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (settings);
-	char *new_hostname;
-
-	new_hostname = nm_hostname_manager_get_hostname (settings);
-
-	if (   (new_hostname && !priv->value)
-	    || (!new_hostname && priv->value)
-	    || (priv->value && new_hostname && strcmp (priv->value, new_hostname))) {
-
-		_LOGI ("hostname changed from %s%s%s to %s%s%s",
-		       NM_PRINT_FMT_QUOTED (priv->value, "\"", priv->value, "\"", "(none)"),
-		       NM_PRINT_FMT_QUOTED (new_hostname, "\"", new_hostname, "\"", "(none)"));
-		g_free (priv->value);
-		priv->value = new_hostname;
-		_notify (settings, PROP_HOSTNAME);
-	} else
-		g_free (new_hostname);
-}
-
-static void
 hostname_file_changed_cb (GFileMonitor *monitor,
                           GFile *file,
                           GFile *other_file,
                           GFileMonitorEvent event_type,
                           gpointer user_data)
 {
-	hostname_maybe_changed (user_data);
+	_set_hostname_read (user_data);
 }
 
 /*****************************************************************************/
@@ -455,26 +486,13 @@ hostnamed_properties_changed (GDBusProxy *proxy,
 	NMHostnameManager *self = user_data;
 	NMHostnameManagerPrivate *priv = NM_HOSTNAME_MANAGER_GET_PRIVATE (self);
 	GVariant *v_hostname;
-	const char *hostname;
 
 	v_hostname = g_dbus_proxy_get_cached_property (priv->hostnamed_proxy,
 	                                               "StaticHostname");
-	if (!v_hostname)
-		return;
-
-	hostname = g_variant_get_string (v_hostname, NULL);
-
-	if (g_strcmp0 (priv->value, hostname) != 0) {
-		_LOGI ("hostname changed from %s%s%s to %s%s%s",
-		       NM_PRINT_FMT_QUOTED (priv->value, "\"", priv->value, "\"", "(none)"),
-		       NM_PRINT_FMT_QUOTED (hostname, "\"", hostname, "\"", "(none)"));
-		g_free (priv->value);
-		priv->value = g_strdup (hostname);
-		_notify (self, PROP_HOSTNAME);
-		nm_dispatcher_call_hostname (NULL, NULL, NULL);
+	if (v_hostname) {
+		_set_hostname (self, g_variant_get_string (v_hostname, NULL));
+		g_variant_unref (v_hostname);
 	}
-
-	g_variant_unref (v_hostname);
 }
 
 static void
@@ -487,8 +505,6 @@ setup_hostname_file_monitors (NMHostnameManager *self)
 	struct stat file_stat;
 	GFile *file;
 
-	priv->value = nm_hostname_manager_get_hostname (self);
-
 	/* resolve the path to the hostname file if it is a symbolic link */
 	if (   lstat(path, &file_stat) == 0
 	    && S_ISLNK (file_stat.st_mode)
@@ -526,7 +542,7 @@ setup_hostname_file_monitors (NMHostnameManager *self)
 	}
 #endif
 
-	hostname_maybe_changed (self);
+	_set_hostname_read (self);
 }
 
 /*****************************************************************************/
@@ -539,11 +555,7 @@ get_property (GObject *object, guint prop_id,
 
 	switch (prop_id) {
 	case PROP_HOSTNAME:
-		g_value_take_string (value, nm_hostname_manager_get_hostname (self));
-
-		/* Don't ever pass NULL through D-Bus */
-		if (!g_value_get_string (value))
-			g_value_set_static_string (value, "");
+		g_value_set_string (value, nm_hostname_manager_get_hostname (self));
 		break;
 	default:
 		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
@@ -625,7 +637,7 @@ dispose (GObject *object)
 		g_clear_object (&priv->dhcp_monitor);
 	}
 
-	g_clear_pointer (&priv->value, g_free);
+	nm_clear_g_free (&priv->current_hostname);
 
 	G_OBJECT_CLASS (nm_hostname_manager_parent_class)->dispose (object);
 }
diff --git a/src/nm-hostname-manager.h b/src/nm-hostname-manager.h
index 92702bbc1b..a837e9b92d 100644
--- a/src/nm-hostname-manager.h
+++ b/src/nm-hostname-manager.h
@@ -44,7 +44,9 @@ GType nm_hostname_manager_get_type (void);
 
 NMHostnameManager *nm_hostname_manager_get (void);
 
-char *nm_hostname_manager_get_hostname (NMHostnameManager *self);
+const char *nm_hostname_manager_get_hostname (NMHostnameManager *self);
+
+char *nm_hostname_manager_read_hostname (NMHostnameManager *self);
 
 gboolean nm_hostname_manager_write_hostname (NMHostnameManager *self, const char *hostname);
 
diff --git a/src/nm-manager.c b/src/nm-manager.c
index 0d65e58537..a845321ab7 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -35,6 +35,7 @@
 #include "devices/nm-device.h"
 #include "devices/nm-device-generic.h"
 #include "platform/nm-platform.h"
+#include "nm-hostname-manager.h"
 #include "nm-rfkill-manager.h"
 #include "dhcp/nm-dhcp-manager.h"
 #include "settings/nm-settings.h"
@@ -122,6 +123,8 @@ typedef struct {
 
 	NMPolicy *policy;
 
+	NMHostnameManager *hostname_manager;
+
 	NMBusManager  *dbus_mgr;
 	struct {
 		GDBusConnection *connection;
@@ -132,7 +135,6 @@ typedef struct {
 	NMCheckpointManager *checkpoint_mgr;
 
 	NMSettings *settings;
-	char *hostname;
 
 	RadioState radio_states[RFKILL_TYPE_MAX];
 	NMVpnManager *vpn_manager;
@@ -211,7 +213,6 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMManager,
 	PROP_ALL_DEVICES,
 
 	/* Not exported */
-	PROP_HOSTNAME,
 	PROP_SLEEPING,
 );
 
@@ -1404,38 +1405,17 @@ system_unmanaged_devices_changed_cb (NMSettings *settings,
 }
 
 static void
-system_hostname_changed_cb (NMSettings *settings,
-                            GParamSpec *pspec,
-                            gpointer user_data)
+hostname_changed_cb (NMHostnameManager *hostname_manager,
+                     GParamSpec *pspec,
+                     NMManager *self)
 {
-	NMManager *self = NM_MANAGER (user_data);
 	NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
-	char *hostname;
-
-	g_object_get (priv->settings,
-	              NM_SETTINGS_HOSTNAME,
-	              &hostname,
-	              NULL);
-
-	/* nm_settings_get_hostname() does not return an empty hostname. */
-	nm_assert (!hostname || *hostname);
-
-	if (!hostname && !priv->hostname)
-		return;
-	if (hostname && priv->hostname && !strcmp (hostname, priv->hostname)) {
-		g_free (hostname);
-		return;
-	}
-
-	/* realloc, to free possibly trailing data after NUL. */
-	if (hostname)
-		hostname = g_realloc (hostname, strlen (hostname) + 1);
+	const char *hostname;
 
-	g_free (priv->hostname);
-	priv->hostname = hostname;
-	_notify (self, PROP_HOSTNAME);
+	hostname = nm_hostname_manager_get_hostname (priv->hostname_manager);
 
-	nm_dhcp_manager_set_default_hostname (nm_dhcp_manager_get (), priv->hostname);
+	nm_dispatcher_call_hostname (NULL, NULL, NULL);
+	nm_dhcp_manager_set_default_hostname (nm_dhcp_manager_get (), hostname);
 }
 
 /*****************************************************************************/
@@ -5090,7 +5070,7 @@ nm_manager_start (NMManager *self, GError **error)
 	       priv->net_enabled ? "enabled" : "disabled");
 
 	system_unmanaged_devices_changed_cb (priv->settings, NULL, self);
-	system_hostname_changed_cb (priv->settings, NULL, self);
+	hostname_changed_cb (priv->hostname_manager, NULL, self);
 
 	/* Start device factories */
 	nm_device_factory_manager_load_factories (_register_device_factory, self);
@@ -5986,12 +5966,15 @@ constructed (GObject *object)
 	                  G_CALLBACK (settings_startup_complete_changed), self);
 	g_signal_connect (priv->settings, "notify::" NM_SETTINGS_UNMANAGED_SPECS,
 	                  G_CALLBACK (system_unmanaged_devices_changed_cb), self);
-	g_signal_connect (priv->settings, "notify::" NM_SETTINGS_HOSTNAME,
-	                  G_CALLBACK (system_hostname_changed_cb), self);
 	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_ADDED,
 	                  G_CALLBACK (connection_added_cb), self);
 	g_signal_connect (priv->settings, NM_SETTINGS_SIGNAL_CONNECTION_UPDATED,
 	                  G_CALLBACK (connection_updated_cb), self);
+
+	priv->hostname_manager = g_object_ref (nm_hostname_manager_get ());
+	g_signal_connect (priv->hostname_manager, "notify::" NM_HOSTNAME_MANAGER_HOSTNAME,
+	                  G_CALLBACK (hostname_changed_cb), self);
+
 	/*
 	 * Do not delete existing virtual devices to keep connectivity up.
 	 * Virtual devices are reused when NetworkManager is restarted.
@@ -6188,9 +6171,6 @@ get_property (GObject *object, guint prop_id,
 	case PROP_ACTIVATING_CONNECTION:
 		nm_utils_g_value_set_object_path (value, priv->activating_connection);
 		break;
-	case PROP_HOSTNAME:
-		g_value_set_string (value, priv->hostname);
-		break;
 	case PROP_SLEEPING:
 		g_value_set_boolean (value, priv->sleeping);
 		break;
@@ -6299,8 +6279,6 @@ dispose (GObject *object)
 		g_clear_object (&priv->config);
 	}
 
-	g_free (priv->hostname);
-
 	if (priv->policy) {
 		g_signal_handlers_disconnect_by_func (priv->policy, policy_default_device_changed, manager);
 		g_signal_handlers_disconnect_by_func (priv->policy, policy_activating_device_changed, manager);
@@ -6310,12 +6288,16 @@ dispose (GObject *object)
 	if (priv->settings) {
 		g_signal_handlers_disconnect_by_func (priv->settings, settings_startup_complete_changed, manager);
 		g_signal_handlers_disconnect_by_func (priv->settings, system_unmanaged_devices_changed_cb, manager);
-		g_signal_handlers_disconnect_by_func (priv->settings, system_hostname_changed_cb, manager);
 		g_signal_handlers_disconnect_by_func (priv->settings, connection_added_cb, manager);
 		g_signal_handlers_disconnect_by_func (priv->settings, connection_updated_cb, manager);
 		g_clear_object (&priv->settings);
 	}
 
+	if (priv->hostname_manager) {
+		g_signal_handlers_disconnect_by_func (priv->hostname_manager, hostname_changed_cb, manager);
+		g_clear_object (&priv->hostname_manager);
+	}
+
 	g_clear_object (&priv->vpn_manager);
 
 	/* Unregister property filter */
@@ -6477,13 +6459,6 @@ nm_manager_class_init (NMManagerClass *manager_class)
 	                         G_PARAM_READABLE |
 	                         G_PARAM_STATIC_STRINGS);
 
-	/* Hostname is not exported over D-Bus */
-	obj_properties[PROP_HOSTNAME] =
-	    g_param_spec_string (NM_MANAGER_HOSTNAME, "", "",
-	                         NULL,
-	                         G_PARAM_READABLE |
-	                         G_PARAM_STATIC_STRINGS);
-
 	/* Sleeping is not exported over D-Bus */
 	obj_properties[PROP_SLEEPING] =
 	    g_param_spec_boolean (NM_MANAGER_SLEEPING, "", "",
diff --git a/src/nm-manager.h b/src/nm-manager.h
index 676fa995b6..fbbaffc314 100644
--- a/src/nm-manager.h
+++ b/src/nm-manager.h
@@ -54,7 +54,6 @@
 #define NM_MANAGER_ALL_DEVICES "all-devices"
 
 /* Not exported */
-#define NM_MANAGER_HOSTNAME "hostname"
 #define NM_MANAGER_SLEEPING "sleeping"
 
 /* signals */
diff --git a/src/nm-policy.c b/src/nm-policy.c
index 24b3cba8c4..36c05e1afa 100644
--- a/src/nm-policy.c
+++ b/src/nm-policy.c
@@ -587,7 +587,7 @@ static void
 update_system_hostname (NMPolicy *self, NMDevice *best4, NMDevice *best6, const char *msg)
 {
 	NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self);
-	char *configured_hostname = NULL;
+	const char *configured_hostname;
 	gs_free char *temp_hostname = NULL;
 	const char *dhcp_hostname, *p;
 	NMIP4Config *ip4_config;
@@ -638,14 +638,12 @@ update_system_hostname (NMPolicy *self, NMDevice *best4, NMDevice *best6, const
 	 */
 
 	/* Try a persistent hostname first */
-	g_object_get (G_OBJECT (priv->manager), NM_MANAGER_HOSTNAME, &configured_hostname, NULL);
+	configured_hostname = nm_hostname_manager_get_hostname (priv->hostname_manager);
 	if (configured_hostname && nm_utils_is_specific_hostname (configured_hostname)) {
 		_set_hostname (self, configured_hostname, "from system configuration");
 		priv->dhcp_hostname = FALSE;
-		g_free (configured_hostname);
 		return;
 	}
-	g_free (configured_hostname);
 
 	/* Try automatically determined hostname from the best device's IP config */
 	if (!best4)
@@ -1251,7 +1249,7 @@ process_secondaries (NMPolicy *self,
 }
 
 static void
-hostname_changed (NMManager *manager, GParamSpec *pspec, gpointer user_data)
+hostname_changed (NMHostnameManager *hostname_manager, GParamSpec *pspec, gpointer user_data)
 {
 	NMPolicyPrivate *priv = user_data;
 	NMPolicy *self = _PRIV_TO_SELF (priv);
@@ -2350,7 +2348,8 @@ constructed (GObject *object)
 
 	priv->resolver = g_resolver_get_default ();
 
-	g_signal_connect (priv->manager, "notify::" NM_MANAGER_HOSTNAME,           (GCallback) hostname_changed, priv);
+	g_signal_connect (priv->hostname_manager, "notify::" NM_HOSTNAME_MANAGER_HOSTNAME, (GCallback) hostname_changed, priv);
+
 	g_signal_connect (priv->manager, "notify::" NM_MANAGER_SLEEPING,           (GCallback) sleeping_changed, priv);
 	g_signal_connect (priv->manager, "notify::" NM_MANAGER_NETWORKING_ENABLED, (GCallback) sleeping_changed, priv);
 	g_signal_connect (priv->manager, NM_MANAGER_INTERNAL_DEVICE_ADDED,         (GCallback) device_added, priv);
@@ -2429,6 +2428,11 @@ dispose (GObject *object)
 	g_clear_pointer (&priv->cur_hostname, g_free);
 	g_clear_pointer (&priv->last_hostname, g_free);
 
+	if (priv->hostname_manager) {
+		g_signal_handlers_disconnect_by_data (priv->hostname_manager, priv);
+		g_clear_object (&priv->hostname_manager);
+	}
+
 	if (priv->settings) {
 		g_signal_handlers_disconnect_by_data (priv->settings, priv);
 		g_clear_object (&priv->settings);
diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c
index 2e41f9c3a4..8db58c8d3a 100644
--- a/src/settings/nm-settings.c
+++ b/src/settings/nm-settings.c
@@ -156,7 +156,6 @@ typedef struct {
 	gboolean started;
 	gboolean startup_complete;
 
-	char *hostname;
 	NMHostnameManager *hostname_manager;
 
 } NMSettingsPrivate;
@@ -1795,26 +1794,11 @@ nm_settings_get_startup_complete (NMSettings *self)
 /*****************************************************************************/
 
 static void
-_hostname_changed (NMSettings *self)
-{
-	NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self);
-	gs_free char *hostname = NULL;
-
-	hostname = nm_hostname_manager_get_hostname (priv->hostname_manager);
-
-	if (nm_streq0 (hostname, priv->hostname))
-		return;
-	g_free (priv->hostname);
-	priv->hostname = g_steal_pointer (&hostname);
-	_notify (self, PROP_HOSTNAME);
-}
-
-static void
 _hostname_changed_cb (NMHostnameManager *hostname_manager,
                       GParamSpec *pspec,
                       gpointer user_data)
 {
-	_hostname_changed (NM_SETTINGS (user_data));
+	_notify (user_data, PROP_HOSTNAME);
 }
 
 /*****************************************************************************/
@@ -1843,7 +1827,9 @@ nm_settings_start (NMSettings *self, GError **error)
 	                  "notify::"NM_HOSTNAME_MANAGER_HOSTNAME,
 	                  G_CALLBACK (_hostname_changed_cb),
 	                  self);
-	_hostname_changed (self);
+	if (nm_hostname_manager_get_hostname (priv->hostname_manager))
+		_notify (self, PROP_HOSTNAME);
+
 	return TRUE;
 }
 
@@ -1870,10 +1856,10 @@ get_property (GObject *object, guint prop_id,
 		g_value_take_boxed (value, (char **) g_ptr_array_free (array, FALSE));
 		break;
 	case PROP_HOSTNAME:
-		if (priv->hostname)
-			g_value_set_string (value, priv->hostname);
-		else
-			g_value_set_static_string (value, "");
+		g_value_set_string (value,
+		                    priv->hostname_manager
+		                      ? nm_hostname_manager_get_hostname (priv->hostname_manager)
+		                      : NULL);
 		break;
 	case PROP_CAN_MODIFY:
 		g_value_set_boolean (value, !!get_plugin (self, NM_SETTINGS_PLUGIN_CAP_MODIFY_CONNECTIONS));
@@ -1957,8 +1943,6 @@ finalize (GObject *object)
 
 	g_slist_free_full (priv->plugins, g_object_unref);
 
-	g_free (priv->hostname);
-
 	g_clear_object (&priv->config);
 
 	G_OBJECT_CLASS (nm_settings_parent_class)->finalize (object);