From f47f9e3956ee971746f6d93253c3707a5d0ffa2f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 12:19:15 +0100 Subject: [PATCH 1/5] platform: let nmp_cache_lookup_link_full() prefer visible links In nmp_cache_lookup_link_full(), we may have multiple candidates that match. Continue searching, until we find a visible one. That way, visible results are preferred. Note that for links, nmp_object_is_visible() checks whether the link is visible in netlink (instead of only udev). --- src/platform/nmp-object.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/platform/nmp-object.c b/src/platform/nmp-object.c index 5e1ebb56e2..fd505282e2 100644 --- a/src/platform/nmp-object.c +++ b/src/platform/nmp-object.c @@ -1976,6 +1976,8 @@ nmp_cache_lookup_link_full (const NMPCache *cache, } else if (!ifname && !match_fn) return NULL; else { + const NMPObject *obj_best = NULL; + if (ifname) { if (strlen (ifname) >= IFNAMSIZ) return NULL; @@ -1987,16 +1989,21 @@ nmp_cache_lookup_link_full (const NMPCache *cache, nmp_cache_iter_for_each_link (&iter, head_entry, &link) { obj = NMP_OBJECT_UP_CAST (link); - if (visible_only && !nmp_object_is_visible (obj)) - continue; if (link_type != NM_LINK_TYPE_NONE && obj->link.type != link_type) continue; + if (visible_only && !nmp_object_is_visible (obj)) + continue; if (match_fn && !match_fn (obj, user_data)) continue; - return obj; + /* if there are multiple candidates, prefer the visible ones. */ + if ( visible_only + || nmp_object_is_visible (obj)) + return obj; + if (!obj_best) + obj_best = obj; } - return NULL; + return obj_best; } } From 1c7b747f8c5764a0690f514461623ff86e7ba943 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 12:20:09 +0100 Subject: [PATCH 2/5] platform: move assertion from nm_platform_link_get() to nm_platform_link_get_obj() We want to assert for valid input arguments, but we don't want multiple assertions for the same. Move the assertion from nm_platform_link_get() to nm_platform_link_get_obj(). That way, nm_platform_link_get_obj() also checks the input arguments. At the same time, nm_platform_link_get() gets simpler and still does the same amount of assertions. --- src/platform/nm-platform.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index f0a80cb8b5..6721f774db 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -754,6 +754,11 @@ nm_platform_link_get_obj (NMPlatform *self, { const NMPObject *obj_cache; + _CHECK_SELF (self, klass, NULL); + + if (ifindex <= 0) + return NULL; + obj_cache = nmp_cache_lookup_link (nm_platform_get_cache (self), ifindex); if ( !obj_cache || ( visible_only @@ -779,15 +784,7 @@ nm_platform_link_get_obj (NMPlatform *self, const NMPlatformLink * nm_platform_link_get (NMPlatform *self, int ifindex) { - const NMPObject *obj; - - _CHECK_SELF (self, klass, NULL); - - if (ifindex <= 0) - return NULL; - - obj = nm_platform_link_get_obj (self, ifindex, TRUE); - return NMP_OBJECT_CAST_LINK (obj); + return NMP_OBJECT_CAST_LINK (nm_platform_link_get_obj (self, ifindex, TRUE)); } /** From da39a0ada353f2891dd2dd98479506f92a756526 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 16:00:20 +0100 Subject: [PATCH 3/5] platform/tests: improve nmtstp_link_delete() for deleting links nm_platform_link_delete() will soon assert against positive ifindex argument. nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME)); will result in an assertion, if the link does not exist. Extend nmtstp_link_delete() to gracefully skip deleting the link so that it can be used in such situations. Also, rename nmtstp_link_del() to nmtstp_link_delete(), because it's closer to nm_platform_link_delete(). --- src/platform/tests/test-common.c | 14 ++++++++----- src/platform/tests/test-common.h | 9 ++++---- src/platform/tests/test-link.c | 36 ++++++++++++++++---------------- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 1cb2f51689..46f0a28b3a 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -1702,10 +1702,11 @@ nmtstp_link_get (NMPlatform *platform, /*****************************************************************************/ void -nmtstp_link_del (NMPlatform *platform, - gboolean external_command, - int ifindex, - const char *name) +nmtstp_link_delete (NMPlatform *platform, + gboolean external_command, + int ifindex, + const char *name, + gboolean require_exist) { gint64 end_time; const NMPlatformLink *pllink; @@ -1718,7 +1719,10 @@ nmtstp_link_del (NMPlatform *platform, pllink = nmtstp_link_get (platform, ifindex, name); - g_assert (pllink); + if (!pllink) { + g_assert (!require_exist); + return; + } name = name_copy = g_strdup (pllink->name); ifindex = pllink->ifindex; diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index 7e81baeac0..140866e86a 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -327,10 +327,11 @@ const NMPlatformLink *nmtstp_link_vxlan_add (NMPlatform *platform, const char *name, const NMPlatformLnkVxlan *lnk); -void nmtstp_link_del (NMPlatform *platform, - gboolean external_command, - int ifindex, - const char *name); +void nmtstp_link_delete (NMPlatform *platform, + gboolean external_command, + int ifindex, + const char *name, + gboolean require_exist); /*****************************************************************************/ diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index 8efd19447b..6177624c45 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -343,7 +343,7 @@ test_slave (int master, int type, SignalData *master_changed) ensure_no_signal (link_added); ensure_no_signal (link_changed); ensure_no_signal (link_removed); - nmtstp_link_del (NULL, -1, ifindex, NULL); + nmtstp_link_delete (NULL, -1, ifindex, NULL, TRUE); accept_signals (master_changed, 0, 1); accept_signals (link_changed, 0, 1); accept_signal (link_removed); @@ -439,7 +439,7 @@ test_software (NMLinkType link_type, const char *link_typename) free_signal (link_changed); /* Delete */ - nmtstp_link_del (NULL, -1, ifindex, DEVICE_NAME); + nmtstp_link_delete (NULL, -1, ifindex, DEVICE_NAME, TRUE); accept_signal (link_removed); /* Delete again */ @@ -450,7 +450,7 @@ test_software (NMLinkType link_type, const char *link_typename) if (link_type == NM_LINK_TYPE_VLAN) { SignalData *link_removed_parent = add_signal_ifindex (NM_PLATFORM_SIGNAL_LINK_CHANGED, NM_PLATFORM_SIGNAL_REMOVED, link_callback, vlan_parent); - nmtstp_link_del (NULL, -1, vlan_parent, NULL); + nmtstp_link_delete (NULL, -1, vlan_parent, NULL, TRUE); accept_signal (link_removed_parent); free_signal (link_removed_parent); } @@ -534,7 +534,7 @@ test_bridge_addr (void) g_assert_cmpint (plink->addr.len, ==, sizeof (addr)); g_assert (!memcmp (plink->addr.data, addr, sizeof (addr))); - nmtstp_link_del (NULL, -1, link.ifindex, link.name); + nmtstp_link_delete (NULL, -1, link.ifindex, link.name, TRUE); } /*****************************************************************************/ @@ -609,7 +609,7 @@ test_internal (void) accept_signal (link_changed); /* Delete device */ - nmtstp_link_del (NULL, -1, ifindex, DEVICE_NAME); + nmtstp_link_delete (NULL, -1, ifindex, DEVICE_NAME, TRUE); accept_signal (link_removed); /* Try to delete again */ @@ -897,7 +897,7 @@ test_software_detect (gconstpointer user_data) dummy = nmtstp_link_dummy_add (NM_PLATFORM_GET, FALSE, "dummy-tmp"); g_assert_cmpint (dummy->ifindex, ==, i); - nmtstp_link_del (NM_PLATFORM_GET, FALSE, dummy->ifindex, NULL); + nmtstp_link_delete (NM_PLATFORM_GET, FALSE, dummy->ifindex, NULL, TRUE); } if (!nmtstp_link_macvlan_add (NULL, ext, DEVICE_NAME, ifindex_parent, &lnk_macvlan)) @@ -1243,9 +1243,9 @@ test_software_detect (gconstpointer user_data) } } - nmtstp_link_del (NULL, -1, ifindex, DEVICE_NAME); + nmtstp_link_delete (NULL, -1, ifindex, DEVICE_NAME, TRUE); out_delete_parent: - nmtstp_link_del (NULL, -1, ifindex_parent, PARENT_NAME); + nmtstp_link_delete (NULL, -1, ifindex_parent, PARENT_NAME, TRUE); } static void @@ -1822,8 +1822,8 @@ test_vlan_set_xgress (void) _assert_vlan_flags (ifindex, NM_VLAN_FLAG_REORDER_HEADERS | NM_VLAN_FLAG_GVRP); } - nmtstp_link_del (NULL, -1, ifindex, DEVICE_NAME); - nmtstp_link_del (NULL, -1, ifindex_parent, PARENT_NAME); + nmtstp_link_delete (NULL, -1, ifindex, DEVICE_NAME, TRUE); + nmtstp_link_delete (NULL, -1, ifindex_parent, PARENT_NAME, TRUE); } /*****************************************************************************/ @@ -1879,7 +1879,7 @@ test_create_many_links_do (guint n_devices) if (EX == 2) nmtstp_run_command_check ("ip link delete %s", name); else - nmtstp_link_del (NULL, EX, g_array_index (ifindexes, int, i), name); + nmtstp_link_delete (NULL, EX, g_array_index (ifindexes, int, i), name, TRUE); } _LOGI (">>> process events after deleting devices..."); @@ -1965,7 +1965,7 @@ test_nl_bugs_veth (void) }); out: - nmtstp_link_del (NULL, -1, ifindex_veth0, IFACE_VETH0); + nmtstp_link_delete (NULL, -1, ifindex_veth0, IFACE_VETH0, TRUE); g_assert (!nmtstp_link_get (NM_PLATFORM_GET, ifindex_veth0, IFACE_VETH0)); g_assert (!nmtstp_link_get (NM_PLATFORM_GET, ifindex_veth1, IFACE_VETH1)); nmtstp_namespace_handle_release (ns_handle); @@ -2018,7 +2018,7 @@ again: } g_assert (!nmtstp_link_get (NM_PLATFORM_GET, ifindex_bond0, IFACE_BOND0)); - nmtstp_link_del (NULL, -1, ifindex_dummy0, IFACE_DUMMY0); + nmtstp_link_delete (NULL, -1, ifindex_dummy0, IFACE_DUMMY0, TRUE); } /*****************************************************************************/ @@ -2072,8 +2072,8 @@ again: goto again; } - nmtstp_link_del (NULL, -1, ifindex_bridge0, IFACE_BRIDGE0); - nmtstp_link_del (NULL, -1, ifindex_dummy0, IFACE_DUMMY0); + nmtstp_link_delete (NULL, -1, ifindex_bridge0, IFACE_BRIDGE0, TRUE); + nmtstp_link_delete (NULL, -1, ifindex_dummy0, IFACE_DUMMY0, TRUE); } /*****************************************************************************/ @@ -2637,8 +2637,8 @@ test_sysctl_rename (void) } nm_platform_process_events (PL); - nmtstp_link_del (PL, -1, ifindex[0], NULL); - nmtstp_link_del (PL, -1, ifindex[1], NULL); + nmtstp_link_delete (PL, -1, ifindex[0], NULL, TRUE); + nmtstp_link_delete (PL, -1, ifindex[1], NULL, TRUE); } /*****************************************************************************/ @@ -2717,7 +2717,7 @@ test_sysctl_netns_switch (void) else g_assert_cmpint (ifindex_tmp, ==, -1); - nmtstp_link_del (PL, FALSE, ifindex, NULL); + nmtstp_link_delete (PL, FALSE, ifindex, NULL, TRUE); } /*****************************************************************************/ From 945c904f956580616f0d71b13c63f525c184060c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 12:33:14 +0100 Subject: [PATCH 4/5] platform: assert against valid ifindex and remove duplicate assertions We want that all code paths assert strictly and gracefully. That means, if we have function nm_platform_link_get() which calls nm_platform_link_get_obj(), then we don't need to assert the same things twice. Don't have the calling function assert itself, if it is obvious that the first thing that it does, is calling a function that itself asserts the same conditions. On the other hand, it simply indicates a bug passing a non-positive ifindex to any of these platform functions. No longer let nm_platform_link_get_obj() handle negative ifindex gracefully. Instead, let it directly pass it to nmp_cache_lookup_link(), which eventually does a g_return_val_if_fail() check. This quite possible enables assertions on a lot of code paths. But note that g_return_val_if_fail() is graceful and does not lead to a crash (unless G_DEBUG=fatal-criticals is set for debugging). --- src/platform/nm-platform.c | 57 ++++--------------------------- src/platform/tests/test-cleanup.c | 3 +- src/platform/tests/test-common.c | 2 +- src/platform/tests/test-common.h | 4 +-- src/platform/tests/test-link.c | 8 ++--- 5 files changed, 14 insertions(+), 60 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 6721f774db..0e0acf3114 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -756,9 +756,6 @@ nm_platform_link_get_obj (NMPlatform *self, _CHECK_SELF (self, klass, NULL); - if (ifindex <= 0) - return NULL; - obj_cache = nmp_cache_lookup_link (nm_platform_get_cache (self), ifindex); if ( !obj_cache || ( visible_only @@ -1058,8 +1055,6 @@ nm_platform_link_get_name (NMPlatform *self, int ifindex) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, NULL); - pllink = nm_platform_link_get (self, ifindex); return pllink ? pllink->name : NULL; } @@ -1077,8 +1072,6 @@ nm_platform_link_get_type (NMPlatform *self, int ifindex) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, NM_LINK_TYPE_NONE); - pllink = nm_platform_link_get (self, ifindex); return pllink ? pllink->type : NM_LINK_TYPE_NONE; } @@ -1097,10 +1090,7 @@ nm_platform_link_get_type_name (NMPlatform *self, int ifindex) { const NMPObject *obj; - _CHECK_SELF (self, klass, NULL); - obj = nm_platform_link_get_obj (self, ifindex, TRUE); - if (!obj) return NULL; @@ -1221,11 +1211,6 @@ nm_platform_link_get_ifi_flags (NMPlatform *self, { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, -EINVAL); - - if (ifindex <= 0) - return -EINVAL; - /* include invisible links (only in netlink, not udev). */ pllink = NMP_OBJECT_CAST_LINK (nm_platform_link_get_obj (self, ifindex, FALSE)); if (!pllink) @@ -1264,8 +1249,6 @@ nm_platform_link_is_connected (NMPlatform *self, int ifindex) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, FALSE); - pllink = nm_platform_link_get (self, ifindex); return pllink ? pllink->connected : FALSE; } @@ -1331,10 +1314,6 @@ nm_platform_link_get_udev_device (NMPlatform *self, int ifindex) { const NMPObject *obj_cache; - _CHECK_SELF (self, klass, FALSE); - - g_return_val_if_fail (ifindex >= 0, NULL); - obj_cache = nm_platform_link_get_obj (self, ifindex, FALSE); return obj_cache ? obj_cache->_link.udev.device : NULL; } @@ -1355,10 +1334,6 @@ nm_platform_link_get_user_ipv6ll_enabled (NMPlatform *self, int ifindex) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, FALSE); - - g_return_val_if_fail (ifindex >= 0, FALSE); - pllink = nm_platform_link_get (self, ifindex); if (pllink && pllink->inet6_addr_gen_mode_inv) return _nm_platform_uint8_inv (pllink->inet6_addr_gen_mode_inv) == NM_IN6_ADDR_GEN_MODE_NONE; @@ -1424,12 +1399,7 @@ nm_platform_link_get_address (NMPlatform *self, int ifindex, size_t *length) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, NULL); - - g_return_val_if_fail (ifindex > 0, NULL); - pllink = nm_platform_link_get (self, ifindex); - if ( !pllink || pllink->addr.len <= 0) { NM_SET_OUT (length, 0); @@ -1649,8 +1619,6 @@ nm_platform_link_get_mtu (NMPlatform *self, int ifindex) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, 0); - pllink = nm_platform_link_get (self, ifindex); return pllink ? pllink->mtu : 0; } @@ -1833,10 +1801,6 @@ nm_platform_link_get_master (NMPlatform *self, int slave) { const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, 0); - - g_return_val_if_fail (slave >= 0, FALSE); - pllink = nm_platform_link_get (self, slave); return pllink ? pllink->master : 0; } @@ -1882,15 +1846,11 @@ nm_platform_link_get_lnk (NMPlatform *self, int ifindex, NMLinkType link_type, c { const NMPObject *obj; - _CHECK_SELF (self, klass, FALSE); - - NM_SET_OUT (out_link, NULL); - - g_return_val_if_fail (ifindex > 0, NULL); - obj = nm_platform_link_get_obj (self, ifindex, TRUE); - if (!obj) + if (!obj) { + NM_SET_OUT (out_link, NULL); return NULL; + } NM_SET_OUT (out_link, &obj->link); @@ -2215,12 +2175,11 @@ gboolean nm_platform_link_6lowpan_get_properties (NMPlatform *self, int ifindex, int *out_parent) { const NMPlatformLink *plink; - _CHECK_SELF (self, klass, FALSE); plink = nm_platform_link_get (self, ifindex); - if (!plink) return FALSE; + if (plink->type != NM_LINK_TYPE_6LOWPAN) return FALSE; @@ -2823,12 +2782,11 @@ nm_platform_link_veth_get_properties (NMPlatform *self, int ifindex, int *out_pe { const NMPlatformLink *plink; int peer_ifindex; - _CHECK_SELF (self, klass, FALSE); plink = nm_platform_link_get (self, ifindex); - if (!plink) return FALSE; + if (plink->type != NM_LINK_TYPE_VETH) return FALSE; @@ -2886,14 +2844,11 @@ nm_platform_link_tun_get_properties (NMPlatform *self, gint64 group; gint64 flags; - _CHECK_SELF (self, klass, FALSE); - - g_return_val_if_fail (ifindex > 0, FALSE); - /* we consider also invisible links (those that are not yet in udev). */ plobj = nm_platform_link_get_obj (self, ifindex, FALSE); if (!plobj) return FALSE; + if (NMP_OBJECT_CAST_LINK (plobj)->type != NM_LINK_TYPE_TUN) return FALSE; diff --git a/src/platform/tests/test-cleanup.c b/src/platform/tests/test-cleanup.c index 12d9181208..962ae16b63 100644 --- a/src/platform/tests/test-cleanup.c +++ b/src/platform/tests/test-cleanup.c @@ -128,8 +128,7 @@ _nmtstp_init_tests (int *argc, char ***argv) void _nmtstp_setup_tests (void) { - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME)); - g_assert (!nm_platform_link_get_by_ifname (NM_PLATFORM_GET, DEVICE_NAME)); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, DEVICE_NAME, FALSE); g_test_add_func ("/internal", test_cleanup_internal); /* FIXME: add external cleanup check */ diff --git a/src/platform/tests/test-common.c b/src/platform/tests/test-common.c index 46f0a28b3a..8aea3e7d95 100644 --- a/src/platform/tests/test-common.c +++ b/src/platform/tests/test-common.c @@ -2076,7 +2076,7 @@ main (int argc, char **argv) result = g_test_run (); - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME)); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, DEVICE_NAME, FALSE); g_object_unref (NM_PLATFORM_GET); return result; diff --git a/src/platform/tests/test-common.h b/src/platform/tests/test-common.h index 140866e86a..c49fc0b87e 100644 --- a/src/platform/tests/test-common.h +++ b/src/platform/tests/test-common.h @@ -350,8 +350,8 @@ _nmtstp_env1_wrapper_setup (const NmtstTestData *test_data) _LOGT ("TEST[%s]: setup", test_data->testpath); - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME)); - g_assert (!nm_platform_link_get_by_ifname (NM_PLATFORM_GET, DEVICE_NAME)); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, DEVICE_NAME, FALSE); + g_assert_cmpint (nm_platform_link_dummy_add (NM_PLATFORM_GET, DEVICE_NAME, NULL), ==, NM_PLATFORM_ERROR_SUCCESS); *p_ifindex = nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME); diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index 6177624c45..fd72a1aa70 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -443,7 +443,7 @@ test_software (NMLinkType link_type, const char *link_typename) accept_signal (link_removed); /* Delete again */ - g_assert (!nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME))); + g_assert (nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME) <= 0); g_assert (!nm_platform_link_delete (NM_PLATFORM_GET, ifindex)); /* VLAN: Delete parent */ @@ -2829,9 +2829,9 @@ _nmtstp_init_tests (int *argc, char ***argv) void _nmtstp_setup_tests (void) { - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME)); - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, SLAVE_NAME)); - nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, PARENT_NAME)); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, DEVICE_NAME, FALSE); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, SLAVE_NAME, FALSE); + nmtstp_link_delete (NM_PLATFORM_GET, -1, -1, PARENT_NAME, FALSE); g_assert (!nm_platform_link_get_by_ifname (NM_PLATFORM_GET, DEVICE_NAME)); g_assert (!nm_platform_link_get_by_ifname (NM_PLATFORM_GET, SLAVE_NAME)); g_assert (!nm_platform_link_get_by_ifname (NM_PLATFORM_GET, PARENT_NAME)); From f94142284d0e1e9e935468ad3580bda0baa47bf1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 12:35:51 +0100 Subject: [PATCH 5/5] platform: don't consult cache before invoking netlink operation Checking whether the link exists in the cache, before talking to kernel serves no purpose. - in all cases, the caller already has a good indication that the link in fact exists. That is, because the caller makes decisions on what to do, based on what platform told it earlier. Thus, the check usually succeeds anyway. - in the unexpected case it doesn't succeed, we - should not silently return without logging at least a message - we possibly still want to send the netlink message to kernel, just to have it fail. Note that the ifindex is indeed the identifier for the link, so there is no danger of accidentally killing the wrong link. Well, theoretically there is, because the kernel's ifindex counter can wrap or get reused when moving links between namespaces. But checking the cache would not protect against that anyway! Worst case, the cache would already have the impostor link and would not prevent from doing the wrong thing. After all, they do have the same identifier, so how would we know that this is in fact a different link? --- src/platform/nm-platform.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 0e0acf3114..e119707009 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -971,13 +971,9 @@ nm_platform_link_dummy_add (NMPlatform *self, gboolean nm_platform_link_delete (NMPlatform *self, int ifindex) { - const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, FALSE); - pllink = nm_platform_link_get (self, ifindex); - if (!pllink) - return FALSE; + g_return_val_if_fail (ifindex > 0, FALSE); _LOG3D ("link: deleting"); return klass->link_delete (self, ifindex); @@ -994,17 +990,11 @@ nm_platform_link_delete (NMPlatform *self, int ifindex) gboolean nm_platform_link_set_netns (NMPlatform *self, int ifindex, int netns_fd) { - const NMPlatformLink *pllink; - _CHECK_SELF (self, klass, FALSE); g_return_val_if_fail (ifindex > 0, FALSE); g_return_val_if_fail (netns_fd > 0, FALSE); - pllink = nm_platform_link_get (self, ifindex); - if (!pllink) - return FALSE; - _LOG3D ("link: move link to network namespace with fd %d", netns_fd); return klass->link_set_netns (self, ifindex, netns_fd); }