libnm: fix comparing NMSettingIPConfig for address and route properties

When comparing settings, nm_setting_compare() performs a complicated
logic, which basically serializes each GObject property to a GVariant
for the D-Bus representation.
That is wrong for example for ipv4.addresses, which don't contain
address labels. That is, the GObject property is called "addresses",
but the D-Bus field "addresses" cannot encode every information
and thus comparison fails. Instead, it would have to look into
"address-data".

Traditionally, we have virtual functions like compare_property() per
NMSetting to do the comparison. That comparison is based on the GObject
properties. I think that is wrong, because we should have a generic
concept of what a property is, independent from GObject properties.
With libnm, we added NMSettingProperty, which indeed is such an
GObject independent representation to define properties.
However, it is not used thoroughly, instead compare_property() is a hack
of special cases, overloads from NMSettingProperty, overloads of
compare_property(), and default behavior based on GParamSpec.
This should be cleaned up.

For now, just hack it by handle the properties with the problems
explicitly.

2 files changed, 191 insertions, 9 deletions

diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c
index 0b97b07482..55ad7bb0cc 100644
--- a/libnm-core/nm-setting-ip-config.c
+++ b/libnm-core/nm-setting-ip-config.c
@@ -300,17 +300,17 @@ nm_ip_address_unref (NMIPAddress *address)
 }
 
 /**
- * nm_ip_address_equal:
+ * _nm_ip_address_equal:
  * @address: the #NMIPAddress
  * @other: the #NMIPAddress to compare @address to.
+ * @consider_attributes: whether to check for equality of attributes too.
  *
- * Determines if two #NMIPAddress objects contain the same address and prefix
- * (attributes are not compared).
+ * Determines if two #NMIPAddress objects are equal.
  *
  * Returns: %TRUE if the objects contain the same values, %FALSE if they do not.
  **/
-gboolean
-nm_ip_address_equal (NMIPAddress *address, NMIPAddress *other)
+static gboolean
+_nm_ip_address_equal (NMIPAddress *address, NMIPAddress *other, gboolean consider_attributes)
 {
 	g_return_val_if_fail (address != NULL, FALSE);
 	g_return_val_if_fail (address->refcount > 0, FALSE);
@@ -322,10 +322,46 @@ nm_ip_address_equal (NMIPAddress *address, NMIPAddress *other)
 	    || address->prefix != other->prefix
 	    || strcmp (address->address, other->address) != 0)
 		return FALSE;
+	if (consider_attributes) {
+		GHashTableIter iter;
+		const char *key;
+		GVariant *value, *value2;
+		guint n;
+
+		n = address->attributes ? g_hash_table_size (address->attributes) : 0;
+		if (n != (other->attributes ? g_hash_table_size (other->attributes) : 0))
+			return FALSE;
+		if (n) {
+			g_hash_table_iter_init (&iter, address->attributes);
+			while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) {
+				value2 = g_hash_table_lookup (other->attributes, key);
+				if (!value2)
+					return FALSE;
+				if (!g_variant_equal (value, value2))
+					return FALSE;
+			}
+		}
+	}
 	return TRUE;
 }
 
 /**
+ * nm_ip_address_equal:
+ * @address: the #NMIPAddress
+ * @other: the #NMIPAddress to compare @address to.
+ *
+ * Determines if two #NMIPAddress objects contain the same address and prefix
+ * (attributes are not compared).
+ *
+ * Returns: %TRUE if the objects contain the same values, %FALSE if they do not.
+ **/
+gboolean
+nm_ip_address_equal (NMIPAddress *address, NMIPAddress *other)
+{
+	return _nm_ip_address_equal (address, other, FALSE);
+}
+
+/**
  * nm_ip_address_dup:
  * @address: the #NMIPAddress
  *
@@ -717,17 +753,18 @@ nm_ip_route_unref (NMIPRoute *route)
 }
 
 /**
- * nm_ip_route_equal:
+ * _nm_ip_route_equal:
  * @route: the #NMIPRoute
  * @other: the #NMIPRoute to compare @route to.
+ * @consider_attributes: whether to compare attributes too
  *
  * Determines if two #NMIPRoute objects contain the same destination, prefix,
- * next hop, and metric. (Attributes are not compared.)
+ * next hop, and metric.
  *
  * Returns: %TRUE if the objects contain the same values, %FALSE if they do not.
  **/
