libnm: remove unused "nm-property-compare.c"

"nm-property-compare.c" only contains nm_property_compare(), which is
broken.

It tries to compare string dictionaries as equal regardless of the
order of elements. It gets it wrong, for dictionaries with duplicate
keys. Which means, it can only be used with trusted variants that are
known to not contain duplicates. Which is quite a non-starter.

Also, the idea of a compare function for GVariant that ignores the order
of dictionary elements seems wrong. Even if for a certain application
the order does not matter, it still depends what the upper layer makes
of duplicate keys (will they bail out, or take the first/last occurrence
of a duplicate key?). nm_property_compare() doesn't have the knowledge
how upper layer handles it, and it's not obvious what's the right
choice. For example, if you use g_variant_lookup(), the first occurrence
is preferred. If you iterate over the children, possibly later
occurrences overwrite earlier ones.

It's ill defined, and maybe shouldn't be done. What should instead
happen, is that upper layers normalize (sort, uniquify) the keys, so
that we can do a full comparison. For that we have nm_g_variant_cmp().

Drop the now unused code. The core of the function still exists as
nm_g_variant_cmp().

.gitignore

@@ -141,7 +141,6 @@ test-*.trs
 /src/libnm-core-public/nm-version-macros.h
 /src/libnm-core-public/nm-dbus-types.xml
 /src/libnm-core-public/nm-vpn-dbus-types.xml
-/src/libnm-core-impl/tests/test-compare
 /src/libnm-core-impl/tests/test-crypto
 /src/libnm-core-impl/tests/test-settings-defaults
 /src/libnm-core-impl/tests/test-general
@@ -439,6 +438,7 @@ test-*.trs
 /src/initrd/tests/test-dt-reader
 /src/initrd/tests/test-ibft-reader
 /src/libnm-client-impl/nm-settings-docs-gir.xml
+/src/libnm-core-impl/tests/test-compare
 /src/ndisc/tests/test-ndisc-fake
 /src/ndisc/tests/test-ndisc-linux
 /src/nm-daemon-helper/nm-daemon-helper

Makefile.am

@@ -1320,7 +1320,6 @@ src_libnm_core_public_mkenums_h = \
 src_libnm_core_impl_lib_h_priv = \
 	src/libnm-core-impl/nm-connection-private.h \
 	src/libnm-core-impl/nm-default-libnm-core.h \
-	src/libnm-core-impl/nm-property-compare.h \
 	src/libnm-core-impl/nm-setting-private.h \
 	src/libnm-core-impl/nm-team-utils.h \
 	src/libnm-core-impl/nm-utils-private.h \
@@ -1396,7 +1395,6 @@ src_libnm_core_impl_lib_c_real = \
 	src/libnm-core-impl/nm-keyfile-utils.c \
 	src/libnm-core-impl/nm-keyfile.c \
 	src/libnm-core-impl/nm-meta-setting-base-impl.c \
-	src/libnm-core-impl/nm-property-compare.c \
 	src/libnm-core-impl/nm-setting.c \
 	src/libnm-core-impl/nm-simple-connection.c \
 	src/libnm-core-impl/nm-team-utils.c \
@@ -1602,7 +1600,6 @@ EXTRA_DIST += \
 ###############################################################################
 
 check_programs += \
-	src/libnm-core-impl/tests/test-compare \
 	src/libnm-core-impl/tests/test-crypto \
 	src/libnm-core-impl/tests/test-general \
 	src/libnm-core-impl/tests/test-keyfile \
@@ -1631,7 +1628,6 @@ src_libnm_core_impl_tests_cppflags = \
 	$(SANITIZER_EXEC_CFLAGS) \
 	$(NULL)
 
-src_libnm_core_impl_tests_test_compare_CPPFLAGS = $(src_libnm_core_impl_tests_cppflags)
 src_libnm_core_impl_tests_test_crypto_CPPFLAGS = $(src_libnm_core_impl_tests_cppflags)
 src_libnm_core_impl_tests_test_general_CPPFLAGS = $(src_libnm_core_impl_tests_cppflags)
 src_libnm_core_impl_tests_test_keyfile_CPPFLAGS = $(src_libnm_core_impl_tests_cppflags)
@@ -1668,7 +1664,6 @@ src_libnm_core_impl_tests_ldflags = \
 	$(SANITIZER_EXEC_LDFLAGS) \
 	$(NULL)
 
-src_libnm_core_impl_tests_test_compare_LDADD = $(src_libnm_core_impl_tests_ldadd)
 src_libnm_core_impl_tests_test_crypto_LDADD = $(src_libnm_core_impl_tests_ldadd)
 src_libnm_core_impl_tests_test_general_LDADD = $(src_libnm_core_impl_tests_ldadd)
 src_libnm_core_impl_tests_test_keyfile_LDADD = $(src_libnm_core_impl_tests_ldadd)
