fdo-mirrors/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager.git synced 2026-02-04 23:30:28 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

libnm: fix comparing NMSettingIPConfig for address and route properties

Browse source 
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.
This commit is contained in:
Thomas Haller 2016-06-11 00:31:26 +02:00
parent c9ab22f41d
commit 807f846610
2 changed files with 197 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

146
libnm-core/nm-setting-ip-config.c
View file

@ -299,6 +299,52 @@ nm_ip_address_unref (NMIPAddress *address)
}
}
/**
* _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 are equal.
*
* Returns: %TRUE if the objects contain the same values, %FALSE if they do not.
**/
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);
g_return_val_if_fail (other != NULL, FALSE);
g_return_val_if_fail (other->refcount > 0, FALSE);
if ( address->family != other->family
|| 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
@ -312,17 +358,7 @@ nm_ip_address_unref (NMIPAddress *address)
gboolean
nm_ip_address_equal (NMIPAddress *address, NMIPAddress *other)
{
g_return_val_if_fail (address != NULL, FALSE);
g_return_val_if_fail (address->refcount > 0, FALSE);
g_return_val_if_fail (other != NULL, FALSE);
g_return_val_if_fail (other->refcount > 0, FALSE);
if ( address->family != other->family
|| address->prefix != other->prefix
|| strcmp (address->address, other->address) != 0)
return FALSE;
return TRUE;
return _nm_ip_address_equal (address, other, FALSE);
}
/**
@ -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,9 +777,45 @@ 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 */

66
libnm-core/tests/test-general.c
View file

@ -2385,6 +2385,70 @@ test_setting_compare_id (void)
g_assert (success);
}
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)
{
@ -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, \