-gboolean
-nm_ip_route_equal (NMIPRoute *route, NMIPRoute *other)
+static gboolean
+_nm_ip_route_equal (NMIPRoute *route, NMIPRoute *other, gboolean consider_attributes)
 {
 	g_return_val_if_fail (route != NULL, FALSE);
 	g_return_val_if_fail (route->refcount > 0, FALSE);
@@ -740,10 +777,46 @@ nm_ip_route_equal (NMIPRoute *route, NMIPRoute *other)
 	    || strcmp (route->dest, other->dest) != 0
 	    || g_strcmp0 (route->next_hop, other->next_hop) != 0)
 		return FALSE;
+	if (consider_attributes) {
+		GHashTableIter iter;
+		const char *key;
+		GVariant *value, *value2;
+		guint n;
+
+		n = route->attributes ? g_hash_table_size (route->attributes) : 0;
+		if (n != (other->attributes ? g_hash_table_size (other->attributes) : 0))
+			return FALSE;
+		if (n) {
+			g_hash_table_iter_init (&iter, route->attributes);
+			while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &value)) {
+				value2 = g_hash_table_lookup (other->attributes, key);
+				if (!value2)
+					return FALSE;
+				if (!g_variant_equal (value, value2))
+					return FALSE;
+			}
+		}
+	}
 	return TRUE;
 }
 
 /**
+ * nm_ip_route_equal:
+ * @route: the #NMIPRoute
+ * @other: the #NMIPRoute to compare @route to.
+ *
+ * Determines if two #NMIPRoute objects contain the same destination, prefix,
+ * next hop, and metric. (Attributes are not compared.)
+ *
+ * Returns: %TRUE if the objects contain the same values, %FALSE if they do not.
+ **/
+gboolean
+nm_ip_route_equal (NMIPRoute *route, NMIPRoute *other)
+{
+	return _nm_ip_route_equal (route, other, FALSE);
+}
+
+/**
  * nm_ip_route_dup:
  * @route: the #NMIPRoute
  *
@@ -2306,6 +2379,48 @@ verify (NMSetting *setting, NMConnection *connection, GError **error)
 	return TRUE;
 }
 
+static gboolean
+compare_property (NMSetting *setting,
+                  NMSetting *other,
+                  const GParamSpec *prop_spec,
+                  NMSettingCompareFlags flags)
+{
+	NMSettingIPConfigPrivate *a_priv, *b_priv;
+	NMSettingClass *parent_class;
+	guint i;
+
+	if (nm_streq (prop_spec->name, NM_SETTING_IP_CONFIG_ADDRESSES)) {
+		a_priv = NM_SETTING_IP_CONFIG_GET_PRIVATE (setting);
+		b_priv = NM_SETTING_IP_CONFIG_GET_PRIVATE (other);
+
+		if (a_priv->addresses->len != b_priv->addresses->len)
+			return FALSE;
+		for (i = 0; i < a_priv->addresses->len; i++) {
+			if (!_nm_ip_address_equal (a_priv->addresses->pdata[i], b_priv->addresses->pdata[i], TRUE))
+				return FALSE;
+		}
+		return TRUE;
+	}
+
+	if (nm_streq (prop_spec->name, NM_SETTING_IP_CONFIG_ROUTES)) {
+		a_priv = NM_SETTING_IP_CONFIG_GET_PRIVATE (setting);
+		b_priv = NM_SETTING_IP_CONFIG_GET_PRIVATE (other);
+
+		if (a_priv->routes->len != b_priv->routes->len)
+			return FALSE;
+		for (i = 0; i < a_priv->routes->len; i++) {
+			if (!_nm_ip_route_equal (a_priv->routes->pdata[i], b_priv->routes->pdata[i], TRUE))
+				return FALSE;
+		}
+		return TRUE;
+	}
+
+	/* Otherwise chain up to parent to handle generic compare */
+	parent_class = NM_SETTING_CLASS (nm_setting_ip_config_parent_class);
+	return parent_class->compare_property (setting, other, prop_spec, flags);
+}
+
+/*****************************************************************************/
 
 static void
 nm_setting_ip_config_init (NMSettingIPConfig *setting)