@@ -1676,7 +1671,6 @@ src_libnm_core_impl_tests_test_secrets_LDADD = $(src_libnm_core_impl_tests_ldadd
 src_libnm_core_impl_tests_test_setting_LDADD = $(src_libnm_core_impl_tests_ldadd)
 src_libnm_core_impl_tests_test_settings_defaults_LDADD = $(src_libnm_core_impl_tests_ldadd)
 
-src_libnm_core_impl_tests_test_compare_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
 src_libnm_core_impl_tests_test_crypto_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
 src_libnm_core_impl_tests_test_general_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
 src_libnm_core_impl_tests_test_keyfile_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
@@ -1684,7 +1678,6 @@ src_libnm_core_impl_tests_test_secrets_LDFLAGS = $(src_libnm_core_impl_tests_ldf
 src_libnm_core_impl_tests_test_setting_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
 src_libnm_core_impl_tests_test_settings_defaults_LDFLAGS = $(src_libnm_core_impl_tests_ldflags)
 
-$(src_libnm_core_impl_tests_test_compare_OBJECTS): $(src_libnm_core_public_mkenums_h)
 $(src_libnm_core_impl_tests_test_crypto_OBJECTS): $(src_libnm_core_public_mkenums_h)
 $(src_libnm_core_impl_tests_test_general_OBJECTS): $(src_libnm_core_public_mkenums_h)
 $(src_libnm_core_impl_tests_test_keyfile_OBJECTS): $(src_libnm_core_public_mkenums_h)

docs/libnm/Makefile.am

@@ -52,7 +52,6 @@ IGNORE_HFILES= \
 	\
 	nm-connection-private.h \
 	nm-default-libnm-core.h \
-	nm-property-compare.h \
 	nm-setting-private.h \
 	nm-team-utils.h \
 	nm-utils-private.h \

docs/libnm/meson.build

@@ -15,7 +15,6 @@ private_headers = [
 
   'nm-connection-private.h',
   'nm-default-libnm-core.h',
-  'nm-property-compare.h',
   'nm-setting-private.h',
   'nm-team-utils.h',
   'nm-utils-private.h',

src/libnm-core-impl/meson.build

@@ -68,7 +68,6 @@ libnm_core_impl_sources = files(
   'nm-keyfile-utils.c',
   'nm-keyfile.c',
   'nm-meta-setting-base-impl.c',
-  'nm-property-compare.c',
   'nm-setting.c',
   'nm-simple-connection.c',
   'nm-team-utils.c',

src/libnm-core-impl/nm-property-compare.c

@@ -1,136 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2007 - 2014 Red Hat, Inc.
- * Copyright (C) 2007 - 2008 Novell, Inc.
- */
-
-#include "libnm-core-impl/nm-default-libnm-core.h"
-
-#include "nm-property-compare.h"
-
-#include <netinet/in.h>
-
-static int
-_nm_property_compare_collection(GVariant *value1, GVariant *value2)
-{
-    GVariant *child1, *child2;
-    int       i, len1, len2;
-    int       ret;
-
-    len1 = g_variant_n_children(value1);
-    len2 = g_variant_n_children(value2);
-
-    if (len1 != len2)
-        return len1 < len2 ? -1 : len1 > len2;
-
-    for (i = 0; i < len1; i++) {
-        child1 = g_variant_get_child_value(value1, i);
-        child2 = g_variant_get_child_value(value2, i);
-
-        ret = nm_property_compare(child1, child2);
-        g_variant_unref(child1);
-        g_variant_unref(child2);
-
-        if (ret)
-            return ret;
-    }
-
-    return 0;
-}
-
-static int
-_nm_property_compare_vardict(GVariant *value1, GVariant *value2)
-{
-    GVariantIter iter;
-    int          len1, len2;
-    const char  *key;
-    GVariant    *val1, *val2;
-
-    len1 = g_variant_n_children(value1);
-    len2 = g_variant_n_children(value2);
-
-    if (len1 != len2)
-        return len1 < len2 ? -1 : 1;
-
-    g_variant_iter_init(&iter, value1);
-    while (g_variant_iter_next(&iter, "{&sv}", &key, &val1)) {
-        if (!g_variant_lookup(value2, key, "v", &val2)) {
-            g_variant_unref(val1);
-            return -1;
-        }
-        if (!g_variant_equal(val1, val2)) {
-            g_variant_unref(val1);
-            g_variant_unref(val2);
-            return -1;
-        }
-        g_variant_unref(val1);
-        g_variant_unref(val2);
-    }
-
-    return 0;
-}
-
-static int
-_nm_property_compare_strdict(GVariant *value1, GVariant *value2)
-{
-    GVariantIter iter;
-    int          len1, len2;
-    const char  *key, *val1, *val2;
-    int          ret;
-
-    len1 = g_variant_n_children(value1);
-    len2 = g_variant_n_children(value2);
-
-    if (len1 != len2)
-        return len1 < len2 ? -1 : len1 > len2;
-
-    g_variant_iter_init(&iter, value1);
-    while (g_variant_iter_next(&iter, "{&s&s}", &key, &val1)) {
-        if (!g_variant_lookup(value2, key, "&s", &val2))
-            return -1;
-
-        ret = strcmp(val1, val2);
-        if (ret)
-            return ret;
-    }
-
-    return 0;
-}
-
-int
-nm_property_compare(GVariant *value1, GVariant *value2)
-{
-    const GVariantType *type1;
-    const GVariantType *type2;
-    int                 ret;
-
-    if (value1 == value2)
-        return 0;
-    if (!value1)
-        return 1;
-    if (!value2)
-        return -1;
-
-    type1 = g_variant_get_type(value1);
-    type2 = g_variant_get_type(value2);
-
-    if (!g_variant_type_equal(type1, type2))
-        return type1 < type2 ? -1 : type1 > type2;
-
-    if (g_variant_type_is_basic(type1))
-        ret = g_variant_compare(value1, value2);
-    else if (g_variant_is_of_type(value1, G_VARIANT_TYPE("a{ss}")))
-        ret = _nm_property_compare_strdict(value1, value2);
-    else if (g_variant_is_of_type(value1, G_VARIANT_TYPE("a{sv}")))
-        ret = _nm_property_compare_vardict(value1, value2);
-    else if (g_variant_type_is_array(type1))
-        ret = _nm_property_compare_collection(value1, value2);
-    else if (g_variant_type_is_tuple(type1))
-        ret = _nm_property_compare_collection(value1, value2);
-    else {
-        g_warning("Don't know how to compare variant type '%s'", (const char *) type1);
-        ret = value1 == value2;
-    }
-
-    return ret;
-}

src/libnm-core-impl/nm-property-compare.h

@@ -1,16 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2007 - 2014 Red Hat, Inc.
- * Copyright (C) 2007 - 2008 Novell, Inc.
- */
-
-#ifndef __NM_PROPERTY_COMPARE_H__
-#define __NM_PROPERTY_COMPARE_H__
-
-#if !((NETWORKMANAGER_COMPILATION) & NM_NETWORKMANAGER_COMPILATION_WITH_LIBNM_CORE_PRIVATE)
-#error Cannot use this header.
-#endif
-
-int nm_property_compare(GVariant *value1, GVariant *value2);
-
-#endif /* __NM_PROPERTY_COMPARE_H__ */

src/libnm-core-impl/tests/meson.build

@@ -8,7 +8,6 @@ enum_sources = gnome.mkenums_simple(
 )
 
 test_units = [
-  'test-compare',
   'test-crypto',
   'test-general',
   'test-keyfile',

src/libnm-core-impl/tests/test-compare.c

@@ -1,231 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2007 - 2014 Red Hat, Inc.
- * Copyright (C) 2007 - 2008 Novell, Inc.
- */
-
-#include "libnm-core-impl/nm-default-libnm-core.h"
-
-#include <arpa/inet.h>
-#include <netinet/in.h>
-
-#include "nm-property-compare.h"
-
-#include "libnm-glib-aux/nm-test-utils.h"
-
-static void
-compare_ints(void)
-{
-    GVariant *value1, *value2;
-
-    value1 = g_variant_new_int32(5);
-    value2 = g_variant_new_int32(5);
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_int32(10);
-    g_assert(nm_property_compare(value1, value2) < 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_int32(-1);
-    g_assert(nm_property_compare(value1, value2) > 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-static void
-compare_strings(void)
-{
-    GVariant   *value1, *value2;
-    const char *str1 = "hello";
-    const char *str2 = "world";
-
-    value1 = g_variant_new_string(str1);
-    value2 = g_variant_new_string(str1);
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_string(str2);
-    g_assert(nm_property_compare(value1, value2) < 0);
-
-    g_assert(nm_property_compare(value2, value1) > 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-static void
-compare_strv(void)
-{
-    GVariant         *value1, *value2;
-    const char *const strv1[] = {"foo", "bar", "baz", NULL};
-    const char *const strv2[] = {"foo", "bar", "bar", NULL};
-    const char *const strv3[] = {"foo", "bar", NULL};
-    const char *const strv4[] = {"foo", "bar", "baz", "bam", NULL};
-
-    value1 = g_variant_new_strv(strv1, -1);
-    value2 = g_variant_new_strv(strv1, -1);
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_strv(strv2, -1);
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_strv(strv3, -1);
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_strv(strv4, -1);
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-static void
-compare_arrays(void)
-{
-    GVariant *value1, *value2;
-    guint32   array[] = {0, 1, 2, 3, 4};
-
-    value1 = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
-                                       array,
-                                       G_N_ELEMENTS(array),
-                                       sizeof(guint32));
-    value2 = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
-                                       array,
-                                       G_N_ELEMENTS(array),
-                                       sizeof(guint32));
-
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
-                                       array + 1,
-                                       G_N_ELEMENTS(array) - 1,
-                                       sizeof(guint32));
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    array[0] = 7;
-    g_variant_unref(value2);
-    value2 = g_variant_new_fixed_array(G_VARIANT_TYPE_UINT32,
-                                       array,
-                                       G_N_ELEMENTS(array),
-                                       sizeof(guint32));
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-static void
-compare_str_hash(void)
-{
-    GVariant       *value1, *value2;
-    GVariantBuilder builder;
-
-    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}"));
-    g_variant_builder_add(&builder, "{ss}", "key1", "hello");
-    g_variant_builder_add(&builder, "{ss}", "key2", "world");
-    g_variant_builder_add(&builder, "{ss}", "key3", "!");
-    value1 = g_variant_builder_end(&builder);
-
-    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}"));
-    g_variant_builder_add(&builder, "{ss}", "key3", "!");
-    g_variant_builder_add(&builder, "{ss}", "key2", "world");
-    g_variant_builder_add(&builder, "{ss}", "key1", "hello");
-    value2 = g_variant_builder_end(&builder);
-
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}"));
-    g_variant_builder_add(&builder, "{ss}", "key1", "hello");
-    g_variant_builder_add(&builder, "{ss}", "key3", "!");
-    value2 = g_variant_builder_end(&builder);
-
-    g_assert(nm_property_compare(value1, value2) != 0);
-    g_assert(nm_property_compare(value2, value1) != 0);
-
-    g_variant_unref(value2);
-    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{ss}"));
-    g_variant_builder_add(&builder, "{ss}", "key1", "hello");
-    g_variant_builder_add(&builder, "{ss}", "key2", "moon");
-    g_variant_builder_add(&builder, "{ss}", "key3", "!");
-    value2 = g_variant_builder_end(&builder);
-
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-static void
-compare_ip6_addresses(void)
-{
-    GVariant       *value1, *value2;
-    struct in6_addr addr1;
-    struct in6_addr addr2;
-    struct in6_addr addr3;
-    guint32         prefix1 = 64;
-    guint32         prefix2 = 64;
-    guint32         prefix3 = 0;
-
-    inet_pton(AF_INET6, "1:2:3:4:5:6:7:8", &addr1);
-    inet_pton(AF_INET6, "ffff:2:3:4:5:6:7:8", &addr2);
-    inet_pton(AF_INET6, "::", &addr3);
-
-    value1 = g_variant_new(
-        "(@ayu@ay)",
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr1.s6_addr, 16, 1),
-        prefix1,
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr3.s6_addr, 16, 1));
-
-    value2 = g_variant_new(
-        "(@ayu@ay)",
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr1.s6_addr, 16, 1),
-        prefix1,
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr3.s6_addr, 16, 1));
-
-    g_assert(nm_property_compare(value1, value2) == 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new(
-        "(@ayu@ay)",
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr2.s6_addr, 16, 1),
-        prefix2,
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr3.s6_addr, 16, 1));
-
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value2);
-    value2 = g_variant_new(
-        "(@ayu@ay)",
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr3.s6_addr, 16, 1),
-        prefix3,
-        g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE, (guint8 *) addr3.s6_addr, 16, 1));
-
-    g_assert(nm_property_compare(value1, value2) != 0);
-
-    g_variant_unref(value1);
-    g_variant_unref(value2);
-}
-
-NMTST_DEFINE();
-
-int
-main(int argc, char *argv[])
-{
-    nmtst_init(&argc, &argv, TRUE);
-
-    g_test_add_func("/libnm/compare/ints", compare_ints);
-    g_test_add_func("/libnm/compare/strings", compare_strings);
-    g_test_add_func("/libnm/compare/strv", compare_strv);
-    g_test_add_func("/libnm/compare/arrays", compare_arrays);
-    g_test_add_func("/libnm/compare/str_hash", compare_str_hash);
-    g_test_add_func("/libnm/compare/ip6_addresses", compare_ip6_addresses);
-
-    return g_test_run();
-}