From 945c904f956580616f0d71b13c63f525c184060c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 12:33:14 +0100 Subject: 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)); -- cgit v1.2.1