From 94c025452bd3f8248ce20fe5e40e123698225e80 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 5 Aug 2017 15:14:44 +0200 Subject: [PATCH] platform: preserve order of objects during dump NMPCache can preserve the order of the objects. Until now, the order was however arbitrary. Soon we will require to preserve the order of routes. During a dump, force appending new objects at the end. That ensures, correct ordering during the dump. Note that we track objects in several distrinct indexes. Those partition the set of all objects. Outside a dump when receiving events about new objects (e.g. RTM_NEWROUTE), it is very unclear at which place the new object should be sorted. It is especially unclear, as an object might move from one partition (of an index) to another. In general, a deterministic order will only be useful in one particular instance: the NMP_CACHE_ID_TYPE_ROUTES_BY_DESTINATION index for routes. In this case, we will ensure a particular order of the routes. --- src/platform/nm-fake-platform.c | 11 ++++-- src/platform/nm-linux-platform.c | 21 ++++++++--- src/platform/nmp-object.c | 52 ++++++++++++++++++++-------- src/platform/nmp-object.h | 8 ++++- src/platform/tests/test-nmp-object.c | 2 +- 5 files changed, 69 insertions(+), 25 deletions(-) diff --git a/src/platform/nm-fake-platform.c b/src/platform/nm-fake-platform.c index 338b022e79..9adf2b43e7 100644 --- a/src/platform/nm-fake-platform.c +++ b/src/platform/nm-fake-platform.c @@ -311,6 +311,7 @@ link_add (NMPlatform *platform, link_add_prepare (platform, device, (NMPObject *) device->obj); cache_op = nmp_cache_update_netlink (nm_platform_get_cache (platform), (NMPObject *) device->obj, + FALSE, &obj_old, &obj_new); g_assert (cache_op == NMP_CACHE_OPS_ADDED); nmp_object_unref (device->obj); @@ -319,6 +320,7 @@ link_add (NMPlatform *platform, link_add_prepare (platform, device_veth, (NMPObject *) device_veth->obj); cache_op_veth = nmp_cache_update_netlink (nm_platform_get_cache (platform), (NMPObject *) device_veth->obj, + FALSE, &obj_old_veth, &obj_new_veth); g_assert (cache_op == NMP_CACHE_OPS_ADDED); nmp_object_unref (device->obj); @@ -359,6 +361,7 @@ link_add_one (NMPlatform *platform, link_add_prepare (platform, device, (NMPObject *) device->obj); cache_op = nmp_cache_update_netlink (nm_platform_get_cache (platform), (NMPObject *) device->obj, + FALSE, &obj_old, &obj_new); g_assert (cache_op == NMP_CACHE_OPS_ADDED); nmp_object_unref (device->obj); @@ -431,7 +434,9 @@ link_set_obj (NMPlatform *platform, link_add_prepare (platform, device, obj_tmp); cache_op = nmp_cache_update_netlink (nm_platform_get_cache (platform), - obj_tmp, &obj_old, &obj_new); + obj_tmp, + FALSE, + &obj_old, &obj_new); g_assert (NM_IN_SET (cache_op, NMP_CACHE_OPS_UNCHANGED, NMP_CACHE_OPS_UPDATED)); g_assert (obj_old == device->obj); @@ -971,7 +976,7 @@ ipx_address_add (NMPlatform *platform, int addr_family, const NMPlatformObject * : NMP_OBJECT_TYPE_IP6_ADDRESS, address); - cache_op = nmp_cache_update_netlink (cache, obj, &obj_old, &obj_new); + cache_op = nmp_cache_update_netlink (cache, obj, FALSE, &obj_old, &obj_new); nm_platform_cache_update_emit_signal (platform, cache_op, obj_old, obj_new); return TRUE; } @@ -1275,7 +1280,7 @@ ip_route_add (NMPlatform *platform, } } - cache_op = nmp_cache_update_netlink (cache, obj, &obj_old, &obj_new); + cache_op = nmp_cache_update_netlink (cache, obj, FALSE, &obj_old, &obj_new); nm_platform_cache_update_emit_signal (platform, cache_op, obj_old, obj_new); return TRUE; } diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 413e12edbb..e62d98ea5d 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3429,9 +3429,7 @@ cache_on_change (NMPlatform *platform, NMPCache *cache = nm_platform_get_cache (platform); ASSERT_nmp_cache_ops (cache, cache_op, obj_old, obj_new); - - if (cache_op == NMP_CACHE_OPS_UNCHANGED) - return; + nm_assert (cache_op != NMP_CACHE_OPS_UNCHANGED); klass = obj_old ? NMP_OBJECT_GET_CLASS (obj_old) : NMP_OBJECT_GET_CLASS (obj_new); @@ -3892,6 +3890,7 @@ event_valid_msg (NMPlatform *platform, struct nl_msg *msg, gboolean handle_event char buf_nlmsghdr[400]; gboolean id_only = FALSE; NMPCache *cache = nm_platform_get_cache (platform); + gboolean is_dump; msghdr = nlmsg_hdr (msg); @@ -3914,8 +3913,20 @@ event_valid_msg (NMPlatform *platform, struct nl_msg *msg, gboolean handle_event return; } - _LOGT ("event-notification: %s: %s", + switch (msghdr->nlmsg_type) { + case RTM_NEWADDR: + case RTM_NEWLINK: + case RTM_NEWROUTE: + is_dump = delayed_action_refresh_all_in_progress (platform, + delayed_action_refresh_from_object_type (NMP_OBJECT_GET_TYPE (obj))); + break; + default: + is_dump = FALSE; + } + + _LOGT ("event-notification: %s%s: %s", _nl_nlmsghdr_to_str (msghdr, buf_nlmsghdr, sizeof (buf_nlmsghdr)), + is_dump ? ", in-dump" : "", nmp_object_to_string (obj, id_only ? NMP_OBJECT_TO_STRING_ID : NMP_OBJECT_TO_STRING_PUBLIC, NULL, 0)); @@ -3930,7 +3941,7 @@ event_valid_msg (NMPlatform *platform, struct nl_msg *msg, gboolean handle_event case RTM_NEWADDR: case RTM_NEWROUTE: case RTM_GETLINK: - cache_op = nmp_cache_update_netlink (cache, obj, &obj_old, &obj_new); + cache_op = nmp_cache_update_netlink (cache, obj, is_dump, &obj_old, &obj_new); if (cache_op != NMP_CACHE_OPS_UNCHANGED) { cache_on_change (platform, cache_op, obj_old, obj_new); cache_post (platform, msghdr, cache_op, obj, obj_old, obj_new); diff --git a/src/platform/nmp-object.c b/src/platform/nmp-object.c index 8723cec27a..15594e82b3 100644 --- a/src/platform/nmp-object.c +++ b/src/platform/nmp-object.c @@ -1792,10 +1792,11 @@ nmp_cache_lookup_link_full (const NMPCache *cache, /*****************************************************************************/ static void -_idxcache_update_box_move (NMPCache *cache, - NMPCacheIdType cache_id_type, - const NMPObject *obj_old, - const NMPObject *obj_new) +_idxcache_update_other_cache_ids (NMPCache *cache, + NMPCacheIdType cache_id_type, + const NMPObject *obj_old, + const NMPObject *obj_new, + gboolean is_dump) { const NMDedupMultiEntry *entry_new; const NMDedupMultiEntry *entry_old; @@ -1847,8 +1848,12 @@ _idxcache_update_box_move (NMPCache *cache, nm_dedup_multi_index_add_full (cache->multi_idx, idx_type, obj_new, - NM_DEDUP_MULTI_IDX_MODE_APPEND, - entry_order, + is_dump + ? NM_DEDUP_MULTI_IDX_MODE_APPEND_FORCE + : NM_DEDUP_MULTI_IDX_MODE_APPEND, + is_dump + ? NULL + : entry_order, entry_new ?: NM_DEDUP_MULTI_ENTRY_MISSING, entry_new ? entry_new->head : (entry_order ? entry_order->head : NULL), &entry_new, @@ -1874,6 +1879,7 @@ static void _idxcache_update (NMPCache *cache, const NMDedupMultiEntry *entry_old, NMPObject *obj_new, + gboolean is_dump, const NMDedupMultiEntry **out_entry_new) { const NMPClass *klass; @@ -1920,7 +1926,9 @@ _idxcache_update (NMPCache *cache, nm_dedup_multi_index_add_full (cache->multi_idx, idx_type_o, obj_new, - NM_DEDUP_MULTI_IDX_MODE_APPEND, + is_dump + ? NM_DEDUP_MULTI_IDX_MODE_APPEND_FORCE + : NM_DEDUP_MULTI_IDX_MODE_APPEND, NULL, entry_old ?: NM_DEDUP_MULTI_ENTRY_MISSING, NULL, @@ -1940,9 +1948,10 @@ _idxcache_update (NMPCache *cache, if (id_type == NMP_CACHE_ID_TYPE_OBJECT_TYPE) continue; - _idxcache_update_box_move (cache, id_type, - obj_old, - entry_new ? entry_new->obj : NULL); + _idxcache_update_other_cache_ids (cache, id_type, + obj_old, + entry_new ? entry_new->obj : NULL, + is_dump); } NM_SET_OUT (out_entry_new, entry_new); @@ -1974,7 +1983,7 @@ nmp_cache_remove (NMPCache *cache, * @obj_needle. */ return NMP_CACHE_OPS_UNCHANGED; } - _idxcache_update (cache, entry_old, NULL, NULL); + _idxcache_update (cache, entry_old, NULL, FALSE, NULL); return NMP_CACHE_OPS_REMOVED; } @@ -2015,7 +2024,7 @@ nmp_cache_remove_netlink (NMPCache *cache, if (!obj_old->_link.udev.device) { /* the update would make @obj_old invalid. Remove it. */ - _idxcache_update (cache, entry_old, NULL, NULL); + _idxcache_update (cache, entry_old, NULL, FALSE, NULL); NM_SET_OUT (out_obj_new, NULL); return NMP_CACHE_OPS_REMOVED; } @@ -2029,6 +2038,7 @@ nmp_cache_remove_netlink (NMPCache *cache, _idxcache_update (cache, entry_old, obj_new, + FALSE, &entry_new); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); return NMP_CACHE_OPS_UPDATED; @@ -2036,7 +2046,7 @@ nmp_cache_remove_netlink (NMPCache *cache, NM_SET_OUT (out_obj_old, nmp_object_ref (obj_old)); NM_SET_OUT (out_obj_new, NULL); - _idxcache_update (cache, entry_old, NULL, NULL); + _idxcache_update (cache, entry_old, NULL, FALSE, NULL); return NMP_CACHE_OPS_REMOVED; } @@ -2050,6 +2060,12 @@ nmp_cache_remove_netlink (NMPCache *cache, * calling nmp_cache_update_netlink() you hand @obj over to the cache. * Except, that the cache will increment the ref count as appropriate. You * must still unref the obj to release your part of the ownership. + * @is_dump: whether this update comes during a dump of object of the same kind. + * kernel dumps objects in a certain order, which matters especially for routes. + * Before a dump we mark all objects as dirty, and remove all untouched objects + * afterwards. Hence, during a dump, every update should move the object to the + * end of the list, to obtain the correct order. That means, to use NM_DEDUP_MULTI_IDX_MODE_APPEND_FORCE, + * instead of NM_DEDUP_MULTI_IDX_MODE_APPEND. * @out_obj_old: (allow-none): (out): return the object with same ID as @obj_hand_over, * that was in the cache before update. If an object is returned, the caller must * unref it afterwards. @@ -2064,6 +2080,7 @@ nmp_cache_remove_netlink (NMPCache *cache, NMPCacheOpsType nmp_cache_update_netlink (NMPCache *cache, NMPObject *obj_hand_over, + gboolean is_dump, const NMPObject **out_obj_old, const NMPObject **out_obj_new) { @@ -2102,6 +2119,7 @@ nmp_cache_update_netlink (NMPCache *cache, _idxcache_update (cache, entry_old, obj_hand_over, + is_dump, &entry_new); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); return NMP_CACHE_OPS_ADDED; @@ -2155,7 +2173,7 @@ nmp_cache_update_netlink (NMPCache *cache, if (!is_alive) { /* the update would make @obj_old invalid. Remove it. */ - _idxcache_update (cache, entry_old, NULL, NULL); + _idxcache_update (cache, entry_old, NULL, FALSE, NULL); NM_SET_OUT (out_obj_new, NULL); return NMP_CACHE_OPS_REMOVED; } @@ -2169,6 +2187,7 @@ nmp_cache_update_netlink (NMPCache *cache, _idxcache_update (cache, entry_old, obj_hand_over, + is_dump, &entry_new); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); return NMP_CACHE_OPS_UPDATED; @@ -2204,6 +2223,7 @@ nmp_cache_update_link_udev (NMPCache *cache, _idxcache_update (cache, NULL, obj_new, + FALSE, &entry_new); NM_SET_OUT (out_obj_old, NULL); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); @@ -2219,7 +2239,7 @@ nmp_cache_update_link_udev (NMPCache *cache, if (!udevice && !obj_old->_link.netlink.is_in_netlink) { /* the update would make @obj_old invalid. Remove it. */ - _idxcache_update (cache, entry_old, NULL, NULL); + _idxcache_update (cache, entry_old, NULL, FALSE, NULL); NM_SET_OUT (out_obj_new, NULL); return NMP_CACHE_OPS_REMOVED; } @@ -2234,6 +2254,7 @@ nmp_cache_update_link_udev (NMPCache *cache, _idxcache_update (cache, entry_old, obj_new, + FALSE, &entry_new); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); return NMP_CACHE_OPS_UPDATED; @@ -2274,6 +2295,7 @@ nmp_cache_update_link_master_connected (NMPCache *cache, _idxcache_update (cache, entry_old, obj_new, + FALSE, &entry_new); NM_SET_OUT (out_obj_new, nmp_object_ref (entry_new->obj)); return NMP_CACHE_OPS_UPDATED; diff --git a/src/platform/nmp-object.h b/src/platform/nmp-object.h index 14fe9821e8..2201cb7600 100644 --- a/src/platform/nmp-object.h +++ b/src/platform/nmp-object.h @@ -413,6 +413,11 @@ const NMPClass *nmp_class_from_type (NMPObjectType obj_type); static inline const NMPObject * nmp_object_ref (const NMPObject *obj) { + if (!obj) { + /* for convenience, allow NULL. */ + return NULL; + } + /* ref and unref accept const pointers. NMPObject is supposed to be shared * and kept immutable. Disallowing to take/retrun a reference to a const * NMPObject is cumbersome, because callers are precisely expected to @@ -599,7 +604,8 @@ NMPCacheOpsType nmp_cache_remove_netlink (NMPCache *cache, const NMPObject **out_obj_old, const NMPObject **out_obj_new); NMPCacheOpsType nmp_cache_update_netlink (NMPCache *cache, - NMPObject *obj, + NMPObject *obj_hand_over, + gboolean is_dump, const NMPObject **out_obj_old, const NMPObject **out_obj_new); NMPCacheOpsType nmp_cache_update_link_udev (NMPCache *cache, diff --git a/src/platform/tests/test-nmp-object.c b/src/platform/tests/test-nmp-object.c index 11fc59e24b..a02388d24d 100644 --- a/src/platform/tests/test-nmp-object.c +++ b/src/platform/tests/test-nmp-object.c @@ -235,7 +235,7 @@ _nmp_cache_update_netlink (NMPCache *cache, NMPObject *obj, const NMPObject **ou obj_new_expected->_link.udev.device = udev_device_ref (obj_prev->_link.udev.device); _nmp_object_fixup_link_udev_fields (&obj_new_expected, NULL, nmp_cache_use_udev_get (cache)); - ops_type = nmp_cache_update_netlink (cache, obj, &obj_old, &obj_new); + ops_type = nmp_cache_update_netlink (cache, obj, FALSE, &obj_old, &obj_new); ops_post_check (cache, ops_type, obj_old, obj_new, nmp_object_is_alive (obj_new_expected) ? obj_new_expected : NULL, expected_ops_type);