From ee34eeafb9c4379828e3ac6c5b279ca9426bd99e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Nov 2022 12:45:37 +0100 Subject: [PATCH 1/7] platform: fix nmp_object_copy(id_only) for object that don't implement cmd_plobj_id_copy() The if-else-if was wrong. It meant that if an object did not implement cmd_plobj_id_copy(), nothign was copied (for id-only). I think this code path was not actually hit, because we never clone an object only by ID. Fixes: c91a4617a102 ('nmp-object: allow missing implementations for certain virtual functions') --- src/libnm-platform/nmp-object.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 46d6c6e758..19478cddb7 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -1317,10 +1317,9 @@ nmp_object_copy(NMPObject *dst, const NMPObject *src, gboolean id_only) g_return_if_fail(klass == NMP_OBJECT_GET_CLASS(src)); - if (id_only) { - if (klass->cmd_plobj_id_copy) - klass->cmd_plobj_id_copy(&dst->object, &src->object); - } else if (klass->cmd_obj_copy) + if (id_only && klass->cmd_plobj_id_copy) + klass->cmd_plobj_id_copy(&dst->object, &src->object); + else if (klass->cmd_obj_copy) klass->cmd_obj_copy(dst, src); else memcpy(&dst->object, &src->object, klass->sizeof_data); From 8feeb199ad4b8e311e29b658d6a8ce772833fd88 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Nov 2022 12:06:27 +0100 Subject: [PATCH 2/7] platform: drop redundant hook implementations from NMPObject classes A NMPClass that has data outside the plobj part, needs to implement the cmd_obj_*() hooks, instead of cmd_plobj_*(). For those objects, reasoning only about the plobj part is not sufficient. Implementing both hooks is also unnecessary and confusing. Ensure that if we have cmd_obj_*() hooks set, that the corresponding cmd_plobj_*() hooks are unset. --- src/libnm-platform/nmp-object.c | 117 ++++++++++++++++---------------- src/libnm-platform/nmp-object.h | 13 ++-- 2 files changed, 64 insertions(+), 66 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 19478cddb7..a8da9d6e8e 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -907,30 +907,33 @@ nmp_object_to_string(const NMPObject *obj, klass = NMP_OBJECT_GET_CLASS(obj); - if (klass->cmd_obj_to_string) + if (klass->cmd_obj_to_string) { + nm_assert(!klass->cmd_plobj_to_string); + nm_assert(!klass->cmd_plobj_to_string_id); return klass->cmd_obj_to_string(obj, to_string_mode, buf, buf_size); + } + + nm_assert(klass->cmd_plobj_to_string); switch (to_string_mode) { case NMP_OBJECT_TO_STRING_ID: - if (!klass->cmd_plobj_to_string_id) { - g_snprintf(buf, buf_size, NM_HASH_OBFUSCATE_PTR_FMT, NM_HASH_OBFUSCATE_PTR(obj)); - return buf; - } - return klass->cmd_plobj_to_string_id(&obj->object, buf, buf_size); + if (!klass->cmd_plobj_to_string_id) + return klass->cmd_plobj_to_string_id(&obj->object, buf, buf_size); + g_snprintf(buf, buf_size, NM_HASH_OBFUSCATE_PTR_FMT, NM_HASH_OBFUSCATE_PTR(obj)); + return buf; case NMP_OBJECT_TO_STRING_ALL: - g_snprintf( - buf, - buf_size, - "[%s," NM_HASH_OBFUSCATE_PTR_FMT ",%u,%calive,%cvisible; %s]", - klass->obj_type_name, - NM_HASH_OBFUSCATE_PTR(obj), - obj->parent._ref_count, - nmp_object_is_alive(obj) ? '+' : '-', - nmp_object_is_visible(obj) ? '+' : '-', - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, buf2, sizeof(buf2))); + g_snprintf(buf, + buf_size, + "[%s," NM_HASH_OBFUSCATE_PTR_FMT ",%u,%calive,%cvisible; %s]", + klass->obj_type_name, + NM_HASH_OBFUSCATE_PTR(obj), + obj->parent._ref_count, + nmp_object_is_alive(obj) ? '+' : '-', + nmp_object_is_visible(obj) ? '+' : '-', + klass->cmd_plobj_to_string(&obj->object, buf2, sizeof(buf2))); return buf; case NMP_OBJECT_TO_STRING_PUBLIC: - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, buf, buf_size); + klass->cmd_plobj_to_string(&obj->object, buf, buf_size); return buf; default: g_return_val_if_reached("ERROR"); @@ -948,7 +951,8 @@ _vt_cmd_obj_to_string_link(const NMPObject *obj, switch (to_string_mode) { case NMP_OBJECT_TO_STRING_ID: - return klass->cmd_plobj_to_string_id(&obj->object, buf, buf_size); + g_snprintf(buf, buf_size, "%d", obj->link.ifindex); + return buf; case NMP_OBJECT_TO_STRING_ALL: nm_strbuf_append(&b, &buf_size, @@ -961,7 +965,7 @@ _vt_cmd_obj_to_string_link(const NMPObject *obj, nmp_object_is_visible(obj) ? '+' : '-', obj->_link.netlink.is_in_netlink ? '+' : '-', NM_HASH_OBFUSCATE_PTR(obj->_link.udev.device)); - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, b, buf_size); + nm_platform_link_to_string(&obj->link, b, buf_size); nm_strbuf_seek_end(&b, &buf_size); if (obj->_link.netlink.lnk) { nm_strbuf_append_str(&b, &buf_size, "; "); @@ -971,7 +975,7 @@ _vt_cmd_obj_to_string_link(const NMPObject *obj, nm_strbuf_append_c(&b, &buf_size, ']'); return buf; case NMP_OBJECT_TO_STRING_PUBLIC: - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, b, buf_size); + nm_platform_link_to_string(&obj->link, b, buf_size); if (obj->_link.netlink.lnk) { nm_strbuf_seek_end(&b, &buf_size); nm_strbuf_append_str(&b, &buf_size, "; "); @@ -1013,7 +1017,7 @@ _vt_cmd_obj_to_string_lnk_vlan(const NMPObject *obj, nmp_object_to_string(obj, NMP_OBJECT_TO_STRING_PUBLIC, buf2, sizeof(buf2))); return buf; case NMP_OBJECT_TO_STRING_PUBLIC: - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, buf, buf_size); + nm_platform_lnk_vlan_to_string(&obj->lnk_vlan, buf, buf_size); b = buf; l = strlen(b); @@ -1092,8 +1096,7 @@ _vt_cmd_obj_to_string_lnk_wireguard(const NMPObject *obj, return buf; case NMP_OBJECT_TO_STRING_PUBLIC: - NMP_OBJECT_GET_CLASS(obj)->cmd_plobj_to_string(&obj->object, buf, buf_size); - + nm_platform_lnk_wireguard_to_string(&obj->lnk_wireguard, buf, buf_size); return buf; default: g_return_val_if_reached("ERROR"); @@ -1114,8 +1117,6 @@ _vt_cmd_obj_to_string_lnk_wireguard(const NMPObject *obj, } \ _NM_DUMMY_STRUCT_FOR_TRAILING_SEMICOLON -_vt_cmd_plobj_to_string_id(link, NMPlatformLink, "%d", obj->ifindex); - _vt_cmd_plobj_to_string_id( ip4_address, NMPlatformIP4Address, @@ -1147,13 +1148,15 @@ nmp_object_hash_update(const NMPObject *obj, NMHashState *h) klass = NMP_OBJECT_GET_CLASS(obj); + nm_assert((!!klass->cmd_obj_cmp) == (!!klass->cmd_obj_hash_update)); + nm_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); + nm_assert((!!klass->cmd_obj_cmp) ^ (!!klass->cmd_plobj_cmp)); + nm_hash_update_val(h, klass->obj_type); if (klass->cmd_obj_hash_update) klass->cmd_obj_hash_update(obj, h); - else if (klass->cmd_plobj_hash_update) - klass->cmd_plobj_hash_update(&obj->object, h); else - nm_hash_update_val(h, obj); + klass->cmd_plobj_hash_update(&obj->object, h); } static void @@ -1231,6 +1234,10 @@ nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFl } } + nm_assert((!!klass1->cmd_obj_cmp) == (!!klass1->cmd_obj_hash_update)); + nm_assert((!!klass1->cmd_plobj_cmp) == (!!klass1->cmd_plobj_hash_update)); + nm_assert((!!klass1->cmd_obj_cmp) ^ (!!klass1->cmd_plobj_cmp)); + if (klass1->cmd_obj_cmp) return klass1->cmd_obj_cmp(obj1, obj2); return klass1->cmd_plobj_cmp(&obj1->object, &obj2->object); @@ -3148,10 +3155,6 @@ const NMPClass _nmp_classes[NMP_OBJECT_TYPE_MAX] = { .cmd_plobj_id_copy = _vt_cmd_plobj_id_copy_link, .cmd_plobj_id_cmp = _vt_cmd_plobj_id_cmp_link, .cmd_plobj_id_hash_update = _vt_cmd_plobj_id_hash_update_link, - .cmd_plobj_to_string_id = _vt_cmd_plobj_to_string_id_link, - .cmd_plobj_to_string = (CmdPlobjToStringFunc) nm_platform_link_to_string, - .cmd_plobj_hash_update = (CmdPlobjHashUpdateFunc) nm_platform_link_hash_update, - .cmd_plobj_cmp = (CmdPlobjCmpFunc) nm_platform_link_cmp, }, [NMP_OBJECT_TYPE_IP4_ADDRESS - 1] = { @@ -3451,20 +3454,17 @@ const NMPClass _nmp_classes[NMP_OBJECT_TYPE_MAX] = { }, [NMP_OBJECT_TYPE_LNK_VLAN - 1] = { - .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), - .obj_type = NMP_OBJECT_TYPE_LNK_VLAN, - .sizeof_data = sizeof(NMPObjectLnkVlan), - .sizeof_public = sizeof(NMPlatformLnkVlan), - .obj_type_name = "vlan", - .lnk_link_type = NM_LINK_TYPE_VLAN, - .cmd_obj_hash_update = _vt_cmd_obj_hash_update_lnk_vlan, - .cmd_obj_cmp = _vt_cmd_obj_cmp_lnk_vlan, - .cmd_obj_copy = _vt_cmd_obj_copy_lnk_vlan, - .cmd_obj_dispose = _vt_cmd_obj_dispose_lnk_vlan, - .cmd_obj_to_string = _vt_cmd_obj_to_string_lnk_vlan, - .cmd_plobj_to_string = (CmdPlobjToStringFunc) nm_platform_lnk_vlan_to_string, - .cmd_plobj_hash_update = (CmdPlobjHashUpdateFunc) nm_platform_lnk_vlan_hash_update, - .cmd_plobj_cmp = (CmdPlobjCmpFunc) nm_platform_lnk_vlan_cmp, + .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), + .obj_type = NMP_OBJECT_TYPE_LNK_VLAN, + .sizeof_data = sizeof(NMPObjectLnkVlan), + .sizeof_public = sizeof(NMPlatformLnkVlan), + .obj_type_name = "vlan", + .lnk_link_type = NM_LINK_TYPE_VLAN, + .cmd_obj_hash_update = _vt_cmd_obj_hash_update_lnk_vlan, + .cmd_obj_cmp = _vt_cmd_obj_cmp_lnk_vlan, + .cmd_obj_copy = _vt_cmd_obj_copy_lnk_vlan, + .cmd_obj_dispose = _vt_cmd_obj_dispose_lnk_vlan, + .cmd_obj_to_string = _vt_cmd_obj_to_string_lnk_vlan, }, [NMP_OBJECT_TYPE_LNK_VRF - 1] = { @@ -3492,20 +3492,17 @@ const NMPClass _nmp_classes[NMP_OBJECT_TYPE_MAX] = { }, [NMP_OBJECT_TYPE_LNK_WIREGUARD - 1] = { - .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), - .obj_type = NMP_OBJECT_TYPE_LNK_WIREGUARD, - .sizeof_data = sizeof(NMPObjectLnkWireGuard), - .sizeof_public = sizeof(NMPlatformLnkWireGuard), - .obj_type_name = "wireguard", - .lnk_link_type = NM_LINK_TYPE_WIREGUARD, - .cmd_obj_hash_update = _vt_cmd_obj_hash_update_lnk_wireguard, - .cmd_obj_cmp = _vt_cmd_obj_cmp_lnk_wireguard, - .cmd_obj_copy = _vt_cmd_obj_copy_lnk_wireguard, - .cmd_obj_dispose = _vt_cmd_obj_dispose_lnk_wireguard, - .cmd_obj_to_string = _vt_cmd_obj_to_string_lnk_wireguard, - .cmd_plobj_to_string = (CmdPlobjToStringFunc) nm_platform_lnk_wireguard_to_string, - .cmd_plobj_hash_update = (CmdPlobjHashUpdateFunc) nm_platform_lnk_wireguard_hash_update, - .cmd_plobj_cmp = (CmdPlobjCmpFunc) nm_platform_lnk_wireguard_cmp, + .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), + .obj_type = NMP_OBJECT_TYPE_LNK_WIREGUARD, + .sizeof_data = sizeof(NMPObjectLnkWireGuard), + .sizeof_public = sizeof(NMPlatformLnkWireGuard), + .obj_type_name = "wireguard", + .lnk_link_type = NM_LINK_TYPE_WIREGUARD, + .cmd_obj_hash_update = _vt_cmd_obj_hash_update_lnk_wireguard, + .cmd_obj_cmp = _vt_cmd_obj_cmp_lnk_wireguard, + .cmd_obj_copy = _vt_cmd_obj_copy_lnk_wireguard, + .cmd_obj_dispose = _vt_cmd_obj_dispose_lnk_wireguard, + .cmd_obj_to_string = _vt_cmd_obj_to_string_lnk_wireguard, }, [NMP_OBJECT_TYPE_LNK_BOND - 1] = { diff --git a/src/libnm-platform/nmp-object.h b/src/libnm-platform/nmp-object.h index 4958404a47..350077ef79 100644 --- a/src/libnm-platform/nmp-object.h +++ b/src/libnm-platform/nmp-object.h @@ -183,25 +183,26 @@ typedef struct { /* Only for NMPObjectLnk* types. */ NMLinkType lnk_link_type; - void (*cmd_obj_hash_update)(const NMPObject *obj, NMHashState *h); - int (*cmd_obj_cmp)(const NMPObject *obj1, const NMPObject *obj2); - void (*cmd_obj_copy)(NMPObject *dst, const NMPObject *src); - void (*cmd_obj_dispose)(NMPObject *obj); gboolean (*cmd_obj_is_alive)(const NMPObject *obj); gboolean (*cmd_obj_is_visible)(const NMPObject *obj); + void (*cmd_obj_copy)(NMPObject *dst, const NMPObject *src); + void (*cmd_obj_dispose)(NMPObject *obj); + + void (*cmd_obj_hash_update)(const NMPObject *obj, NMHashState *h); + int (*cmd_obj_cmp)(const NMPObject *obj1, const NMPObject *obj2); const char *(*cmd_obj_to_string)(const NMPObject *obj, NMPObjectToStringMode to_string_mode, char *buf, gsize buf_size); /* functions that operate on NMPlatformObject */ + void (*cmd_plobj_hash_update)(const NMPlatformObject *obj, NMHashState *h); + int (*cmd_plobj_cmp)(const NMPlatformObject *obj1, const NMPlatformObject *obj2); void (*cmd_plobj_id_copy)(NMPlatformObject *dst, const NMPlatformObject *src); int (*cmd_plobj_id_cmp)(const NMPlatformObject *obj1, const NMPlatformObject *obj2); void (*cmd_plobj_id_hash_update)(const NMPlatformObject *obj, NMHashState *h); const char *(*cmd_plobj_to_string_id)(const NMPlatformObject *obj, char *buf, gsize buf_size); const char *(*cmd_plobj_to_string)(const NMPlatformObject *obj, char *buf, gsize len); - void (*cmd_plobj_hash_update)(const NMPlatformObject *obj, NMHashState *h); - int (*cmd_plobj_cmp)(const NMPlatformObject *obj1, const NMPlatformObject *obj2); } NMPClass; extern const NMPClass _nmp_classes[NMP_OBJECT_TYPE_MAX]; From 5da0d18fbe045db8036dfd489b999b3c91c1c72f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 4 Nov 2022 12:24:08 +0100 Subject: [PATCH 3/7] platform/tests: add unit test checking consistency of NMPClass --- src/libnm-platform/tests/test-nm-platform.c | 57 ++++++++++++++++++--- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/src/libnm-platform/tests/test-nm-platform.c b/src/libnm-platform/tests/test-nm-platform.c index cd54df9291..e69ceac219 100644 --- a/src/libnm-platform/tests/test-nm-platform.c +++ b/src/libnm-platform/tests/test-nm-platform.c @@ -6,19 +6,12 @@ #include "libnm-platform/nm-netlink.h" #include "libnm-platform/nmp-netns.h" #include "libnm-platform/nm-platform-utils.h" +#include "libnm-platform/nmp-object.h" #include "libnm-glib-aux/nm-test-utils.h" /*****************************************************************************/ -void -_nm_logging_clear_platform_logging_cache(void) -{ - /* this symbols is required by nm-log-core library. */ -} - -/*****************************************************************************/ - static void test_use_symbols(void) { @@ -140,6 +133,53 @@ test_nmp_link_mode_all_advertised_modes_bits(void) /*****************************************************************************/ +static void +test_nmpclass_consistency(void) +{ + NMPObjectType obj_type; + NMPObjectType obj_type2; + + G_STATIC_ASSERT(G_N_ELEMENTS(_nmp_classes) == NMP_OBJECT_TYPE_MAX); + + for (obj_type = 1; obj_type <= NMP_OBJECT_TYPE_MAX; obj_type++) { + const NMPClass *klass = nmp_class_from_type(obj_type); + gboolean is_lnk; + + g_assert(klass); + g_assert(klass == &_nmp_classes[obj_type - 1]); + + g_assert_cmpint(klass->obj_type, ==, obj_type); + g_assert(klass->obj_type_name); + + g_assert((!!klass->cmd_obj_cmp) == (!!klass->cmd_obj_hash_update)); + g_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); + + g_assert((!!klass->cmd_obj_cmp) != (!!klass->cmd_plobj_cmp)); + g_assert((!!klass->cmd_obj_hash_update) != (!!klass->cmd_plobj_hash_update)); + + g_assert_cmpint(klass->sizeof_public, >, 0); + g_assert_cmpint(klass->sizeof_data, >=, klass->sizeof_public); + + g_assert((!!klass->cmd_obj_to_string) != (!!klass->cmd_plobj_to_string)); + g_assert(!klass->cmd_plobj_to_string_id || klass->cmd_plobj_to_string); + + is_lnk = (obj_type >= NMP_OBJECT_TYPE_LNK_BRIDGE && obj_type <= NMP_OBJECT_TYPE_LNK_BOND); + if (klass->lnk_link_type == NM_LINK_TYPE_NONE) { + G_STATIC_ASSERT(NM_LINK_TYPE_NONE == 0); + g_assert(!is_lnk); + } else + g_assert(is_lnk); + + for (obj_type2 = 1; obj_type2 < obj_type; obj_type2++) { + const NMPClass *klass2 = nmp_class_from_type(obj_type2); + + g_assert_cmpstr(klass->obj_type_name, !=, klass2->obj_type_name); + } + } +} + +/*****************************************************************************/ + NMTST_DEFINE(); int @@ -150,6 +190,7 @@ main(int argc, char **argv) g_test_add_func("/nm-platform/test_use_symbols", test_use_symbols); g_test_add_func("/nm-platform/test_nmp_link_mode_all_advertised_modes_bits", test_nmp_link_mode_all_advertised_modes_bits); + g_test_add_func("/nm-platform/test_nmpclass_consistency", test_nmpclass_consistency); return g_test_run(); } From ff63b2eb6e53b19e8bf496210b79090ebeb8a43f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Nov 2022 10:54:53 +0100 Subject: [PATCH 4/7] platform: unify full/id hash/cmp implementations for NMPObject --- src/libnm-platform/nmp-object.c | 136 +++++++++----------- src/libnm-platform/nmp-object.h | 39 +++++- src/libnm-platform/tests/test-nm-platform.c | 1 + 3 files changed, 97 insertions(+), 79 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index a8da9d6e8e..41212d0fbb 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -1140,7 +1140,7 @@ _vt_cmd_plobj_to_string_id(qdisc, NMPlatformQdisc, "%d: %d", obj->ifindex, obj-> _vt_cmd_plobj_to_string_id(tfilter, NMPlatformTfilter, "%d: %d", obj->ifindex, obj->parent); void -nmp_object_hash_update(const NMPObject *obj, NMHashState *h) +nmp_object_hash_update_full(const NMPObject *obj, gboolean for_id, NMHashState *h) { const NMPClass *klass; @@ -1148,9 +1148,24 @@ nmp_object_hash_update(const NMPObject *obj, NMHashState *h) klass = NMP_OBJECT_GET_CLASS(obj); + nm_assert((!!klass->cmd_plobj_id_cmp) == (!!klass->cmd_plobj_id_hash_update)); nm_assert((!!klass->cmd_obj_cmp) == (!!klass->cmd_obj_hash_update)); nm_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); - nm_assert((!!klass->cmd_obj_cmp) ^ (!!klass->cmd_plobj_cmp)); + + nm_assert((!!klass->cmd_obj_hash_update) ^ (!!klass->cmd_plobj_hash_update)); + + if (for_id) { + if (!klass->cmd_plobj_id_hash_update) { + /* The klass doesn't implement ID compare. It means, to use pointer + * equality. */ + nm_hash_update_val(h, obj); + return; + } + + nm_hash_update_val(h, klass->obj_type); + klass->cmd_plobj_id_hash_update(&obj->object, h); + return; + } nm_hash_update_val(h, klass->obj_type); if (klass->cmd_obj_hash_update) @@ -1201,46 +1216,72 @@ _vt_cmd_obj_hash_update_lnk_wireguard(const NMPObject *obj, NMHashState *h) int nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFlags flags) { - const NMPClass *klass1; + const NMPClass *klass; const NMPClass *klass2; NMPObject obj_stackcopy; + nm_assert(!NM_FLAGS_ANY(flags, + ~(NMP_OBJECT_CMP_FLAGS_ID | NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX))); + + /* The ID flag (currently) cannot be combined with other flags. That's partly because + * it's not implemented, but also because some objects use only pointer equality. So it's + * not clear how that combines with other flags (well, they'd be ignored). */ + nm_assert(!NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_ID) + || flags == NMP_OBJECT_CMP_FLAGS_ID); + NM_CMP_SELF(obj1, obj2); g_return_val_if_fail(NMP_OBJECT_IS_VALID(obj1), -1); g_return_val_if_fail(NMP_OBJECT_IS_VALID(obj2), 1); - klass1 = NMP_OBJECT_GET_CLASS(obj1); + klass = NMP_OBJECT_GET_CLASS(obj1); + + nm_assert(klass); + + nm_assert((!!klass->cmd_plobj_id_cmp) == (!!klass->cmd_plobj_id_hash_update)); + nm_assert((!!klass->cmd_obj_cmp) == (!!klass->cmd_obj_hash_update)); + nm_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); + + nm_assert((!!klass->cmd_obj_cmp) ^ (!!klass->cmd_plobj_cmp)); + klass2 = NMP_OBJECT_GET_CLASS(obj2); - if (klass1 != klass2) { - nm_assert(klass1->obj_type != klass2->obj_type); - return klass1->obj_type < klass2->obj_type ? -1 : 1; + if (klass != klass2) { + nm_assert(klass2); + NM_CMP_DIRECT(klass->obj_type, klass2->obj_type); + return nm_assert_unreachable_val(0); + } + + if (NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_ID)) { + if (!klass->cmd_plobj_id_cmp) { + /* the klass doesn't implement ID cmp(). That means, different objects + * never compare equal, but the cmp() according to their pointer value. */ + NM_CMP_DIRECT_PTR(obj1, obj2); + return nm_assert_unreachable_val(0); + } + + return klass->cmd_plobj_id_cmp(&obj1->object, &obj2->object); } if (NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX)) { - if (!NM_IN_SET(klass1, - nmp_class_from_type(NMP_OBJECT_TYPE_IP4_ADDRESS), - nmp_class_from_type(NMP_OBJECT_TYPE_IP6_ADDRESS), - nmp_class_from_type(NMP_OBJECT_TYPE_IP4_ROUTE), - nmp_class_from_type(NMP_OBJECT_TYPE_IP6_ROUTE))) { + if (!NM_IN_SET(klass->obj_type, + NMP_OBJECT_TYPE_IP4_ADDRESS, + NMP_OBJECT_TYPE_IP6_ADDRESS, + NMP_OBJECT_TYPE_IP4_ROUTE, + NMP_OBJECT_TYPE_IP6_ROUTE)) { /* This flag is currently only implemented for certain types. * That is, because we just create a stack copy, and that naive * approach only knows for types where we know that it works. */ } else if (obj1->obj_with_ifindex.ifindex != obj2->obj_with_ifindex.ifindex) { - nmp_object_stackinit(&obj_stackcopy, klass1->obj_type, &obj2->obj_with_ifindex); + nmp_object_stackinit(&obj_stackcopy, klass->obj_type, &obj2->obj_with_ifindex); obj_stackcopy.obj_with_ifindex.ifindex = obj1->obj_with_ifindex.ifindex; obj2 = &obj_stackcopy; } } - nm_assert((!!klass1->cmd_obj_cmp) == (!!klass1->cmd_obj_hash_update)); - nm_assert((!!klass1->cmd_plobj_cmp) == (!!klass1->cmd_plobj_hash_update)); - nm_assert((!!klass1->cmd_obj_cmp) ^ (!!klass1->cmd_plobj_cmp)); - - if (klass1->cmd_obj_cmp) - return klass1->cmd_obj_cmp(obj1, obj2); - return klass1->cmd_plobj_cmp(&obj1->object, &obj2->object); + if (klass->cmd_obj_cmp) + return klass->cmd_obj_cmp(obj1, obj2); + return klass->cmd_plobj_cmp(&obj1->object, &obj2->object); } static int @@ -1457,39 +1498,6 @@ nmp_object_clone(const NMPObject *obj, gboolean id_only) return dst; } -int -nmp_object_id_cmp(const NMPObject *obj1, const NMPObject *obj2) -{ - const NMPClass *klass, *klass2; - - NM_CMP_SELF(obj1, obj2); - - g_return_val_if_fail(NMP_OBJECT_IS_VALID(obj1), FALSE); - g_return_val_if_fail(NMP_OBJECT_IS_VALID(obj2), FALSE); - - klass = NMP_OBJECT_GET_CLASS(obj1); - nm_assert(!klass->cmd_plobj_id_hash_update == !klass->cmd_plobj_id_cmp); - - klass2 = NMP_OBJECT_GET_CLASS(obj2); - nm_assert(klass); - if (klass != klass2) { - nm_assert(klass2); - NM_CMP_DIRECT(klass->obj_type, klass2->obj_type); - /* resort to pointer comparison */ - NM_CMP_DIRECT_PTR(klass, klass2); - return 0; - } - - if (!klass->cmd_plobj_id_cmp) { - /* the klass doesn't implement ID cmp(). That means, different objects - * never compare equal, but the cmp() according to their pointer value. */ - NM_CMP_DIRECT_PTR(obj1, obj2); - return 0; - } - - return klass->cmd_plobj_id_cmp(&obj1->object, &obj2->object); -} - #define _vt_cmd_plobj_id_cmp(type, plat_type, cmd) \ static int _vt_cmd_plobj_id_cmp_##type(const NMPlatformObject *_obj1, \ const NMPlatformObject *_obj2) \ @@ -1588,28 +1596,6 @@ _vt_cmd_plobj_id_cmp(mptcp_addr, NMPlatformMptcpAddr, { NM_CMP_FIELD(obj1, obj2, port); }); -void -nmp_object_id_hash_update(const NMPObject *obj, NMHashState *h) -{ - const NMPClass *klass; - - g_return_if_fail(NMP_OBJECT_IS_VALID(obj)); - - klass = NMP_OBJECT_GET_CLASS(obj); - - nm_assert(!klass->cmd_plobj_id_hash_update == !klass->cmd_plobj_id_cmp); - - if (!klass->cmd_plobj_id_hash_update) { - /* The klass doesn't implement ID compare. It means, to use pointer - * equality. */ - nm_hash_update_val(h, obj); - return; - } - - nm_hash_update_val(h, klass->obj_type); - klass->cmd_plobj_id_hash_update(&obj->object, h); -} - guint nmp_object_id_hash(const NMPObject *obj) { diff --git a/src/libnm-platform/nmp-object.h b/src/libnm-platform/nmp-object.h index 350077ef79..f0df52aed7 100644 --- a/src/libnm-platform/nmp-object.h +++ b/src/libnm-platform/nmp-object.h @@ -718,14 +718,35 @@ const char *nmp_object_to_string(const NMPObject *obj, NMPObjectToStringMode to_string_mode, char *buf, gsize buf_size); -void nmp_object_hash_update(const NMPObject *obj, NMHashState *h); + +void nmp_object_hash_update_full(const NMPObject *obj, gboolean for_id, NMHashState *h); + +static inline void +nmp_object_hash_update(const NMPObject *obj, NMHashState *h) +{ + return nmp_object_hash_update_full(obj, FALSE, h); +} typedef enum { NMP_OBJECT_CMP_FLAGS_NONE = 0, + /* Only compare for the ID. This is what nmp_object_id_cmp() does. + * + * In most cases, the identity of an object is a (non-strict) subset + * of the attributes of the object. + * + * However, for some objects (like NMPObjectLnk) there is on concept + * of identity. They implement object identity based on pointer equality + * (in that case, the ID is not a subset of the object's attributes). + * + * That's why this flag (currently) cannot be meaningfully combined with + * other flags. + */ + NMP_OBJECT_CMP_FLAGS_ID = NM_BIT(0), + /* Warning: this flag is currently only implemented for certain object types * (address and routes). */ - NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX = (1llu << 0), + NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX = NM_BIT(1), } NMPObjectCmpFlags; int nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFlags flags); @@ -745,8 +766,18 @@ nmp_object_equal(const NMPObject *obj1, const NMPObject *obj2) void nmp_object_copy(NMPObject *dst, const NMPObject *src, gboolean id_only); NMPObject *nmp_object_clone(const NMPObject *obj, gboolean id_only); -int nmp_object_id_cmp(const NMPObject *obj1, const NMPObject *obj2); -void nmp_object_id_hash_update(const NMPObject *obj, NMHashState *h); +static inline int +nmp_object_id_cmp(const NMPObject *obj1, const NMPObject *obj2) +{ + return nmp_object_cmp_full(obj1, obj2, NMP_OBJECT_CMP_FLAGS_ID); +} + +static inline void +nmp_object_id_hash_update(const NMPObject *obj, NMHashState *h) +{ + return nmp_object_hash_update_full(obj, TRUE, h); +} + guint nmp_object_id_hash(const NMPObject *obj); static inline gboolean diff --git a/src/libnm-platform/tests/test-nm-platform.c b/src/libnm-platform/tests/test-nm-platform.c index e69ceac219..a5fdeab33f 100644 --- a/src/libnm-platform/tests/test-nm-platform.c +++ b/src/libnm-platform/tests/test-nm-platform.c @@ -153,6 +153,7 @@ test_nmpclass_consistency(void) g_assert((!!klass->cmd_obj_cmp) == (!!klass->cmd_obj_hash_update)); g_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); + g_assert((!!klass->cmd_plobj_id_cmp) == (!!klass->cmd_plobj_id_hash_update)); g_assert((!!klass->cmd_obj_cmp) != (!!klass->cmd_plobj_cmp)); g_assert((!!klass->cmd_obj_hash_update) != (!!klass->cmd_plobj_hash_update)); From c9123c2eced0e321939d7bd51071225f2b27c1ab Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Nov 2022 12:08:36 +0100 Subject: [PATCH 5/7] platform: extend cmd_obj_{hash_update,cmp}() hooks to check for identity We will extend IPv4 routes with the list of next hops. This field will be heap allocated and be part of the NMPObjectIP4Route object, while also being part of the identity. To support the ID operator that checks fields of the NMPObject, add a "for_id" argument to the hash/cmp hooks. Also, a function that sets cmd_obj_{hash_update,cmp}() MUST not set cmd_plobj_id_{hashupdate,cmp}(), as it would have overlapping functionality. Therefore, the objects that define cmd_obj_{hash_update,cmp}() need to fully implement the ID comparison. --- src/libnm-platform/nmp-object.c | 123 ++++++++++++-------- src/libnm-platform/nmp-object.h | 4 +- src/libnm-platform/tests/test-nm-platform.c | 3 + 3 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 41212d0fbb..5192cb2135 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -1153,32 +1153,39 @@ nmp_object_hash_update_full(const NMPObject *obj, gboolean for_id, NMHashState * nm_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); nm_assert((!!klass->cmd_obj_hash_update) ^ (!!klass->cmd_plobj_hash_update)); + nm_assert((!klass->cmd_obj_hash_update) || (!klass->cmd_plobj_id_hash_update)); + + nm_hash_update_val(h, klass->obj_type); if (for_id) { - if (!klass->cmd_plobj_id_hash_update) { + if (klass->cmd_obj_hash_update) + klass->cmd_obj_hash_update(obj, TRUE, h); + else if (klass->cmd_plobj_id_hash_update) + klass->cmd_plobj_id_hash_update(&obj->object, h); + else { /* The klass doesn't implement ID compare. It means, to use pointer * equality. */ nm_hash_update_val(h, obj); - return; } - - nm_hash_update_val(h, klass->obj_type); - klass->cmd_plobj_id_hash_update(&obj->object, h); return; } - nm_hash_update_val(h, klass->obj_type); if (klass->cmd_obj_hash_update) - klass->cmd_obj_hash_update(obj, h); + klass->cmd_obj_hash_update(obj, FALSE, h); else klass->cmd_plobj_hash_update(&obj->object, h); } static void -_vt_cmd_obj_hash_update_link(const NMPObject *obj, NMHashState *h) +_vt_cmd_obj_hash_update_link(const NMPObject *obj, gboolean for_id, NMHashState *h) { nm_assert(NMP_OBJECT_GET_TYPE(obj) == NMP_OBJECT_TYPE_LINK); + if (for_id) { + nm_hash_update_val(h, obj->link.ifindex); + return; + } + nm_platform_link_hash_update(&obj->link, h); nm_hash_update_vals(h, obj->_link.netlink.is_in_netlink, obj->_link.udev.device); if (obj->_link.netlink.lnk) @@ -1186,10 +1193,15 @@ _vt_cmd_obj_hash_update_link(const NMPObject *obj, NMHashState *h) } static void -_vt_cmd_obj_hash_update_lnk_vlan(const NMPObject *obj, NMHashState *h) +_vt_cmd_obj_hash_update_lnk_vlan(const NMPObject *obj, gboolean for_id, NMHashState *h) { nm_assert(NMP_OBJECT_GET_TYPE(obj) == NMP_OBJECT_TYPE_LNK_VLAN); + if (for_id) { + nm_hash_update_val(h, obj); + return; + } + nm_platform_lnk_vlan_hash_update(&obj->lnk_vlan, h); _vlan_xgress_qos_mappings_hash_update(obj->_lnk_vlan.n_ingress_qos_map, obj->_lnk_vlan.ingress_qos_map, @@ -1200,12 +1212,17 @@ _vt_cmd_obj_hash_update_lnk_vlan(const NMPObject *obj, NMHashState *h) } static void -_vt_cmd_obj_hash_update_lnk_wireguard(const NMPObject *obj, NMHashState *h) +_vt_cmd_obj_hash_update_lnk_wireguard(const NMPObject *obj, gboolean for_id, NMHashState *h) { guint i; nm_assert(NMP_OBJECT_GET_TYPE(obj) == NMP_OBJECT_TYPE_LNK_WIREGUARD); + if (for_id) { + nm_hash_update_val(h, obj); + return; + } + nm_platform_lnk_wireguard_hash_update(&obj->lnk_wireguard, h); nm_hash_update_val(h, obj->_lnk_wireguard.peers_len); @@ -1220,14 +1237,13 @@ nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFl const NMPClass *klass2; NMPObject obj_stackcopy; - nm_assert(!NM_FLAGS_ANY(flags, - ~(NMP_OBJECT_CMP_FLAGS_ID | NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX))); + nm_assert( + !NM_FLAGS_ANY(flags, ~(NMP_OBJECT_CMP_FLAGS_ID | NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX))); /* The ID flag (currently) cannot be combined with other flags. That's partly because * it's not implemented, but also because some objects use only pointer equality. So it's * not clear how that combines with other flags (well, they'd be ignored). */ - nm_assert(!NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_ID) - || flags == NMP_OBJECT_CMP_FLAGS_ID); + nm_assert(!NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_ID) || flags == NMP_OBJECT_CMP_FLAGS_ID); NM_CMP_SELF(obj1, obj2); @@ -1243,6 +1259,7 @@ nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFl nm_assert((!!klass->cmd_plobj_cmp) == (!!klass->cmd_plobj_hash_update)); nm_assert((!!klass->cmd_obj_cmp) ^ (!!klass->cmd_plobj_cmp)); + nm_assert((!klass->cmd_obj_cmp) || (!klass->cmd_plobj_id_cmp)); klass2 = NMP_OBJECT_GET_CLASS(obj2); @@ -1253,14 +1270,15 @@ nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFl } if (NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_ID)) { - if (!klass->cmd_plobj_id_cmp) { - /* the klass doesn't implement ID cmp(). That means, different objects - * never compare equal, but the cmp() according to their pointer value. */ - NM_CMP_DIRECT_PTR(obj1, obj2); - return nm_assert_unreachable_val(0); - } + if (klass->cmd_obj_cmp) + return klass->cmd_obj_cmp(obj1, obj2, TRUE); + if (klass->cmd_plobj_id_cmp) + return klass->cmd_plobj_id_cmp(&obj1->object, &obj2->object); - return klass->cmd_plobj_id_cmp(&obj1->object, &obj2->object); + /* the klass doesn't implement ID cmp(). That means, different objects + * never compare equal, but the cmp() according to their pointer value. */ + NM_CMP_DIRECT_PTR(obj1, obj2); + return nm_assert_unreachable_val(0); } if (NM_FLAGS_HAS(flags, NMP_OBJECT_CMP_FLAGS_IGNORE_IFINDEX)) { @@ -1280,13 +1298,18 @@ nmp_object_cmp_full(const NMPObject *obj1, const NMPObject *obj2, NMPObjectCmpFl } if (klass->cmd_obj_cmp) - return klass->cmd_obj_cmp(obj1, obj2); + return klass->cmd_obj_cmp(obj1, obj2, FALSE); return klass->cmd_plobj_cmp(&obj1->object, &obj2->object); } static int -_vt_cmd_obj_cmp_link(const NMPObject *obj1, const NMPObject *obj2) +_vt_cmd_obj_cmp_link(const NMPObject *obj1, const NMPObject *obj2, gboolean for_id) { + if (for_id) { + NM_CMP_FIELD(obj1, obj2, link.ifindex); + return 0; + } + NM_CMP_RETURN(nm_platform_link_cmp(&obj1->link, &obj2->link)); NM_CMP_DIRECT(obj1->_link.netlink.is_in_netlink, obj2->_link.netlink.is_in_netlink); NM_CMP_RETURN(nmp_object_cmp(obj1->_link.netlink.lnk, obj2->_link.netlink.lnk)); @@ -1308,10 +1331,15 @@ _vt_cmd_obj_cmp_link(const NMPObject *obj1, const NMPObject *obj2) } static int -_vt_cmd_obj_cmp_lnk_vlan(const NMPObject *obj1, const NMPObject *obj2) +_vt_cmd_obj_cmp_lnk_vlan(const NMPObject *obj1, const NMPObject *obj2, gboolean for_id) { int c; + if (for_id) { + NM_CMP_DIRECT_PTR(obj1, obj2); + return nm_assert_unreachable_val(0); + } + c = nm_platform_lnk_vlan_cmp(&obj1->lnk_vlan, &obj2->lnk_vlan); if (c) return c; @@ -1334,10 +1362,15 @@ _vt_cmd_obj_cmp_lnk_vlan(const NMPObject *obj1, const NMPObject *obj2) } static int -_vt_cmd_obj_cmp_lnk_wireguard(const NMPObject *obj1, const NMPObject *obj2) +_vt_cmd_obj_cmp_lnk_wireguard(const NMPObject *obj1, const NMPObject *obj2, gboolean for_id) { guint i; + if (for_id) { + NM_CMP_DIRECT_PTR(obj1, obj2); + return nm_assert_unreachable_val(0); + } + NM_CMP_RETURN(nm_platform_lnk_wireguard_cmp(&obj1->lnk_wireguard, &obj2->lnk_wireguard)); NM_CMP_FIELD(obj1, obj2, _lnk_wireguard.peers_len); @@ -1513,8 +1546,6 @@ nmp_object_clone(const NMPObject *obj, gboolean id_only) } \ _NM_DUMMY_STRUCT_FOR_TRAILING_SEMICOLON -_vt_cmd_plobj_id_cmp(link, NMPlatformLink, { NM_CMP_FIELD(obj1, obj2, ifindex); }); - static int _vt_cmd_plobj_id_cmp_ip4_address(const NMPlatformObject *obj1, const NMPlatformObject *obj2) { @@ -1619,8 +1650,6 @@ nmp_object_id_hash(const NMPObject *obj) } \ _NM_DUMMY_STRUCT_FOR_TRAILING_SEMICOLON -_vt_cmd_plobj_id_hash_update(link, NMPlatformLink, { nm_hash_update_val(h, obj->ifindex); }); - _vt_cmd_plobj_id_hash_update(ip4_address, NMPlatformIP4Address, { nm_hash_update_vals( h, @@ -3122,25 +3151,23 @@ typedef int (*CmdPlobjCmpFunc)(const NMPlatformObject *obj1, const NMPlatformObj const NMPClass _nmp_classes[NMP_OBJECT_TYPE_MAX] = { [NMP_OBJECT_TYPE_LINK - 1] = { - .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), - .obj_type = NMP_OBJECT_TYPE_LINK, - .sizeof_data = sizeof(NMPObjectLink), - .sizeof_public = sizeof(NMPlatformLink), - .obj_type_name = "link", - .rtm_gettype = RTM_GETLINK, - .signal_type_id = NM_PLATFORM_SIGNAL_ID_LINK, - .signal_type = NM_PLATFORM_SIGNAL_LINK_CHANGED, - .supported_cache_ids = _supported_cache_ids_link, - .cmd_obj_hash_update = _vt_cmd_obj_hash_update_link, - .cmd_obj_cmp = _vt_cmd_obj_cmp_link, - .cmd_obj_copy = _vt_cmd_obj_copy_link, - .cmd_obj_dispose = _vt_cmd_obj_dispose_link, - .cmd_obj_is_alive = _vt_cmd_obj_is_alive_link, - .cmd_obj_is_visible = _vt_cmd_obj_is_visible_link, - .cmd_obj_to_string = _vt_cmd_obj_to_string_link, - .cmd_plobj_id_copy = _vt_cmd_plobj_id_copy_link, - .cmd_plobj_id_cmp = _vt_cmd_plobj_id_cmp_link, - .cmd_plobj_id_hash_update = _vt_cmd_plobj_id_hash_update_link, + .parent = DEDUP_MULTI_OBJ_CLASS_INIT(), + .obj_type = NMP_OBJECT_TYPE_LINK, + .sizeof_data = sizeof(NMPObjectLink), + .sizeof_public = sizeof(NMPlatformLink), + .obj_type_name = "link", + .rtm_gettype = RTM_GETLINK, + .signal_type_id = NM_PLATFORM_SIGNAL_ID_LINK, + .signal_type = NM_PLATFORM_SIGNAL_LINK_CHANGED, + .supported_cache_ids = _supported_cache_ids_link, + .cmd_obj_hash_update = _vt_cmd_obj_hash_update_link, + .cmd_obj_cmp = _vt_cmd_obj_cmp_link, + .cmd_obj_copy = _vt_cmd_obj_copy_link, + .cmd_obj_dispose = _vt_cmd_obj_dispose_link, + .cmd_obj_is_alive = _vt_cmd_obj_is_alive_link, + .cmd_obj_is_visible = _vt_cmd_obj_is_visible_link, + .cmd_obj_to_string = _vt_cmd_obj_to_string_link, + .cmd_plobj_id_copy = _vt_cmd_plobj_id_copy_link, }, [NMP_OBJECT_TYPE_IP4_ADDRESS - 1] = { diff --git a/src/libnm-platform/nmp-object.h b/src/libnm-platform/nmp-object.h index f0df52aed7..090436d217 100644 --- a/src/libnm-platform/nmp-object.h +++ b/src/libnm-platform/nmp-object.h @@ -188,8 +188,8 @@ typedef struct { void (*cmd_obj_copy)(NMPObject *dst, const NMPObject *src); void (*cmd_obj_dispose)(NMPObject *obj); - void (*cmd_obj_hash_update)(const NMPObject *obj, NMHashState *h); - int (*cmd_obj_cmp)(const NMPObject *obj1, const NMPObject *obj2); + void (*cmd_obj_hash_update)(const NMPObject *obj, gboolean for_id, NMHashState *h); + int (*cmd_obj_cmp)(const NMPObject *obj1, const NMPObject *obj2, gboolean for_id); const char *(*cmd_obj_to_string)(const NMPObject *obj, NMPObjectToStringMode to_string_mode, char *buf, diff --git a/src/libnm-platform/tests/test-nm-platform.c b/src/libnm-platform/tests/test-nm-platform.c index a5fdeab33f..19d81bf4d2 100644 --- a/src/libnm-platform/tests/test-nm-platform.c +++ b/src/libnm-platform/tests/test-nm-platform.c @@ -158,6 +158,9 @@ test_nmpclass_consistency(void) g_assert((!!klass->cmd_obj_cmp) != (!!klass->cmd_plobj_cmp)); g_assert((!!klass->cmd_obj_hash_update) != (!!klass->cmd_plobj_hash_update)); + g_assert((!klass->cmd_obj_cmp) || (!klass->cmd_plobj_id_cmp)); + g_assert((!klass->cmd_obj_hash_update) || (!klass->cmd_plobj_id_hash_update)); + g_assert_cmpint(klass->sizeof_public, >, 0); g_assert_cmpint(klass->sizeof_data, >=, klass->sizeof_public); From dd2c5044f6ff7eb1605a3e62f6dc9a95cdbd4567 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Nov 2022 17:31:25 +0100 Subject: [PATCH 6/7] platform: add internal helper function to get full NMPObject size --- src/libnm-platform/nmp-object.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index 5192cb2135..fdd0a9e418 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -762,17 +762,23 @@ _vt_cmd_obj_dispose_lnk_wireguard(NMPObject *obj) _wireguard_clear(&obj->_lnk_wireguard); } +static gsize +_NMP_OBJECT_STRUCT_SIZE(const NMPClass *klass) +{ + nm_assert(klass); + nm_assert(klass->sizeof_public > 0); + nm_assert(klass->sizeof_public <= klass->sizeof_data); + + return klass->sizeof_data + G_STRUCT_OFFSET(NMPObject, object); +} + static NMPObject * _nmp_object_new_from_class(const NMPClass *klass) { NMPObject *obj; - nm_assert(klass); - nm_assert(klass->sizeof_data > 0); - nm_assert(klass->sizeof_public > 0 && klass->sizeof_public <= klass->sizeof_data); - - obj = g_slice_alloc0(klass->sizeof_data + G_STRUCT_OFFSET(NMPObject, object)); - obj->_class = klass; + obj = g_slice_alloc0(_NMP_OBJECT_STRUCT_SIZE(klass)); + obj->_class = klass; obj->parent._ref_count = 1; return obj; } @@ -1880,7 +1886,7 @@ _vt_dedup_obj_destroy(NMDedupMultiObj *obj) klass = o->_class; if (klass->cmd_obj_dispose) klass->cmd_obj_dispose(o); - g_slice_free1(klass->sizeof_data + G_STRUCT_OFFSET(NMPObject, object), o); + g_slice_free1(_NMP_OBJECT_STRUCT_SIZE(klass), o); } static const NMDedupMultiObj * From 57b23c12cc8cda8d39b7de456415f6a6e54e97f7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 7 Nov 2022 17:32:57 +0100 Subject: [PATCH 7/7] platform: only initialize actual data for stackinit NMPObject The NMPObject is a tagged union. There is no need to initialize anything after the size of the actually used union field. Change this, so maybe we get a valgrind warning about uninitialized memory if we wrongly try to access it. On the other hand, the object really is supposed to be a full NMPObject. Previously, we would get a valgrind warning, if we tried to pass fewer data there. It really doesn't matter much, but all other functions don't assume that there is any important data after the size indicated by the class. --- src/libnm-platform/nmp-object.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libnm-platform/nmp-object.c b/src/libnm-platform/nmp-object.c index fdd0a9e418..a6ad5f65b0 100644 --- a/src/libnm-platform/nmp-object.c +++ b/src/libnm-platform/nmp-object.c @@ -811,9 +811,8 @@ static NMPObject * _nmp_object_stackinit_from_class(NMPObject *obj, const NMPClass *klass) { nm_assert(obj); - nm_assert(klass); - memset(obj, 0, sizeof(NMPObject)); + memset(obj, 0, _NMP_OBJECT_STRUCT_SIZE(klass)); obj->_class = klass; obj->parent._ref_count = NM_OBJ_REF_COUNT_STACKINIT; return obj;