@@ -2536,6 +2651,7 @@ nm_setting_ip_config_class_init (NMSettingIPConfigClass *setting_class)
 	object_class->get_property = get_property;
 	object_class->finalize     = finalize;
 	parent_class->verify       = verify;
+	parent_class->compare_property = compare_property;
 
 	/* Properties */
 
diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c
index 7ff6b8d235..9d263dcb38 100644
--- a/libnm-core/tests/test-general.c
+++ b/libnm-core/tests/test-general.c
@@ -2386,6 +2386,70 @@ test_setting_compare_id (void)
 }
 
 static void
+test_setting_compare_addresses (void)
+{
+	gs_unref_object NMSetting *s1 = NULL, *s2 = NULL;
+	gboolean success;
+	NMIPAddress *a;
+	GHashTable *result = NULL;
+
+	s1 = nm_setting_ip4_config_new ();
+	s2 = nm_setting_ip4_config_new ();
+
+	a = nm_ip_address_new (AF_INET, "192.168.7.5", 24, NULL);
+
+	nm_ip_address_set_attribute (a, "label", g_variant_new_string ("xoxoxo"));
+	nm_setting_ip_config_add_address ((NMSettingIPConfig *) s1, a);
+
+	nm_ip_address_set_attribute (a, "label", g_variant_new_string ("hello"));
+	nm_setting_ip_config_add_address ((NMSettingIPConfig *) s2, a);
+
+	nm_ip_address_unref (a);
+
+	if (nmtst_get_rand_int () % 2)
+		NMTST_SWAP (s1, s2);
+
+	success = nm_setting_compare (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT);
+	g_assert (!success);
+
+	success = nm_setting_diff (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT, FALSE, &result);
+	g_assert (!success);
+	g_clear_pointer (&result, g_hash_table_unref);
+}
+
+static void
+test_setting_compare_routes (void)
+{
+	gs_unref_object NMSetting *s1 = NULL, *s2 = NULL;
+	gboolean success;
+	NMIPRoute *r;
+	GHashTable *result = NULL;
+
+	s1 = nm_setting_ip4_config_new ();
+	s2 = nm_setting_ip4_config_new ();
+
+	r = nm_ip_route_new (AF_INET, "192.168.12.0", 24, "192.168.11.1", 473, NULL);
+
+	nm_ip_route_set_attribute (r, "label", g_variant_new_string ("xoxoxo"));
+	nm_setting_ip_config_add_route ((NMSettingIPConfig *) s1, r);
+
+	nm_ip_route_set_attribute (r, "label", g_variant_new_string ("hello"));
+	nm_setting_ip_config_add_route ((NMSettingIPConfig *) s2, r);
+
+	nm_ip_route_unref (r);
+
+	if (nmtst_get_rand_int () % 2)
+		NMTST_SWAP (s1, s2);
+
+	success = nm_setting_compare (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT);
+	g_assert (!success);
+
+	success = nm_setting_diff (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT, FALSE, &result);
+	g_assert (!success);
+	g_clear_pointer (&result, g_hash_table_unref);
+}
+
+static void
 test_setting_compare_timestamp (void)
 {
 	gs_unref_object NMSetting *old = NULL, *new = NULL;
@@ -5136,6 +5200,8 @@ int main (int argc, char **argv)
 	g_test_add_func ("/core/general/test_setting_to_dbus_transform", test_setting_to_dbus_transform);
 	g_test_add_func ("/core/general/test_setting_to_dbus_enum", test_setting_to_dbus_enum);
 	g_test_add_func ("/core/general/test_setting_compare_id", test_setting_compare_id);
+	g_test_add_func ("/core/general/test_setting_compare_addresses", test_setting_compare_addresses);
+	g_test_add_func ("/core/general/test_setting_compare_routes", test_setting_compare_routes);
 	g_test_add_func ("/core/general/test_setting_compare_timestamp", test_setting_compare_timestamp);
 #define ADD_FUNC(name, func, secret_flags, comp_flags, remove_secret) \
 	g_test_add_data_func_full ("/core/general/" G_STRINGIFY (func) "_" name, \