From 3f19c1fa524e4f41e0ba937276390bdd28874a67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 21:26:22 +0200 Subject: [PATCH 01/28] shared/tests: add nmtst_assert_variant_bytestring() helper --- shared/nm-utils/nm-test-utils.h | 37 +++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index 15362da00d..c270d2b941 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -2326,6 +2326,43 @@ _nmtst_variant_new_vardict (int dummy, ...) g_assert_cmpint (_l, ==, strlen (_str)); \ } G_STMT_END +#ifdef __NM_SHARED_UTILS_H__ +#define _nmtst_assert_variant_bytestring_cmp_str(_ptr, _ptr2, _len) \ + G_STMT_START { \ + if (memcmp (_ptr2, _ptr, _len) != 0) { \ + gs_free char *_x1 = NULL; \ + gs_free char *_x2 = NULL; \ + const char *_xx1; \ + const char *_xx2; \ + \ + _xx1 = nm_utils_buf_utf8safe_escape (_ptr, _len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL, &_x1); \ + _xx2 = nm_utils_buf_utf8safe_escape (_ptr2, _len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL, &_x2); \ + g_assert_cmpstr (_xx1, ==, _xx2); \ + g_assert_not_reached (); \ + } \ + } G_STMT_END +#else +#define _nmtst_assert_variant_bytestring_cmp_str(_ptr, _ptr2, _len) G_STMT_START { } G_STMT_END +#endif + +#define nmtst_assert_variant_bytestring(variant, ptr, len) \ + G_STMT_START { \ + GVariant *_variant = (variant); \ + gconstpointer _ptr = (ptr); \ + gconstpointer _ptr2; \ + gsize _len = (len); \ + gsize _len2; \ + \ + nmtst_assert_variant_is_of_type (_variant, G_VARIANT_TYPE_BYTESTRING); \ + _ptr2 = g_variant_get_fixed_array (_variant, &_len2, 1); \ + g_assert_cmpint (_len2, ==, _len); \ + if ( _len != 0 \ + && _ptr) { \ + _nmtst_assert_variant_bytestring_cmp_str(_ptr, _ptr2, _len); \ + g_assert_cmpmem (_ptr2, _len2, _ptr, _len); \ + } \ + } G_STMT_END + typedef enum { NMTST_VARIANT_EDITOR_CONNECTION, NMTST_VARIANT_EDITOR_SETTING, From fce73b1c82179443710fb4ba8b1516f4cbc4a331 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 16:36:28 +0200 Subject: [PATCH 02/28] shared: add nm_g_variant_builder_add_sv_*() helpers --- shared/nm-glib-aux/nm-shared-utils.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 7bc9e00214..2919865610 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1262,6 +1262,30 @@ nm_g_variant_is_of_type (GVariant *value, && g_variant_is_of_type (value, type); } +static inline void +nm_g_variant_builder_add_sv (GVariantBuilder *builder, const char *key, GVariant *val) +{ + g_variant_builder_add (builder, "{sv}", key, val); +} + +static inline void +nm_g_variant_builder_add_sv_bytearray (GVariantBuilder *builder, const char *key, const guint8 *arr, gsize len) +{ + g_variant_builder_add (builder, "{sv}", key, g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, arr, len, 1)); +} + +static inline void +nm_g_variant_builder_add_sv_uint32 (GVariantBuilder *builder, const char *key, guint32 val) +{ + nm_g_variant_builder_add_sv (builder, key, g_variant_new_uint32 (val)); +} + +static inline void +nm_g_variant_builder_add_sv_str (GVariantBuilder *builder, const char *key, const char *str) +{ + nm_g_variant_builder_add_sv (builder, key, g_variant_new_string (str)); +} + static inline void nm_g_source_destroy_and_unref (GSource *source) { From 39127758bd2b6446ba0e326e8defe5401b138eec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 13:43:47 +0200 Subject: [PATCH 03/28] shared: add nm_utils_ether_addr_equal(), nm_utils_ether_addr_cmp() --- shared/nm-glib-aux/nm-shared-utils.c | 6 ++++++ shared/nm-glib-aux/nm-shared-utils.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 7f7636c7b3..54b849ce57 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -13,6 +13,7 @@ #include #include #include +#include #include "nm-errno.h" #include "nm-str-buf.h" @@ -88,6 +89,11 @@ nm_ip_addr_set_from_untrusted (int addr_family, /*****************************************************************************/ +G_STATIC_ASSERT (ETH_ALEN == sizeof (struct ether_addr)); +G_STATIC_ASSERT (ETH_ALEN == 6); + +/*****************************************************************************/ + gsize nm_utils_get_next_realloc_size (gboolean true_realloc, gsize requested) { diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 2919865610..8b43b289a4 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -178,6 +178,24 @@ nm_ip4_addr_is_localhost (in_addr_t addr4) /*****************************************************************************/ +struct ether_addr; + +static inline int +nm_utils_ether_addr_cmp (const struct ether_addr *a1, const struct ether_addr *a2) +{ + nm_assert (a1); + nm_assert (a2); + return memcmp (a1, a2, 6 /*ETH_ALEN*/); +} + +static inline gboolean +nm_utils_ether_addr_equal (const struct ether_addr *a1, const struct ether_addr *a2) +{ + return nm_utils_ether_addr_cmp (a1, a2) == 0; +} + +/*****************************************************************************/ + #define NM_UTILS_INET_ADDRSTRLEN INET6_ADDRSTRLEN static inline const char * From 393bc8c8f66a852e04e2328675bdb0a26066cd0b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 16:55:03 +0200 Subject: [PATCH 04/28] shared: add nm_utils_buf_utf8safe_escape_cp() helper --- shared/nm-glib-aux/nm-shared-utils.c | 11 +++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 1 + 2 files changed, 12 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 54b849ce57..39e98a31df 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -2705,6 +2705,17 @@ nm_utils_buf_utf8safe_escape_bytes (GBytes *bytes, NMUtilsStrUtf8SafeFlags flags return nm_utils_buf_utf8safe_escape (p, l, flags, to_free); } +char * +nm_utils_buf_utf8safe_escape_cp (gconstpointer buf, gssize buflen, NMUtilsStrUtf8SafeFlags flags) +{ + const char *s_const; + char *s; + + s_const = nm_utils_buf_utf8safe_escape (buf, buflen, flags, &s); + nm_assert (!s || s == s_const); + return s ?: g_strdup (s_const); +} + /*****************************************************************************/ const char * diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 8b43b289a4..eeb8be5b7d 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1226,6 +1226,7 @@ typedef enum { } NMUtilsStrUtf8SafeFlags; const char *nm_utils_buf_utf8safe_escape (gconstpointer buf, gssize buflen, NMUtilsStrUtf8SafeFlags flags, char **to_free); +char *nm_utils_buf_utf8safe_escape_cp (gconstpointer buf, gssize buflen, NMUtilsStrUtf8SafeFlags flags); const char *nm_utils_buf_utf8safe_escape_bytes (GBytes *bytes, NMUtilsStrUtf8SafeFlags flags, char **to_free); gconstpointer nm_utils_buf_utf8safe_unescape (const char *str, NMUtilsStrUtf8SafeFlags flags, gsize *out_len, gpointer *to_free); From 4aa657086eeba93633ed01ee98e8365143633823 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 18:58:55 +0200 Subject: [PATCH 05/28] lldp/tests: assert for variant lookup in tests --- src/devices/tests/test-lldp.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index 8fc4ad67d6..25e87babf6 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -263,11 +263,11 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) g_assert_cmpuint (g_variant_n_children (attr), ==, 1); child = g_variant_get_child_value (attr, 0); g_assert (child); - g_variant_lookup (child, "interface-number", "u", &v_uint); + g_assert (g_variant_lookup (child, "interface-number", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 1001); - g_variant_lookup (child, "interface-number-subtype", "u", &v_uint); + g_assert (g_variant_lookup (child, "interface-number-subtype", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 2); - g_variant_lookup (child, "address-subtype", "u", &v_uint); + g_assert (g_variant_lookup (child, "address-subtype", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 6); nm_clear_g_variant (&child); nm_clear_g_variant (&attr); @@ -275,22 +275,22 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) /* IEEE 802.3 - Power Via MDI */ attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_IEEE_802_3_POWER_VIA_MDI, G_VARIANT_TYPE_VARDICT); g_assert (attr); - g_variant_lookup (attr, "mdi-power-support", "u", &v_uint); + g_assert (g_variant_lookup (attr, "mdi-power-support", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 7); - g_variant_lookup (attr, "pse-power-pair", "u", &v_uint); + g_assert (g_variant_lookup (attr, "pse-power-pair", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 1); - g_variant_lookup (attr, "power-class", "u", &v_uint); + g_assert (g_variant_lookup (attr, "power-class", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 0); nm_clear_g_variant (&attr); /* IEEE 802.3 - MAC/PHY Configuration/Status */ attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_IEEE_802_3_MAC_PHY_CONF, G_VARIANT_TYPE_VARDICT); g_assert (attr); - g_variant_lookup (attr, "autoneg", "u", &v_uint); + g_assert (g_variant_lookup (attr, "autoneg", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 3); - g_variant_lookup (attr, "pmd-autoneg-cap", "u", &v_uint); + g_assert (g_variant_lookup (attr, "pmd-autoneg-cap", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 0x6c00); - g_variant_lookup (attr, "operational-mau-type", "u", &v_uint); + g_assert (g_variant_lookup (attr, "operational-mau-type", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 16); nm_clear_g_variant (&attr); @@ -319,9 +319,9 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) g_assert_cmpuint (g_variant_n_children (attr), ==, 1); child = g_variant_get_child_value (attr, 0); g_assert (child); - g_variant_lookup (child, "ppvid", "u", &v_uint); + g_assert (g_variant_lookup (child, "ppvid", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 0); - g_variant_lookup (child, "flags", "u", &v_uint); + g_assert (g_variant_lookup (child, "flags", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 1); nm_clear_g_variant (&child); nm_clear_g_variant (&attr); @@ -339,9 +339,9 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) g_assert_cmpuint (g_variant_n_children (attr), ==, 1); child = g_variant_get_child_value (attr, 0); g_assert (child); - g_variant_lookup (child, "vid", "u", &v_uint); + g_assert (g_variant_lookup (child, "vid", "u", &v_uint)); g_assert_cmpint (v_uint, ==, 488); - g_variant_lookup (child, "name", "&s", &v_str); + g_assert (g_variant_lookup (child, "name", "&s", &v_str)); g_assert_cmpstr (v_str, ==, "v2-0488-03-0505"); nm_clear_g_variant (&child); nm_clear_g_variant (&attr); From 2d50370e077b1a94820bc4ca484d3a2cc8ccf07b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 23:11:48 +0200 Subject: [PATCH 06/28] lldp/tests: add test for parsing LLDP frame --- src/devices/nm-lldp-listener.c | 28 ++++++++++++++++++++++++++++ src/devices/nm-lldp-listener.h | 3 +++ src/devices/tests/test-lldp.c | 27 +++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 22f356d115..decc97e55d 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -815,6 +815,34 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) /*****************************************************************************/ +GVariant * +nmtst_lldp_parse_from_raw (const guint8 *raw_data, + gsize raw_len) +{ + nm_auto (sd_lldp_neighbor_unrefp) sd_lldp_neighbor *neighbor_sd = NULL; + nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL; + gs_free_error GError *error = NULL; + GVariant *variant; + int r; + + g_assert (raw_data); + g_assert (raw_len > 0); + + r = sd_lldp_neighbor_from_raw (&neighbor_sd, raw_data, raw_len); + g_assert (r >= 0); + + neigh = lldp_neighbor_new (neighbor_sd, &error); + g_assert (neigh); + g_assert (!error); + + variant = lldp_neighbor_to_variant (neigh); + g_assert (variant); + + return g_variant_ref (variant); +} + +/*****************************************************************************/ + static void data_changed_notify (NMLldpListener *self, NMLldpListenerPrivate *priv) { diff --git a/src/devices/nm-lldp-listener.h b/src/devices/nm-lldp-listener.h index 92e1d00ffd..adea91b8d9 100644 --- a/src/devices/nm-lldp-listener.h +++ b/src/devices/nm-lldp-listener.h @@ -25,4 +25,7 @@ gboolean nm_lldp_listener_is_running (NMLldpListener *self); GVariant *nm_lldp_listener_get_neighbors (NMLldpListener *self); +GVariant *nmtst_lldp_parse_from_raw (const guint8 *raw_data, + gsize raw_len); + #endif /* __NM_LLDP_LISTENER__ */ diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index 25e87babf6..54f5a413e2 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -119,7 +119,7 @@ TEST_RECV_FRAME_DEFINE (_test_recv_data0_frame0, ); static void -_test_recv_data0_check (GMainLoop *loop, NMLldpListener *listener) +_test_recv_data0_check_do (GMainLoop *loop, NMLldpListener *listener, const TestRecvFrame *frame) { GVariant *neighbors, *attr; gs_unref_variant GVariant *neighbor = NULL; @@ -151,6 +151,12 @@ _test_recv_data0_check (GMainLoop *loop, NMLldpListener *listener) nm_clear_g_variant (&attr); } +static void +_test_recv_data0_check (GMainLoop *loop, NMLldpListener *listener) +{ + _test_recv_data0_check_do (loop, listener, &_test_recv_data0_frame0); +} + TEST_RECV_DATA_DEFINE (_test_recv_data0, 1, _test_recv_data0_check, &_test_recv_data0_frame0); TEST_RECV_DATA_DEFINE (_test_recv_data0_twice, 1, _test_recv_data0_check, &_test_recv_data0_frame0, &_test_recv_data0_frame0); @@ -374,7 +380,7 @@ _test_recv_data2_ttl1_check (GMainLoop *loop, NMLldpListener *listener) gulong notify_id; GVariant *neighbors; - _test_recv_data0_check (loop, listener); + _test_recv_data0_check_do (loop, listener, &_test_recv_data2_frame0_ttl1); /* wait for signal. */ notify_id = g_signal_connect (listener, "notify::" NM_LLDP_LISTENER_NEIGHBORS, @@ -513,6 +519,19 @@ _test_recv_fixture_teardown (TestRecvFixture *fixture, gconstpointer user_data) /*****************************************************************************/ +static void +test_parse_frames (gconstpointer test_data) +{ + const TestRecvFrame *frame = test_data; + gs_unref_variant GVariant *v_neighbor = NULL; + gs_unref_variant GVariant *attr = NULL; + + v_neighbor = nmtst_lldp_parse_from_raw (frame->frame, frame->frame_len); + g_assert (v_neighbor); +} + +/*****************************************************************************/ + NMTstpSetupFunc const _nmtstp_setup_platform_func = nm_linux_platform_setup; void @@ -530,4 +549,8 @@ _nmtstp_setup_tests (void) _TEST_ADD_RECV ("/lldp/recv/0_twice", &_test_recv_data0_twice); _TEST_ADD_RECV ("/lldp/recv/1", &_test_recv_data1); _TEST_ADD_RECV ("/lldp/recv/2_ttl1", &_test_recv_data2_ttl1); + + g_test_add_data_func ("/lldp/parse-frames/0", &_test_recv_data0_frame0, test_parse_frames); + g_test_add_data_func ("/lldp/parse-frames/1", &_test_recv_data1_frame0, test_parse_frames); + g_test_add_data_func ("/lldp/parse-frames/2", &_test_recv_data2_frame0_ttl1, test_parse_frames); } From 2b52b003f8736cda149a37edfa138e9b7b68ee8c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 09:17:28 +0200 Subject: [PATCH 07/28] lldp/tests: assert for expected GVariant when parsing LLDP neighbor Currently the LLDP parsing code uses GVariantBuild, which possibly does not ensure a stable order of elements. Thus the test may not be stable. However, that will be fixed very soon. --- src/devices/tests/test-lldp.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index 54f5a413e2..b394b0e506 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -77,10 +77,12 @@ typedef struct { typedef struct { gsize frame_len; const uint8_t *frame; + const char *as_variant; } TestRecvFrame; -#define TEST_RECV_FRAME_DEFINE(name, ...) \ +#define TEST_RECV_FRAME_DEFINE(name, _as_variant, ...) \ static const guint8 _##name##_v[] = { __VA_ARGS__ }; \ static const TestRecvFrame name = { \ + .as_variant = _as_variant, \ .frame_len = sizeof (_##name##_v), \ .frame = _##name##_v, \ } @@ -102,6 +104,7 @@ typedef struct { #define TEST_IFNAME "nm-tap-test0" TEST_RECV_FRAME_DEFINE (_test_recv_data0_frame0, + "{'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", /* Ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ @@ -163,7 +166,7 @@ TEST_RECV_DATA_DEFINE (_test_recv_data0_twice, 1, _test_recv_data0_check, &_tes TEST_RECV_FRAME_DEFINE (_test_recv_data1_frame0, /* lldp.detailed.pcap from * https://wiki.wireshark.org/SampleCaptures#Link_Layer_Discovery_Protocol_.28LLDP.29 */ - + "{'chassis-id-type': , 'chassis-id': <'00:01:30:F9:AD:A0'>, 'port-id-type': , 'port-id': <'1/1'>, 'destination': <'nearest-bridge'>, 'port-description': <'Summit300-48-Port 1001'>, 'system-name': <'Summit300-48'>, 'system-description': <'Summit300-48 - Version 7.4e.1 (Build 5) by Release_Master 05/27/05 04:53:11'>, 'system-capabilities': , 'management-addresses': <[{'address-subtype': , 'object-id': <@ay []>, 'interface-number': , 'address': <[byte 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0]>, 'interface-number-subtype': }]>, 'ieee-802-1-pvid': , 'ieee-802-1-ppvid': , 'ieee-802-1-ppvid-flags': , 'ieee-802-1-ppvids': <[{'flags': , 'ppvid': }]>, 'ieee-802-1-vid': , 'ieee-802-1-vlan-name': <'v2-0488-03-0505'>, 'ieee-802-1-vlans': <[{'vid': , 'name': <'v2-0488-03-0505'>}]>, 'ieee-802-3-mac-phy-conf': <{'operational-mau-type': , 'autoneg': , 'pmd-autoneg-cap': }>, 'ieee-802-3-power-via-mdi': <{'pse-power-pair': , 'mdi-power-support': , 'power-class': }>, 'ieee-802-3-max-frame-size': }", /* ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, /* destination mac */ 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, /* source mac */ @@ -358,6 +361,7 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) TEST_RECV_DATA_DEFINE (_test_recv_data1, 1, _test_recv_data1_check, &_test_recv_data1_frame0); TEST_RECV_FRAME_DEFINE (_test_recv_data2_frame0_ttl1, + "{'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", /* Ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ @@ -525,9 +529,14 @@ test_parse_frames (gconstpointer test_data) const TestRecvFrame *frame = test_data; gs_unref_variant GVariant *v_neighbor = NULL; gs_unref_variant GVariant *attr = NULL; + gs_free char *as_variant = NULL; v_neighbor = nmtst_lldp_parse_from_raw (frame->frame, frame->frame_len); g_assert (v_neighbor); + + as_variant = g_variant_print (v_neighbor, TRUE); + g_assert (as_variant); + g_assert_cmpstr (frame->as_variant, ==, as_variant); } /*****************************************************************************/ From 8cd9b87c914e82e0e366b64b3b954761ba8135d9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 16:55:40 +0200 Subject: [PATCH 08/28] lldp: backslash escape untrusted chassis-id,port-id strings This is a serious issue, because this is not guaranteed to be UTF-8 data. Fixes: 07a9364d9c15 ('device: export list of LLDP neighbors through D-Bus') --- src/devices/nm-lldp-listener.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index decc97e55d..34250f82fe 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -522,7 +522,8 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME: case SD_LLDP_CHASSIS_SUBTYPE_LOCALLY_ASSIGNED: case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT: - neigh->chassis_id = g_strndup ((const char *) chassis_id, chassis_id_len); + neigh->chassis_id = nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) + ?: g_new0 (char, 1); break; case SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS: neigh->chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len); @@ -538,7 +539,8 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME: case SD_LLDP_PORT_SUBTYPE_LOCALLY_ASSIGNED: case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT: - neigh->port_id = strndup ((char *) port_id, port_id_len); + neigh->port_id = nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) + ?: g_new0 (char, 1); break; case SD_LLDP_PORT_SUBTYPE_MAC_ADDRESS: neigh->port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len); From 7c0d73d94a2824ef2cddedb100f2c1d47bbd5751 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 19:55:21 +0200 Subject: [PATCH 09/28] lldp: fix lldp_neighbor_equal() to compare lists of variants Fixes: 6c52d946fc8c ('lldp: add support for management address TLV') --- src/devices/nm-lldp-listener.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 34250f82fe..e1ca6b8d23 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -388,6 +388,19 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) if (!nm_streq (a->attrs[attr_id].v_string, b->attrs[attr_id].v_string)) return FALSE; break; + case LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS: { + NMCListElem *itr_a, *itr_b; + + if (c_list_length (&a->attrs[attr_id].v_variant_list) != c_list_length (&b->attrs[attr_id].v_variant_list)) + return FALSE; + itr_b = c_list_first_entry (&b->attrs[attr_id].v_variant_list, NMCListElem, lst); + c_list_for_each_entry (itr_a, &a->attrs[attr_id].v_variant_list, lst) { + if (!g_variant_equal (itr_a->data, itr_b->data)) + return FALSE; + itr_b = c_list_entry (&itr_b->lst, NMCListElem, lst); + } + break; + } default: nm_assert (a->attrs[attr_id].attr_type == LLDP_ATTR_TYPE_NONE); break; From 9b7c5ca12d1c181c5965c8c0856897a6e4eb5d37 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 19:55:21 +0200 Subject: [PATCH 10/28] lldp: fix lldp_neighbor_equal() to compare variants Fixes: 8200078ec5d5 ('lldp: support IEEE 802.3 TLVs') --- src/devices/nm-lldp-listener.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index e1ca6b8d23..5b6609111a 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -388,6 +388,10 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) if (!nm_streq (a->attrs[attr_id].v_string, b->attrs[attr_id].v_string)) return FALSE; break; + case LLDP_ATTR_TYPE_VARDICT: + if (!g_variant_equal (a->attrs[attr_id].v_variant, b->attrs[attr_id].v_variant)) + return FALSE; + break; case LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS: { NMCListElem *itr_a, *itr_b; From 2aab266dac344168f913b14691f2c16ca1c3b1da Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 17:20:01 +0200 Subject: [PATCH 11/28] lldp: rename LLDP_ATTR_TYPE_VARDICT to LLDP_ATTR_TYPE_VARIANT VARDICT sounds like it would be a variant of "a{sv}" type. But it can be really any GVariant. Rename to make the type more generic. This will be used to also hold a binary variant of type "ay". --- src/devices/nm-lldp-listener.c | 50 +++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 5b6609111a..1a10979078 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -27,8 +27,8 @@ typedef enum { LLDP_ATTR_TYPE_NONE, LLDP_ATTR_TYPE_UINT32, LLDP_ATTR_TYPE_STRING, - LLDP_ATTR_TYPE_VARDICT, - LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS, + LLDP_ATTR_TYPE_VARIANT, + LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, } LldpAttrType; typedef enum { @@ -182,16 +182,16 @@ NM_UTILS_LOOKUP_DEFINE (_lldp_attr_id_to_type, LldpAttrId, LldpAttrType, NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_NAME, LLDP_ATTR_TYPE_STRING), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_DESCRIPTION, LLDP_ATTR_TYPE_STRING), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_CAPABILITIES, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS), + NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PVID, LLDP_ATTR_TYPE_UINT32), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID, LLDP_ATTR_TYPE_UINT32), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVIDS, LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS), + NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVIDS, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VID, LLDP_ATTR_TYPE_UINT32), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, LLDP_ATTR_TYPE_STRING), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLANS, LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, LLDP_ATTR_TYPE_VARDICT), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, LLDP_ATTR_TYPE_VARDICT), + NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLANS, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), + NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, LLDP_ATTR_TYPE_VARIANT), + NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, LLDP_ATTR_TYPE_VARIANT), NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, LLDP_ATTR_TYPE_UINT32), NM_UTILS_LOOKUP_ITEM_IGNORE (_LLDP_ATTR_ID_COUNT), ); @@ -245,11 +245,11 @@ _lldp_attr_set_uint32 (LldpAttrData *pdata, LldpAttrId attr_id, guint32 v_uint32 } static void -_lldp_attr_set_vardict (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) +_lldp_attr_set_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) { nm_assert (pdata); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_VARDICT); + nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_VARIANT); pdata = &pdata[attr_id]; @@ -259,24 +259,24 @@ _lldp_attr_set_vardict (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *varia return; } - pdata->attr_type = LLDP_ATTR_TYPE_VARDICT; + pdata->attr_type = LLDP_ATTR_TYPE_VARIANT; pdata->v_variant = g_variant_ref_sink (variant); } static void -_lldp_attr_add_vardict (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) +_lldp_attr_add_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) { nm_assert (pdata); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS); + nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); g_variant_ref_sink (variant); pdata = &pdata[attr_id]; if (pdata->attr_type == LLDP_ATTR_TYPE_NONE) { c_list_init (&pdata->v_variant_list); - pdata->attr_type = LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS; + pdata->attr_type = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS; } else - nm_assert (pdata->attr_type == LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS); + nm_assert (pdata->attr_type == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); c_list_link_tail (&pdata->v_variant_list, &nm_c_list_elem_new_stale (variant)->lst); } @@ -340,10 +340,10 @@ lldp_neighbor_free (LldpNeighbor *neighbor) case LLDP_ATTR_TYPE_STRING: g_free (neighbor->attrs[attr_id].v_string); break; - case LLDP_ATTR_TYPE_VARDICT: + case LLDP_ATTR_TYPE_VARIANT: g_variant_unref (neighbor->attrs[attr_id].v_variant); break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS: + case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: nm_c_list_elem_free_all (&neighbor->attrs[attr_id].v_variant_list, (GDestroyNotify) g_variant_unref); break; @@ -388,11 +388,11 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) if (!nm_streq (a->attrs[attr_id].v_string, b->attrs[attr_id].v_string)) return FALSE; break; - case LLDP_ATTR_TYPE_VARDICT: + case LLDP_ATTR_TYPE_VARIANT: if (!g_variant_equal (a->attrs[attr_id].v_variant, b->attrs[attr_id].v_variant)) return FALSE; break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS: { + case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { NMCListElem *itr_a, *itr_b; if (c_list_length (&a->attrs[attr_id].v_variant_list) != c_list_length (&b->attrs[attr_id].v_variant_list)) @@ -601,7 +601,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_TYPE_MGMT_ADDRESS: variant = parse_management_address_tlv (data8, len); if (variant) { - _lldp_attr_add_vardict (neigh->attrs, + _lldp_attr_add_variant (neigh->attrs, LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, variant); } @@ -665,7 +665,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); - _lldp_attr_add_vardict (neigh->attrs, + _lldp_attr_add_variant (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVIDS, g_variant_dict_end (&dict)); break; @@ -691,7 +691,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "vid", "u", vid); g_variant_dict_insert (&dict, "name", "s", name); - _lldp_attr_add_vardict (neigh->attrs, + _lldp_attr_add_variant (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLANS, g_variant_dict_end (&dict)); @@ -718,7 +718,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); - _lldp_attr_set_vardict (neigh->attrs, + _lldp_attr_set_variant (neigh->attrs, LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, g_variant_dict_end (&dict)); break; @@ -731,7 +731,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); - _lldp_attr_set_vardict (neigh->attrs, + _lldp_attr_set_variant (neigh->attrs, LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, g_variant_dict_end (&dict)); break; @@ -805,12 +805,12 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) _lldp_attr_id_to_name (attr_id), g_variant_new_string (data->v_string)); break; - case LLDP_ATTR_TYPE_VARDICT: + case LLDP_ATTR_TYPE_VARIANT: g_variant_builder_add (&builder, "{sv}", _lldp_attr_id_to_name (attr_id), data->v_variant); break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARDICTS: { + case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { NMCListElem *elem; GVariantBuilder builder2; From 89795fbe3d59c0d7563b621211c58fcc0ffeff27 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 17:32:21 +0200 Subject: [PATCH 12/28] lldp: rework _lldp_attr_id_to_name() to lookup by ID NM_UTILS_LOOKUP_STR_DEFINE() is implemented via a switch statement. You'd expect that the compiler could optimize that to plain lookup, since all indexes are consecutive numbers. Anyway, my compiler doesn't, so use the array ourself. Note that NM_UTILS_LOOKUP_STR_DEFINE() is exactly intended to lookup by enum/integer, if the enum values are not consecutive numbers. It may not be best, when you can directly use the numbers as lookup index. --- src/devices/nm-lldp-listener.c | 98 ++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 40 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 1a10979078..37b4b41a95 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -154,47 +154,65 @@ ether_addr_equal (const struct ether_addr *a1, const struct ether_addr *a2) /*****************************************************************************/ -static -NM_UTILS_LOOKUP_STR_DEFINE (_lldp_attr_id_to_name, LldpAttrId, - NM_UTILS_LOOKUP_DEFAULT_WARN (NULL), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_PORT_DESCRIPTION, NM_LLDP_ATTR_PORT_DESCRIPTION), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_SYSTEM_NAME, NM_LLDP_ATTR_SYSTEM_NAME), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_SYSTEM_DESCRIPTION, NM_LLDP_ATTR_SYSTEM_DESCRIPTION), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_SYSTEM_CAPABILITIES, NM_LLDP_ATTR_SYSTEM_CAPABILITIES), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, NM_LLDP_ATTR_MANAGEMENT_ADDRESSES), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_PVID, NM_LLDP_ATTR_IEEE_802_1_PVID), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID, NM_LLDP_ATTR_IEEE_802_1_PPVID), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, NM_LLDP_ATTR_IEEE_802_1_PPVID_FLAGS), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVIDS, NM_LLDP_ATTR_IEEE_802_1_PPVIDS), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_VID, NM_LLDP_ATTR_IEEE_802_1_VID), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, NM_LLDP_ATTR_IEEE_802_1_VLAN_NAME), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLANS, NM_LLDP_ATTR_IEEE_802_1_VLANS), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, NM_LLDP_ATTR_IEEE_802_3_MAC_PHY_CONF), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, NM_LLDP_ATTR_IEEE_802_3_POWER_VIA_MDI), - NM_UTILS_LOOKUP_STR_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE,NM_LLDP_ATTR_IEEE_802_3_MAX_FRAME_SIZE), - NM_UTILS_LOOKUP_ITEM_IGNORE (_LLDP_ATTR_ID_COUNT), -); +static const char * +_lldp_attr_id_to_name (LldpAttrId attr_id) +{ + static const char *const names[_LLDP_ATTR_ID_COUNT] = { + [LLDP_ATTR_ID_PORT_DESCRIPTION] = NM_LLDP_ATTR_PORT_DESCRIPTION, + [LLDP_ATTR_ID_SYSTEM_NAME] = NM_LLDP_ATTR_SYSTEM_NAME, + [LLDP_ATTR_ID_SYSTEM_DESCRIPTION] = NM_LLDP_ATTR_SYSTEM_DESCRIPTION, + [LLDP_ATTR_ID_SYSTEM_CAPABILITIES] = NM_LLDP_ATTR_SYSTEM_CAPABILITIES, + [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = NM_LLDP_ATTR_MANAGEMENT_ADDRESSES, + [LLDP_ATTR_ID_IEEE_802_1_PVID] = NM_LLDP_ATTR_IEEE_802_1_PVID, + [LLDP_ATTR_ID_IEEE_802_1_PPVID] = NM_LLDP_ATTR_IEEE_802_1_PPVID, + [LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS] = NM_LLDP_ATTR_IEEE_802_1_PPVID_FLAGS, + [LLDP_ATTR_ID_IEEE_802_1_PPVIDS] = NM_LLDP_ATTR_IEEE_802_1_PPVIDS, + [LLDP_ATTR_ID_IEEE_802_1_VID] = NM_LLDP_ATTR_IEEE_802_1_VID, + [LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME] = NM_LLDP_ATTR_IEEE_802_1_VLAN_NAME, + [LLDP_ATTR_ID_IEEE_802_1_VLANS] = NM_LLDP_ATTR_IEEE_802_1_VLANS, + [LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF] = NM_LLDP_ATTR_IEEE_802_3_MAC_PHY_CONF, + [LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI] = NM_LLDP_ATTR_IEEE_802_3_POWER_VIA_MDI, + [LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE] = NM_LLDP_ATTR_IEEE_802_3_MAX_FRAME_SIZE, + }; -static -NM_UTILS_LOOKUP_DEFINE (_lldp_attr_id_to_type, LldpAttrId, LldpAttrType, - NM_UTILS_LOOKUP_DEFAULT_WARN (LLDP_ATTR_TYPE_NONE), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_PORT_DESCRIPTION, LLDP_ATTR_TYPE_STRING), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_NAME, LLDP_ATTR_TYPE_STRING), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_DESCRIPTION, LLDP_ATTR_TYPE_STRING), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_SYSTEM_CAPABILITIES, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PVID, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_PPVIDS, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VID, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, LLDP_ATTR_TYPE_STRING), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_1_VLANS, LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, LLDP_ATTR_TYPE_VARIANT), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, LLDP_ATTR_TYPE_VARIANT), - NM_UTILS_LOOKUP_ITEM (LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, LLDP_ATTR_TYPE_UINT32), - NM_UTILS_LOOKUP_ITEM_IGNORE (_LLDP_ATTR_ID_COUNT), -); + if (NM_MORE_ASSERT_ONCE (5)) + nm_assert (nm_utils_strv_find_first ((char **) names, G_N_ELEMENTS (names), NULL) == -1); + nm_assert (_NM_INT_NOT_NEGATIVE (attr_id)); + nm_assert (attr_id < G_N_ELEMENTS (names)); + return names[attr_id]; +}; + +static LldpAttrType +_lldp_attr_id_to_type (LldpAttrId attr_id) +{ + static const LldpAttrType types[_LLDP_ATTR_ID_COUNT] = { + [LLDP_ATTR_ID_PORT_DESCRIPTION] = LLDP_ATTR_TYPE_STRING, + [LLDP_ATTR_ID_SYSTEM_NAME] = LLDP_ATTR_TYPE_STRING, + [LLDP_ATTR_ID_SYSTEM_DESCRIPTION] = LLDP_ATTR_TYPE_STRING, + [LLDP_ATTR_ID_SYSTEM_CAPABILITIES] = LLDP_ATTR_TYPE_UINT32, + [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, + [LLDP_ATTR_ID_IEEE_802_1_PVID] = LLDP_ATTR_TYPE_UINT32, + [LLDP_ATTR_ID_IEEE_802_1_PPVID] = LLDP_ATTR_TYPE_UINT32, + [LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS] = LLDP_ATTR_TYPE_UINT32, + [LLDP_ATTR_ID_IEEE_802_1_PPVIDS] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, + [LLDP_ATTR_ID_IEEE_802_1_VID] = LLDP_ATTR_TYPE_UINT32, + [LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME] = LLDP_ATTR_TYPE_STRING, + [LLDP_ATTR_ID_IEEE_802_1_VLANS] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, + [LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF] = LLDP_ATTR_TYPE_VARIANT, + [LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI] = LLDP_ATTR_TYPE_VARIANT, + [LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE] = LLDP_ATTR_TYPE_UINT32, + }; + + if (NM_MORE_ASSERT_ONCE (5)) { + int i; + + for (i = 0; i < G_N_ELEMENTS (types); i++) + nm_assert (types[i] != 0); + } + nm_assert (_NM_INT_NOT_NEGATIVE (attr_id)); + nm_assert (attr_id < G_N_ELEMENTS (types)); + return types[attr_id]; +} static void _lldp_attr_set_str (LldpAttrData *pdata, LldpAttrId attr_id, const char *v_string) From 597e71765990ee4a25a058ecd008ac4eed83b562 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 18:05:53 +0200 Subject: [PATCH 13/28] lldp: track LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS as array instead of CList Allocating and growing the buffer with realloc isn't really complicated. Do that instead of using a CList. Also, if there is only one element, then we can track it in-place. --- src/devices/nm-lldp-listener.c | 102 ++++++++++++++++++++++----------- 1 file changed, 67 insertions(+), 35 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 37b4b41a95..9f4fd8a69f 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -58,7 +58,14 @@ typedef struct { guint32 v_uint32; char *v_string; GVariant *v_variant; - CList v_variant_list; + struct { + union { + GVariant **arr; + GVariant *arr1; + }; + guint len; + guint alloc; + } v_variant_list; }; } LldpAttrData; @@ -288,15 +295,33 @@ _lldp_attr_add_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *varia nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); g_variant_ref_sink (variant); + pdata = &pdata[attr_id]; if (pdata->attr_type == LLDP_ATTR_TYPE_NONE) { - c_list_init (&pdata->v_variant_list); pdata->attr_type = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS; - } else - nm_assert (pdata->attr_type == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); + pdata->v_variant_list.arr1 = variant; + pdata->v_variant_list.len = 1; + pdata->v_variant_list.alloc = 1; + return; + } - c_list_link_tail (&pdata->v_variant_list, &nm_c_list_elem_new_stale (variant)->lst); + nm_assert (pdata->attr_type == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); + + if (pdata->v_variant_list.len == pdata->v_variant_list.alloc) { + if (pdata->v_variant_list.alloc == 1) { + GVariant *arr1 = pdata->v_variant_list.arr1; + + pdata->v_variant_list.alloc = 4; + pdata->v_variant_list.arr = g_new (GVariant *, 4); + pdata->v_variant_list.arr[0] = arr1; + } else { + pdata->v_variant_list.alloc *= 2u; + pdata->v_variant_list.arr = g_realloc (pdata->v_variant_list.arr, + pdata->v_variant_list.alloc); + } + } + pdata->v_variant_list.arr[pdata->v_variant_list.len++] = g_variant_ref_sink (variant); } /*****************************************************************************/ @@ -344,7 +369,7 @@ static void lldp_neighbor_free (LldpNeighbor *neighbor) { LldpAttrId attr_id; - LldpAttrType attr_type; + guint i; if (!neighbor) return; @@ -352,18 +377,23 @@ lldp_neighbor_free (LldpNeighbor *neighbor) g_free (neighbor->chassis_id); g_free (neighbor->port_id); for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - attr_type = neighbor->attrs[attr_id].attr_type; + LldpAttrData *pdata = &neighbor->attrs[attr_id]; - switch (attr_type) { + switch (pdata->attr_type) { case LLDP_ATTR_TYPE_STRING: - g_free (neighbor->attrs[attr_id].v_string); + g_free (pdata->v_string); break; case LLDP_ATTR_TYPE_VARIANT: - g_variant_unref (neighbor->attrs[attr_id].v_variant); + g_variant_unref (pdata->v_variant); break; case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: - nm_c_list_elem_free_all (&neighbor->attrs[attr_id].v_variant_list, - (GDestroyNotify) g_variant_unref); + if (pdata->v_variant_list.alloc == 1) + g_variant_unref (pdata->v_variant_list.arr1); + else { + for (i = 0; i < pdata->v_variant_list.len; i++) + g_variant_unref (pdata->v_variant_list.arr[i]); + g_free (pdata->v_variant_list.arr); + } break; default: ; @@ -383,6 +413,7 @@ static gboolean lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) { LldpAttrId attr_id; + guint i; nm_assert (a); nm_assert (b); @@ -410,19 +441,20 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) if (!g_variant_equal (a->attrs[attr_id].v_variant, b->attrs[attr_id].v_variant)) return FALSE; break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { - NMCListElem *itr_a, *itr_b; - - if (c_list_length (&a->attrs[attr_id].v_variant_list) != c_list_length (&b->attrs[attr_id].v_variant_list)) + case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: + if (a->attrs[attr_id].v_variant_list.len != b->attrs[attr_id].v_variant_list.len) return FALSE; - itr_b = c_list_first_entry (&b->attrs[attr_id].v_variant_list, NMCListElem, lst); - c_list_for_each_entry (itr_a, &a->attrs[attr_id].v_variant_list, lst) { - if (!g_variant_equal (itr_a->data, itr_b->data)) + if (a->attrs[attr_id].v_variant_list.alloc == 1) { + nm_assert (b->attrs[attr_id].v_variant_list.alloc == 1); + if (!g_variant_equal (a->attrs[attr_id].v_variant_list.arr1, b->attrs[attr_id].v_variant_list.arr1)) return FALSE; - itr_b = c_list_entry (&itr_b->lst, NMCListElem, lst); + } else { + for (i = 0; i < a->attrs[attr_id].v_variant_list.len; i++) { + if (!g_variant_equal (a->attrs[attr_id].v_variant_list.arr[i], b->attrs[attr_id].v_variant_list.arr[i])) + return FALSE; + } } break; - } default: nm_assert (a->attrs[attr_id].attr_type == LLDP_ATTR_TYPE_NONE); break; @@ -809,37 +841,37 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) } for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - const LldpAttrData *data = &neigh->attrs[attr_id]; + const LldpAttrData *pdata = &neigh->attrs[attr_id]; - nm_assert (NM_IN_SET (data->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); - switch (data->attr_type) { + nm_assert (NM_IN_SET (pdata->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); + switch (pdata->attr_type) { case LLDP_ATTR_TYPE_UINT32: g_variant_builder_add (&builder, "{sv}", _lldp_attr_id_to_name (attr_id), - g_variant_new_uint32 (data->v_uint32)); + g_variant_new_uint32 (pdata->v_uint32)); break; case LLDP_ATTR_TYPE_STRING: g_variant_builder_add (&builder, "{sv}", _lldp_attr_id_to_name (attr_id), - g_variant_new_string (data->v_string)); + g_variant_new_string (pdata->v_string)); break; case LLDP_ATTR_TYPE_VARIANT: g_variant_builder_add (&builder, "{sv}", _lldp_attr_id_to_name (attr_id), - data->v_variant); + pdata->v_variant); break; case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { - NMCListElem *elem; - GVariantBuilder builder2; - - g_variant_builder_init (&builder2, G_VARIANT_TYPE ("aa{sv}")); - - c_list_for_each_entry (elem, &data->v_variant_list, lst) - g_variant_builder_add_value (&builder2, elem->data); + GVariant *const*variants; + if (pdata->v_variant_list.alloc == 1) + variants = &pdata->v_variant_list.arr1; + else + variants = pdata->v_variant_list.arr; g_variant_builder_add (&builder, "{sv}", _lldp_attr_id_to_name (attr_id), - g_variant_builder_end (&builder2)); + g_variant_new_array (G_VARIANT_TYPE ("a{sv}"), + variants, + pdata->v_variant_list.len)); break; } case LLDP_ATTR_TYPE_NONE: From e189d65ab64cef952802d00596dd1b4defc78bdc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 5 Jun 2020 19:54:05 +0200 Subject: [PATCH 14/28] lldp: expose raw LLDP message on D-Bus Also, track sd_lldp_neighbor instance directly. sd_lldp_neighbor is a perfectly reasonable container for keeping track of the LLDP neighbor information. Just keep a reference to it, and don't clone the data. Especially since the LLDP library keeps a reference to this instance as well. Also, to compare whether two neighbors are the same, it is sufficient to only consider the raw data. Everything else depends on these fields anyway. This is only possible and useful becuase sd_lldp_neighbor is of course immutable. It wouldn't make sense otherwise, but it also would be bad design to mutate the sd_lldp_neighbor instances. This couples our code slightly more to the systemd code, which we usually try to avoid. But when we move away in the future from systemd LLDP library, we anyway need to rework this heavily (and then too, we wouldn't want to clone the data, when we could just share the reference). --- libnm-core/nm-dbus-interface.h | 1 + src/devices/nm-lldp-listener.c | 97 ++++++++++++++++++---------------- src/devices/tests/test-lldp.c | 22 ++++++-- 3 files changed, 70 insertions(+), 50 deletions(-) diff --git a/libnm-core/nm-dbus-interface.h b/libnm-core/nm-dbus-interface.h index 1e656333c6..f902ee9ef1 100644 --- a/libnm-core/nm-dbus-interface.h +++ b/libnm-core/nm-dbus-interface.h @@ -860,6 +860,7 @@ typedef enum /*< flags >*/ { #undef NM_AVAILABLE_IN_1_8 #endif +#define NM_LLDP_ATTR_RAW "raw" #define NM_LLDP_ATTR_DESTINATION "destination" #define NM_LLDP_ATTR_CHASSIS_ID_TYPE "chassis-id-type" #define NM_LLDP_ATTR_CHASSIS_ID "chassis-id" diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 9f4fd8a69f..3623826826 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -108,6 +108,8 @@ typedef struct { char *chassis_id; char *port_id; + sd_lldp_neighbor *neighbor_sd; + LldpAttrData attrs[_LLDP_ATTR_ID_COUNT]; struct ether_addr destination_address; @@ -326,6 +328,27 @@ _lldp_attr_add_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *varia /*****************************************************************************/ +static void +lldp_neighbor_get_raw (LldpNeighbor *neigh, + const guint8 **out_raw_data, + gsize *out_raw_len) +{ + gconstpointer raw_data; + gsize raw_len; + int r; + + nm_assert (neigh); + + r = sd_lldp_neighbor_get_raw (neigh->neighbor_sd, &raw_data, &raw_len); + + nm_assert (r >= 0); + nm_assert (raw_data); + nm_assert (raw_len > 0); + + *out_raw_data = raw_data; + *out_raw_len = raw_len; +} + static guint lldp_neighbor_id_hash (gconstpointer ptr) { @@ -400,6 +423,7 @@ lldp_neighbor_free (LldpNeighbor *neighbor) } } nm_g_variant_unref (neighbor->variant); + sd_lldp_neighbor_unref (neighbor->neighbor_sd); nm_g_slice_free (neighbor); } @@ -412,56 +436,30 @@ lldp_neighbor_freep (LldpNeighbor **ptr) static gboolean lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) { - LldpAttrId attr_id; - guint i; + const guint8 *raw_data_a; + const guint8 *raw_data_b; + gsize raw_len_a; + gsize raw_len_b; + gboolean equal; - nm_assert (a); - nm_assert (b); + if (a->neighbor_sd == b->neighbor_sd) + return TRUE; - if ( a->chassis_id_type != b->chassis_id_type - || a->port_id_type != b->port_id_type - || ether_addr_equal (&a->destination_address, &b->destination_address) - || !nm_streq0 (a->chassis_id, b->chassis_id) - || !nm_streq0 (a->port_id, b->port_id)) + lldp_neighbor_get_raw (a, &raw_data_a, &raw_len_a); + lldp_neighbor_get_raw (b, &raw_data_b, &raw_len_b); + + if (raw_len_a != raw_len_b) return FALSE; - for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - if (a->attrs[attr_id].attr_type != b->attrs[attr_id].attr_type) - return FALSE; - switch (a->attrs[attr_id].attr_type) { - case LLDP_ATTR_TYPE_UINT32: - if (a->attrs[attr_id].v_uint32 != b->attrs[attr_id].v_uint32) - return FALSE; - break; - case LLDP_ATTR_TYPE_STRING: - if (!nm_streq (a->attrs[attr_id].v_string, b->attrs[attr_id].v_string)) - return FALSE; - break; - case LLDP_ATTR_TYPE_VARIANT: - if (!g_variant_equal (a->attrs[attr_id].v_variant, b->attrs[attr_id].v_variant)) - return FALSE; - break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: - if (a->attrs[attr_id].v_variant_list.len != b->attrs[attr_id].v_variant_list.len) - return FALSE; - if (a->attrs[attr_id].v_variant_list.alloc == 1) { - nm_assert (b->attrs[attr_id].v_variant_list.alloc == 1); - if (!g_variant_equal (a->attrs[attr_id].v_variant_list.arr1, b->attrs[attr_id].v_variant_list.arr1)) - return FALSE; - } else { - for (i = 0; i < a->attrs[attr_id].v_variant_list.len; i++) { - if (!g_variant_equal (a->attrs[attr_id].v_variant_list.arr[i], b->attrs[attr_id].v_variant_list.arr[i])) - return FALSE; - } - } - break; - default: - nm_assert (a->attrs[attr_id].attr_type == LLDP_ATTR_TYPE_NONE); - break; - } - } + equal = (memcmp (raw_data_a, raw_data_b, raw_len_a) == 0); - return TRUE; + nm_assert ( !equal + || ( a->chassis_id_type == b->chassis_id_type + && a->port_id_type == b->port_id_type + && ether_addr_equal (&a->destination_address, &b->destination_address) + && nm_streq0 (a->chassis_id, b->chassis_id) + && nm_streq0 (a->port_id, b->port_id))); + return equal; } static GVariant * @@ -575,6 +573,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) *neigh = (LldpNeighbor) { .chassis_id_type = chassis_id_type, .port_id_type = port_id_type, + .neighbor_sd = sd_lldp_neighbor_ref (neighbor_sd), }; r = sd_lldp_neighbor_get_destination_address (neighbor_sd, &neigh->destination_address); @@ -807,12 +806,20 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) GVariantBuilder builder; const char *dest_str; LldpAttrId attr_id; + const guint8 *raw_data; + gsize raw_len; if (neigh->variant) return neigh->variant; + lldp_neighbor_get_raw (neigh, &raw_data, &raw_len); + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_bytearray (&builder, + NM_LLDP_ATTR_RAW, + raw_data, + raw_len); g_variant_builder_add (&builder, "{sv}", NM_LLDP_ATTR_CHASSIS_ID_TYPE, g_variant_new_uint32 (neigh->chassis_id_type)); diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index b394b0e506..4d563c6040 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -104,7 +104,7 @@ typedef struct { #define TEST_IFNAME "nm-tap-test0" TEST_RECV_FRAME_DEFINE (_test_recv_data0_frame0, - "{'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", + "{'raw': <[byte 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x88, 0xcc, 0x02, 0x07, 0x04, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x04, 0x04, 0x05, 0x31, 0x2f, 0x33, 0x06, 0x02, 0x00, 0x78, 0x08, 0x04, 0x50, 0x6f, 0x72, 0x74, 0x0a, 0x03, 0x53, 0x59, 0x53, 0x0c, 0x04, 0x66, 0x6f, 0x6f, 0x00, 0x00, 0x00]>, 'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", /* Ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ @@ -135,7 +135,11 @@ _test_recv_data0_check_do (GMainLoop *loop, NMLldpListener *listener, const Test SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS, "00:01:02:03:04:05", SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME, "1/3"); g_assert (neighbor); - g_assert_cmpint (g_variant_n_children (neighbor), ==, 4 + 4); + g_assert_cmpint (g_variant_n_children (neighbor), ==, 1 + 4 + 4); + + attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_RAW, G_VARIANT_TYPE_BYTESTRING); + nmtst_assert_variant_bytestring (attr, frame->frame, frame->frame_len); + nm_clear_g_variant (&attr); attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_PORT_DESCRIPTION, G_VARIANT_TYPE_STRING); nmtst_assert_variant_string (attr, "Port"); @@ -166,7 +170,7 @@ TEST_RECV_DATA_DEFINE (_test_recv_data0_twice, 1, _test_recv_data0_check, &_tes TEST_RECV_FRAME_DEFINE (_test_recv_data1_frame0, /* lldp.detailed.pcap from * https://wiki.wireshark.org/SampleCaptures#Link_Layer_Discovery_Protocol_.28LLDP.29 */ - "{'chassis-id-type': , 'chassis-id': <'00:01:30:F9:AD:A0'>, 'port-id-type': , 'port-id': <'1/1'>, 'destination': <'nearest-bridge'>, 'port-description': <'Summit300-48-Port 1001'>, 'system-name': <'Summit300-48'>, 'system-description': <'Summit300-48 - Version 7.4e.1 (Build 5) by Release_Master 05/27/05 04:53:11'>, 'system-capabilities': , 'management-addresses': <[{'address-subtype': , 'object-id': <@ay []>, 'interface-number': , 'address': <[byte 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0]>, 'interface-number-subtype': }]>, 'ieee-802-1-pvid': , 'ieee-802-1-ppvid': , 'ieee-802-1-ppvid-flags': , 'ieee-802-1-ppvids': <[{'flags': , 'ppvid': }]>, 'ieee-802-1-vid': , 'ieee-802-1-vlan-name': <'v2-0488-03-0505'>, 'ieee-802-1-vlans': <[{'vid': , 'name': <'v2-0488-03-0505'>}]>, 'ieee-802-3-mac-phy-conf': <{'operational-mau-type': , 'autoneg': , 'pmd-autoneg-cap': }>, 'ieee-802-3-power-via-mdi': <{'pse-power-pair': , 'mdi-power-support': , 'power-class': }>, 'ieee-802-3-max-frame-size': }", + "{'raw': <[byte 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x88, 0xcc, 0x02, 0x07, 0x04, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x04, 0x04, 0x05, 0x31, 0x2f, 0x31, 0x06, 0x02, 0x00, 0x78, 0x08, 0x17, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x2d, 0x50, 0x6f, 0x72, 0x74, 0x20, 0x31, 0x30, 0x30, 0x31, 0x00, 0x0a, 0x0d, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x00, 0x0c, 0x4c, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x20, 0x2d, 0x20, 0x56, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x20, 0x37, 0x2e, 0x34, 0x65, 0x2e, 0x31, 0x20, 0x28, 0x42, 0x75, 0x69, 0x6c, 0x64, 0x20, 0x35, 0x29, 0x20, 0x62, 0x79, 0x20, 0x52, 0x65, 0x6c, 0x65, 0x61, 0x73, 0x65, 0x5f, 0x4d, 0x61, 0x73, 0x74, 0x65, 0x72, 0x20, 0x30, 0x35, 0x2f, 0x32, 0x37, 0x2f, 0x30, 0x35, 0x20, 0x30, 0x34, 0x3a, 0x35, 0x33, 0x3a, 0x31, 0x31, 0x00, 0x0e, 0x04, 0x00, 0x14, 0x00, 0x14, 0x10, 0x0e, 0x07, 0x06, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x02, 0x00, 0x00, 0x03, 0xe9, 0x00, 0xfe, 0x07, 0x00, 0x12, 0x0f, 0x02, 0x07, 0x01, 0x00, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x01, 0x03, 0x6c, 0x00, 0x00, 0x10, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x03, 0x01, 0x00, 0x00, 0x00, 0x00, 0xfe, 0x06, 0x00, 0x12, 0x0f, 0x04, 0x05, 0xf2, 0xfe, 0x06, 0x00, 0x80, 0xc2, 0x01, 0x01, 0xe8, 0xfe, 0x07, 0x00, 0x80, 0xc2, 0x02, 0x01, 0x00, 0x00, 0xfe, 0x16, 0x00, 0x80, 0xc2, 0x03, 0x01, 0xe8, 0x0f, 0x76, 0x32, 0x2d, 0x30, 0x34, 0x38, 0x38, 0x2d, 0x30, 0x33, 0x2d, 0x30, 0x35, 0x30, 0x35, 0xfe, 0x05, 0x00, 0x80, 0xc2, 0x04, 0x00, 0x00, 0x00]>, 'chassis-id-type': , 'chassis-id': <'00:01:30:F9:AD:A0'>, 'port-id-type': , 'port-id': <'1/1'>, 'destination': <'nearest-bridge'>, 'port-description': <'Summit300-48-Port 1001'>, 'system-name': <'Summit300-48'>, 'system-description': <'Summit300-48 - Version 7.4e.1 (Build 5) by Release_Master 05/27/05 04:53:11'>, 'system-capabilities': , 'management-addresses': <[{'address-subtype': , 'object-id': <@ay []>, 'interface-number': , 'address': <[byte 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0]>, 'interface-number-subtype': }]>, 'ieee-802-1-pvid': , 'ieee-802-1-ppvid': , 'ieee-802-1-ppvid-flags': , 'ieee-802-1-ppvids': <[{'flags': , 'ppvid': }]>, 'ieee-802-1-vid': , 'ieee-802-1-vlan-name': <'v2-0488-03-0505'>, 'ieee-802-1-vlans': <[{'vid': , 'name': <'v2-0488-03-0505'>}]>, 'ieee-802-3-mac-phy-conf': <{'operational-mau-type': , 'autoneg': , 'pmd-autoneg-cap': }>, 'ieee-802-3-power-via-mdi': <{'pse-power-pair': , 'mdi-power-support': , 'power-class': }>, 'ieee-802-3-max-frame-size': }", /* ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, /* destination mac */ 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, /* source mac */ @@ -238,7 +242,11 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS, "00:01:30:F9:AD:A0", SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME, "1/1"); g_assert (neighbor); - g_assert_cmpint (g_variant_n_children (neighbor), ==, 4 + 16); + g_assert_cmpint (g_variant_n_children (neighbor), ==, 1 + 4 + 16); + + attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_RAW, G_VARIANT_TYPE_BYTESTRING); + nmtst_assert_variant_bytestring (attr, _test_recv_data1_frame0.frame, _test_recv_data1_frame0.frame_len); + nm_clear_g_variant (&attr); attr = g_variant_lookup_value (neighbor, NM_LLDP_ATTR_DESTINATION, G_VARIANT_TYPE_STRING); nmtst_assert_variant_string (attr, NM_LLDP_DEST_NEAREST_BRIDGE); @@ -361,7 +369,7 @@ _test_recv_data1_check (GMainLoop *loop, NMLldpListener *listener) TEST_RECV_DATA_DEFINE (_test_recv_data1, 1, _test_recv_data1_check, &_test_recv_data1_frame0); TEST_RECV_FRAME_DEFINE (_test_recv_data2_frame0_ttl1, - "{'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", + "{'raw': <[byte 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x88, 0xcc, 0x02, 0x07, 0x04, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x04, 0x04, 0x05, 0x31, 0x2f, 0x33, 0x06, 0x02, 0x00, 0x01, 0x08, 0x04, 0x50, 0x6f, 0x72, 0x74, 0x0a, 0x03, 0x53, 0x59, 0x53, 0x0c, 0x04, 0x66, 0x6f, 0x6f, 0x00, 0x00, 0x00]>, 'chassis-id-type': , 'chassis-id': <'00:01:02:03:04:05'>, 'port-id-type': , 'port-id': <'1/3'>, 'destination': <'nearest-non-tpmr-bridge'>, 'port-description': <'Port'>, 'system-name': <'SYS'>, 'system-description': <'foo'>}", /* Ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ @@ -534,6 +542,10 @@ test_parse_frames (gconstpointer test_data) v_neighbor = nmtst_lldp_parse_from_raw (frame->frame, frame->frame_len); g_assert (v_neighbor); + attr = g_variant_lookup_value (v_neighbor, NM_LLDP_ATTR_RAW, G_VARIANT_TYPE_BYTESTRING); + nmtst_assert_variant_bytestring (attr, frame->frame, frame->frame_len); + nm_clear_g_variant (&attr); + as_variant = g_variant_print (v_neighbor, TRUE); g_assert (as_variant); g_assert_cmpstr (frame->as_variant, ==, as_variant); From 28a0093d67be1eb029785be30dfc730957b86ca4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 Jun 2020 13:26:39 +0200 Subject: [PATCH 15/28] lldp: separate LLDP attribute list from LldpNeighbor We actually only need to parse the attributes while constructing the GVariant. In a first step decouple the tracking of the parsed attributes from LldpNeighbor struct. More will follow. --- src/devices/nm-lldp-listener.c | 156 +++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 67 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 3623826826..af0e13efda 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -103,6 +103,10 @@ G_DEFINE_TYPE (NMLldpListener, nm_lldp_listener, G_TYPE_OBJECT) /*****************************************************************************/ +typedef struct { + LldpAttrData a[_LLDP_ATTR_ID_COUNT]; +} LldpAttrs; + typedef struct { GVariant *variant; char *chassis_id; @@ -110,7 +114,7 @@ typedef struct { sd_lldp_neighbor *neighbor_sd; - LldpAttrData attrs[_LLDP_ATTR_ID_COUNT]; + LldpAttrs attrs; struct ether_addr destination_address; @@ -224,12 +228,14 @@ _lldp_attr_id_to_type (LldpAttrId attr_id) } static void -_lldp_attr_set_str (LldpAttrData *pdata, LldpAttrId attr_id, const char *v_string) +_lldp_attrs_set_str (LldpAttrs *attrs, LldpAttrId attr_id, const char *v_string) { - nm_assert (pdata); + LldpAttrData *pdata; + + nm_assert (attrs); nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_STRING); - pdata = &pdata[attr_id]; + pdata = &attrs->a[attr_id]; /* we ignore duplicate fields silently. */ if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) @@ -239,12 +245,14 @@ _lldp_attr_set_str (LldpAttrData *pdata, LldpAttrId attr_id, const char *v_strin } static void -_lldp_attr_set_str_take (LldpAttrData *pdata, LldpAttrId attr_id, char *str) +_lldp_attrs_set_str_take (LldpAttrs *attrs, LldpAttrId attr_id, char *str) { - nm_assert (pdata); + LldpAttrData *pdata; + + nm_assert (attrs); nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_STRING); - pdata = &pdata[attr_id]; + pdata = &attrs->a[attr_id]; /* we ignore duplicate fields silently. */ if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) { @@ -257,12 +265,14 @@ _lldp_attr_set_str_take (LldpAttrData *pdata, LldpAttrId attr_id, char *str) } static void -_lldp_attr_set_uint32 (LldpAttrData *pdata, LldpAttrId attr_id, guint32 v_uint32) +_lldp_attrs_set_uint32 (LldpAttrs *attrs, LldpAttrId attr_id, guint32 v_uint32) { - nm_assert (pdata); + LldpAttrData *pdata; + + nm_assert (attrs); nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_UINT32); - pdata = &pdata[attr_id]; + pdata = &attrs->a[attr_id]; /* we ignore duplicate fields silently. */ if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) @@ -272,13 +282,14 @@ _lldp_attr_set_uint32 (LldpAttrData *pdata, LldpAttrId attr_id, guint32 v_uint32 } static void -_lldp_attr_set_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) +_lldp_attrs_set_variant (LldpAttrs *attrs, LldpAttrId attr_id, GVariant *variant) { + LldpAttrData *pdata; - nm_assert (pdata); + nm_assert (attrs); nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_VARIANT); - pdata = &pdata[attr_id]; + pdata = &attrs->a[attr_id]; /* we ignore duplicate fields silently */ if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) { @@ -291,14 +302,16 @@ _lldp_attr_set_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *varia } static void -_lldp_attr_add_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *variant) +_lldp_attr_add_variant (LldpAttrs *attrs, LldpAttrId attr_id, GVariant *variant) { - nm_assert (pdata); + LldpAttrData *pdata; + + nm_assert (attrs); nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); g_variant_ref_sink (variant); - pdata = &pdata[attr_id]; + pdata = &attrs->a[attr_id]; if (pdata->attr_type == LLDP_ATTR_TYPE_NONE) { pdata->attr_type = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS; @@ -328,6 +341,39 @@ _lldp_attr_add_variant (LldpAttrData *pdata, LldpAttrId attr_id, GVariant *varia /*****************************************************************************/ +static void +_lldp_attrs_clear (LldpAttrs *attrs) +{ + LldpAttrId attr_id; + guint i; + + for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { + LldpAttrData *pdata = &attrs->a[attr_id]; + + switch (pdata->attr_type) { + case LLDP_ATTR_TYPE_STRING: + g_free (pdata->v_string); + break; + case LLDP_ATTR_TYPE_VARIANT: + g_variant_unref (pdata->v_variant); + break; + case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: + if (pdata->v_variant_list.alloc == 1) + g_variant_unref (pdata->v_variant_list.arr1); + else { + for (i = 0; i < pdata->v_variant_list.len; i++) + g_variant_unref (pdata->v_variant_list.arr[i]); + g_free (pdata->v_variant_list.arr); + } + break; + default: + ; + } + } +} + +/*****************************************************************************/ + static void lldp_neighbor_get_raw (LldpNeighbor *neigh, const guint8 **out_raw_data, @@ -391,37 +437,12 @@ lldp_neighbor_id_equal (gconstpointer a, gconstpointer b) static void lldp_neighbor_free (LldpNeighbor *neighbor) { - LldpAttrId attr_id; - guint i; - if (!neighbor) return; g_free (neighbor->chassis_id); g_free (neighbor->port_id); - for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - LldpAttrData *pdata = &neighbor->attrs[attr_id]; - - switch (pdata->attr_type) { - case LLDP_ATTR_TYPE_STRING: - g_free (pdata->v_string); - break; - case LLDP_ATTR_TYPE_VARIANT: - g_variant_unref (pdata->v_variant); - break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: - if (pdata->v_variant_list.alloc == 1) - g_variant_unref (pdata->v_variant_list.arr1); - else { - for (i = 0; i < pdata->v_variant_list.len; i++) - g_variant_unref (pdata->v_variant_list.arr[i]); - g_free (pdata->v_variant_list.arr); - } - break; - default: - ; - } - } + _lldp_attrs_clear (&neighbor->attrs); nm_g_variant_unref (neighbor->variant); sd_lldp_neighbor_unref (neighbor->neighbor_sd); nm_g_slice_free (neighbor); @@ -618,16 +639,16 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) } if (sd_lldp_neighbor_get_port_description (neighbor_sd, &str) == 0) - _lldp_attr_set_str (neigh->attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str); + _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str); if (sd_lldp_neighbor_get_system_name (neighbor_sd, &str) == 0) - _lldp_attr_set_str (neigh->attrs, LLDP_ATTR_ID_SYSTEM_NAME, str); + _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_NAME, str); if (sd_lldp_neighbor_get_system_description (neighbor_sd, &str) == 0) - _lldp_attr_set_str (neigh->attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str); + _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str); if (sd_lldp_neighbor_get_system_capabilities (neighbor_sd, &data16) == 0) - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16); + _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16); r = sd_lldp_neighbor_tlv_rewind (neighbor_sd); if (r < 0) { @@ -650,7 +671,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_TYPE_MGMT_ADDRESS: variant = parse_management_address_tlv (data8, len); if (variant) { - _lldp_attr_add_variant (neigh->attrs, + _lldp_attr_add_variant (&neigh->attrs, LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, variant); } @@ -699,22 +720,22 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: if (len != 2) continue; - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PVID, - unaligned_read_be16 (data8)); + _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PVID, + unaligned_read_be16 (data8)); break; case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID: if (len != 3) continue; - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, - data8[0]); - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID, - unaligned_read_be16 (&data8[1])); + _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, + data8[0]); + _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID, + unaligned_read_be16 (&data8[1])); g_variant_dict_init (&dict, NULL); g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); - _lldp_attr_add_variant (neigh->attrs, + _lldp_attr_add_variant (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVIDS, g_variant_dict_end (&dict)); break; @@ -740,15 +761,15 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "vid", "u", vid); g_variant_dict_insert (&dict, "name", "s", name); - _lldp_attr_add_variant (neigh->attrs, + _lldp_attr_add_variant (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLANS, g_variant_dict_end (&dict)); - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid); + _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid); if (name_to_free) - _lldp_attr_set_str_take (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free); + _lldp_attrs_set_str_take (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free); else - _lldp_attr_set_str (neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name); + _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name); break; } default: @@ -767,9 +788,9 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); - _lldp_attr_set_variant (neigh->attrs, - LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, - g_variant_dict_end (&dict)); + _lldp_attrs_set_variant (&neigh->attrs, + LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, + g_variant_dict_end (&dict)); break; case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: if (len != 3) @@ -780,15 +801,16 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); - _lldp_attr_set_variant (neigh->attrs, - LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, - g_variant_dict_end (&dict)); + _lldp_attrs_set_variant (&neigh->attrs, + LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, + g_variant_dict_end (&dict)); break; case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: if (len != 2) continue; - _lldp_attr_set_uint32 (neigh->attrs, LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, - unaligned_read_be16 (data8)); + _lldp_attrs_set_uint32 (&neigh->attrs, + LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, + unaligned_read_be16 (data8)); break; } } @@ -848,7 +870,7 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) } for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - const LldpAttrData *pdata = &neigh->attrs[attr_id]; + const LldpAttrData *pdata = &neigh->attrs.a[attr_id]; nm_assert (NM_IN_SET (pdata->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); switch (pdata->attr_type) { From 22f161d722b9b5827bbe3383a565a45e47e74af2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 Jun 2020 13:36:41 +0200 Subject: [PATCH 16/28] lldp: split parsing of LLDP attributes from lldp_neighbor_new() Move the parsing of the LLDP attributes to a separate function. In the next step, we will no longer keep all attribute around and no longer parse them during lldp_neighbor_new(). One effect is that we can no longer (easily) declare the LLDP message as invalid, if parsing the attributes fails. That makes IMO more sense, because we should try to expose what little we could parse, and not be forgiving to unexpected data. If we wanted, we still could hide such neighbors entirely from being exposed, but that is not done, because it seems better to expose the parts that were valid. --- src/devices/nm-lldp-listener.c | 372 +++++++++++++++++---------------- 1 file changed, 191 insertions(+), 181 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index af0e13efda..7f44034e01 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -552,16 +552,202 @@ err: return NULL; } +static void +_lldp_attrs_parse (LldpAttrs *attrs, + sd_lldp_neighbor *neighbor_sd) +{ + const char *str; + uint16_t data16; + uint8_t *data8; + gsize len; + int r; + + if (sd_lldp_neighbor_get_port_description (neighbor_sd, &str) == 0) + _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str); + + if (sd_lldp_neighbor_get_system_name (neighbor_sd, &str) == 0) + _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_NAME, str); + + if (sd_lldp_neighbor_get_system_description (neighbor_sd, &str) == 0) + _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str); + + if (sd_lldp_neighbor_get_system_capabilities (neighbor_sd, &data16) == 0) + _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16); + + r = sd_lldp_neighbor_tlv_rewind (neighbor_sd); + if (r < 0) { + nm_assert_not_reached (); + return; + } + do { + guint8 oui[3]; + guint8 type, subtype; + GVariant *variant; + + if (sd_lldp_neighbor_tlv_get_type (neighbor_sd, &type) < 0) + continue; + + if (sd_lldp_neighbor_tlv_get_raw (neighbor_sd, (void *) &data8, &len) < 0) + continue; + + switch (type) { + case SD_LLDP_TYPE_MGMT_ADDRESS: + variant = parse_management_address_tlv (data8, len); + if (variant) { + _lldp_attr_add_variant (attrs, + LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, + variant); + } + continue; + case SD_LLDP_TYPE_PRIVATE: + break; + default: + continue; + } + + r = sd_lldp_neighbor_tlv_get_oui (neighbor_sd, oui, &subtype); + if (r < 0) { + if (r == -ENXIO) + continue; + + /* in other cases, something is seriously wrong. Abort, but + * keep what we parsed so far. */ + return; + } + + if ( memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) != 0 + && memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) != 0) + continue; + + /* skip over leading TLV, OUI and subtype */ +#if NM_MORE_ASSERTS > 5 + { + guint8 check_hdr[] = { + 0xfe | (((len - 2) >> 8) & 0x01), ((len - 2) & 0xFF), + oui[0], oui[1], oui[2], + subtype + }; + + nm_assert (len > 2 + 3 +1); + nm_assert (memcmp (data8, check_hdr, sizeof check_hdr) == 0); + } +#endif + if (len <= 6) + continue; + data8 += 6; + len -= 6; + + if (memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) == 0) { + GVariantDict dict; + + switch (subtype) { + case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: + if (len != 2) + continue; + _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PVID, + unaligned_read_be16 (data8)); + break; + case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID: + if (len != 3) + continue; + _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, + data8[0]); + _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID, + unaligned_read_be16 (&data8[1])); + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); + g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); + + _lldp_attr_add_variant (attrs, + LLDP_ATTR_ID_IEEE_802_1_PPVIDS, + g_variant_dict_end (&dict)); + break; + case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { + int l; + guint32 vid; + const char *name; + char *name_to_free; + + if (len <= 3) + continue; + + l = data8[2]; + if (len != 3 + l) + continue; + if (l > 32) + continue; + + name = nm_utils_buf_utf8safe_escape (&data8[3], l, 0, &name_to_free); + vid = unaligned_read_be16 (&data8[0]); + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "vid", "u", vid); + g_variant_dict_insert (&dict, "name", "s", name); + + _lldp_attr_add_variant (attrs, + LLDP_ATTR_ID_IEEE_802_1_VLANS, + g_variant_dict_end (&dict)); + + _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid); + if (name_to_free) + _lldp_attrs_set_str_take (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free); + else + _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name); + break; + } + default: + continue; + } + } else if (memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) == 0) { + GVariantDict dict; + + switch (subtype) { + case SD_LLDP_OUI_802_3_SUBTYPE_MAC_PHY_CONFIG_STATUS: + if (len != 5) + continue; + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "autoneg", "u", (guint32) data8[0]); + g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); + g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); + + _lldp_attrs_set_variant (attrs, + LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, + g_variant_dict_end (&dict)); + break; + case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: + if (len != 3) + continue; + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "mdi-power-support", "u", (guint32) data8[0]); + g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); + g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); + + _lldp_attrs_set_variant (attrs, + LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, + g_variant_dict_end (&dict)); + break; + case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: + if (len != 2) + continue; + _lldp_attrs_set_uint32 (attrs, + LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, + unaligned_read_be16 (data8)); + break; + } + } + } while (sd_lldp_neighbor_tlv_next (neighbor_sd) > 0); +} + static LldpNeighbor * lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) { nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL; uint8_t chassis_id_type, port_id_type; - uint16_t data16; - uint8_t *data8; const void *chassis_id, *port_id; - gsize chassis_id_len, port_id_len, len; - const char *str; + gsize chassis_id_len, port_id_len; int r; r = sd_lldp_neighbor_get_chassis_id (neighbor_sd, &chassis_id_type, @@ -638,183 +824,7 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) goto out; } - if (sd_lldp_neighbor_get_port_description (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str); - - if (sd_lldp_neighbor_get_system_name (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_NAME, str); - - if (sd_lldp_neighbor_get_system_description (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str); - - if (sd_lldp_neighbor_get_system_capabilities (neighbor_sd, &data16) == 0) - _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16); - - r = sd_lldp_neighbor_tlv_rewind (neighbor_sd); - if (r < 0) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failed reading tlv (rewind): %s", nm_strerror_native (-r)); - goto out; - } - do { - guint8 oui[3]; - guint8 type, subtype; - GVariant *variant; - - if (sd_lldp_neighbor_tlv_get_type (neighbor_sd, &type) < 0) - continue; - - if (sd_lldp_neighbor_tlv_get_raw (neighbor_sd, (void *) &data8, &len) < 0) - continue; - - switch (type) { - case SD_LLDP_TYPE_MGMT_ADDRESS: - variant = parse_management_address_tlv (data8, len); - if (variant) { - _lldp_attr_add_variant (&neigh->attrs, - LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, - variant); - } - continue; - case SD_LLDP_TYPE_PRIVATE: - break; - default: - continue; - } - - r = sd_lldp_neighbor_tlv_get_oui (neighbor_sd, oui, &subtype); - if (r < 0) { - if (r == -ENXIO) - continue; - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failed reading tlv: %s", nm_strerror_native (-r)); - goto out; - } - - if ( memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) != 0 - && memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) != 0) - continue; - - /* skip over leading TLV, OUI and subtype */ -#if NM_MORE_ASSERTS > 5 - { - guint8 check_hdr[] = { - 0xfe | (((len - 2) >> 8) & 0x01), ((len - 2) & 0xFF), - oui[0], oui[1], oui[2], - subtype - }; - - nm_assert (len > 2 + 3 +1); - nm_assert (memcmp (data8, check_hdr, sizeof check_hdr) == 0); - } -#endif - if (len <= 6) - continue; - data8 += 6; - len -= 6; - - if (memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) == 0) { - GVariantDict dict; - - switch (subtype) { - case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: - if (len != 2) - continue; - _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PVID, - unaligned_read_be16 (data8)); - break; - case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID: - if (len != 3) - continue; - _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, - data8[0]); - _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID, - unaligned_read_be16 (&data8[1])); - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); - - _lldp_attr_add_variant (&neigh->attrs, - LLDP_ATTR_ID_IEEE_802_1_PPVIDS, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { - int l; - guint32 vid; - const char *name; - char *name_to_free; - - if (len <= 3) - continue; - - l = data8[2]; - if (len != 3 + l) - continue; - if (l > 32) - continue; - - name = nm_utils_buf_utf8safe_escape (&data8[3], l, 0, &name_to_free); - vid = unaligned_read_be16 (&data8[0]); - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "vid", "u", vid); - g_variant_dict_insert (&dict, "name", "s", name); - - _lldp_attr_add_variant (&neigh->attrs, - LLDP_ATTR_ID_IEEE_802_1_VLANS, - g_variant_dict_end (&dict)); - - _lldp_attrs_set_uint32 (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid); - if (name_to_free) - _lldp_attrs_set_str_take (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free); - else - _lldp_attrs_set_str (&neigh->attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name); - break; - } - default: - continue; - } - } else if (memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) == 0) { - GVariantDict dict; - - switch (subtype) { - case SD_LLDP_OUI_802_3_SUBTYPE_MAC_PHY_CONFIG_STATUS: - if (len != 5) - continue; - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "autoneg", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); - - _lldp_attrs_set_variant (&neigh->attrs, - LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: - if (len != 3) - continue; - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "mdi-power-support", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); - g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); - - _lldp_attrs_set_variant (&neigh->attrs, - LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: - if (len != 2) - continue; - _lldp_attrs_set_uint32 (&neigh->attrs, - LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, - unaligned_read_be16 (data8)); - break; - } - } - } while (sd_lldp_neighbor_tlv_next (neighbor_sd) > 0); + _lldp_attrs_parse (&neigh->attrs, neighbor_sd); neigh->valid = TRUE; From 5e6e361c21ed24228efac295d0ff214ce3b3a698 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 6 Jun 2020 13:43:26 +0200 Subject: [PATCH 17/28] lldp: no longer keep parsed attributes in LldpNeighbor We only need to parse them to construct the GVariant. There is no need to keep them around otherwise. We still keep LldpAttrs array and don't construct the GVariant right away. The benefit is that this way while parsing we set the array fields, and afterwards, when we generate the string dictionary, the keys are sorted. --- src/devices/nm-lldp-listener.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 7f44034e01..67e106d4e0 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -114,8 +114,6 @@ typedef struct { sd_lldp_neighbor *neighbor_sd; - LldpAttrs attrs; - struct ether_addr destination_address; guint8 chassis_id_type; @@ -227,6 +225,8 @@ _lldp_attr_id_to_type (LldpAttrId attr_id) return types[attr_id]; } +/*****************************************************************************/ + static void _lldp_attrs_set_str (LldpAttrs *attrs, LldpAttrId attr_id, const char *v_string) { @@ -442,7 +442,6 @@ lldp_neighbor_free (LldpNeighbor *neighbor) g_free (neighbor->chassis_id); g_free (neighbor->port_id); - _lldp_attrs_clear (&neighbor->attrs); nm_g_variant_unref (neighbor->variant); sd_lldp_neighbor_unref (neighbor->neighbor_sd); nm_g_slice_free (neighbor); @@ -824,8 +823,6 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) goto out; } - _lldp_attrs_parse (&neigh->attrs, neighbor_sd); - neigh->valid = TRUE; out: @@ -840,6 +837,7 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) LldpAttrId attr_id; const guint8 *raw_data; gsize raw_len; + LldpAttrs attrs; if (neigh->variant) return neigh->variant; @@ -879,8 +877,12 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) g_variant_new_string (dest_str)); } + attrs = (LldpAttrs) { }; + + _lldp_attrs_parse (&attrs, neigh->neighbor_sd); + for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - const LldpAttrData *pdata = &neigh->attrs.a[attr_id]; + const LldpAttrData *pdata = &attrs.a[attr_id]; nm_assert (NM_IN_SET (pdata->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); switch (pdata->attr_type) { @@ -918,6 +920,8 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) } } + _lldp_attrs_clear (&attrs); + return (neigh->variant = g_variant_ref_sink (g_variant_builder_end (&builder))); } @@ -1272,4 +1276,3 @@ nm_lldp_listener_class_init (NMLldpListenerClass *klass) g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties); } - From 94ee6f4fe1b17c4f7cec96006402e2474f7178ad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 16:34:36 +0200 Subject: [PATCH 18/28] lldp: use nm_g_variant_builder_add_sv*() helpers in "nm-lldp-listener.c" --- src/devices/nm-lldp-listener.c | 61 ++++++++++++++-------------------- 1 file changed, 25 insertions(+), 36 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 67e106d4e0..d87f40a609 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -833,7 +833,7 @@ static GVariant * lldp_neighbor_to_variant (LldpNeighbor *neigh) { GVariantBuilder builder; - const char *dest_str; + const char *str; LldpAttrId attr_id; const guint8 *raw_data; gsize raw_len; @@ -850,32 +850,21 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) NM_LLDP_ATTR_RAW, raw_data, raw_len); - g_variant_builder_add (&builder, "{sv}", - NM_LLDP_ATTR_CHASSIS_ID_TYPE, - g_variant_new_uint32 (neigh->chassis_id_type)); - g_variant_builder_add (&builder, "{sv}", - NM_LLDP_ATTR_CHASSIS_ID, - g_variant_new_string (neigh->chassis_id)); - g_variant_builder_add (&builder, "{sv}", - NM_LLDP_ATTR_PORT_ID_TYPE, - g_variant_new_uint32 (neigh->port_id_type)); - g_variant_builder_add (&builder, "{sv}", - NM_LLDP_ATTR_PORT_ID, - g_variant_new_string (neigh->port_id)); + nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_CHASSIS_ID_TYPE, neigh->chassis_id_type); + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_CHASSIS_ID, neigh->chassis_id); + nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_PORT_ID_TYPE, neigh->port_id_type); + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_PORT_ID, neigh->port_id); if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_BRIDGE)) - dest_str = NM_LLDP_DEST_NEAREST_BRIDGE; + str = NM_LLDP_DEST_NEAREST_BRIDGE; else if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_NON_TPMR_BRIDGE)) - dest_str = NM_LLDP_DEST_NEAREST_NON_TPMR_BRIDGE; + str = NM_LLDP_DEST_NEAREST_NON_TPMR_BRIDGE; else if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_CUSTOMER_BRIDGE)) - dest_str = NM_LLDP_DEST_NEAREST_CUSTOMER_BRIDGE; + str = NM_LLDP_DEST_NEAREST_CUSTOMER_BRIDGE; else - dest_str = NULL; - if (dest_str) { - g_variant_builder_add (&builder, "{sv}", - NM_LLDP_ATTR_DESTINATION, - g_variant_new_string (dest_str)); - } + str = NULL; + if (str) + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_DESTINATION, str); attrs = (LldpAttrs) { }; @@ -887,19 +876,19 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) nm_assert (NM_IN_SET (pdata->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); switch (pdata->attr_type) { case LLDP_ATTR_TYPE_UINT32: - g_variant_builder_add (&builder, "{sv}", - _lldp_attr_id_to_name (attr_id), - g_variant_new_uint32 (pdata->v_uint32)); + nm_g_variant_builder_add_sv_uint32 (&builder, + _lldp_attr_id_to_name (attr_id), + pdata->v_uint32); break; case LLDP_ATTR_TYPE_STRING: - g_variant_builder_add (&builder, "{sv}", - _lldp_attr_id_to_name (attr_id), - g_variant_new_string (pdata->v_string)); + nm_g_variant_builder_add_sv_str (&builder, + _lldp_attr_id_to_name (attr_id), + pdata->v_string); break; case LLDP_ATTR_TYPE_VARIANT: - g_variant_builder_add (&builder, "{sv}", - _lldp_attr_id_to_name (attr_id), - pdata->v_variant); + nm_g_variant_builder_add_sv (&builder, + _lldp_attr_id_to_name (attr_id), + pdata->v_variant); break; case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { GVariant *const*variants; @@ -908,11 +897,11 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) variants = &pdata->v_variant_list.arr1; else variants = pdata->v_variant_list.arr; - g_variant_builder_add (&builder, "{sv}", - _lldp_attr_id_to_name (attr_id), - g_variant_new_array (G_VARIANT_TYPE ("a{sv}"), - variants, - pdata->v_variant_list.len)); + nm_g_variant_builder_add_sv (&builder, + _lldp_attr_id_to_name (attr_id), + g_variant_new_array (G_VARIANT_TYPE ("a{sv}"), + variants, + pdata->v_variant_list.len)); break; } case LLDP_ATTR_TYPE_NONE: From 4aa0b9180afdcb0be6bc56863e842f392d228d97 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 16:35:07 +0200 Subject: [PATCH 19/28] lldp: add LLDP attributes to GVariant builder without intermediate parsing (1) The intermediate parsing step serves very little purpose. The only use is to ensure that we always add the keys in a stable order, but we can easily ensure that otherwise. --- src/devices/nm-lldp-listener.c | 39 ++++++++++++---------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index d87f40a609..65f37c3130 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -34,10 +34,6 @@ typedef enum { typedef enum { /* the order of the enum values determines the order of the fields in * the variant. */ - LLDP_ATTR_ID_PORT_DESCRIPTION, - LLDP_ATTR_ID_SYSTEM_NAME, - LLDP_ATTR_ID_SYSTEM_DESCRIPTION, - LLDP_ATTR_ID_SYSTEM_CAPABILITIES, LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, LLDP_ATTR_ID_IEEE_802_1_PVID, LLDP_ATTR_ID_IEEE_802_1_PPVID, @@ -169,10 +165,6 @@ static const char * _lldp_attr_id_to_name (LldpAttrId attr_id) { static const char *const names[_LLDP_ATTR_ID_COUNT] = { - [LLDP_ATTR_ID_PORT_DESCRIPTION] = NM_LLDP_ATTR_PORT_DESCRIPTION, - [LLDP_ATTR_ID_SYSTEM_NAME] = NM_LLDP_ATTR_SYSTEM_NAME, - [LLDP_ATTR_ID_SYSTEM_DESCRIPTION] = NM_LLDP_ATTR_SYSTEM_DESCRIPTION, - [LLDP_ATTR_ID_SYSTEM_CAPABILITIES] = NM_LLDP_ATTR_SYSTEM_CAPABILITIES, [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = NM_LLDP_ATTR_MANAGEMENT_ADDRESSES, [LLDP_ATTR_ID_IEEE_802_1_PVID] = NM_LLDP_ATTR_IEEE_802_1_PVID, [LLDP_ATTR_ID_IEEE_802_1_PPVID] = NM_LLDP_ATTR_IEEE_802_1_PPVID, @@ -197,10 +189,6 @@ static LldpAttrType _lldp_attr_id_to_type (LldpAttrId attr_id) { static const LldpAttrType types[_LLDP_ATTR_ID_COUNT] = { - [LLDP_ATTR_ID_PORT_DESCRIPTION] = LLDP_ATTR_TYPE_STRING, - [LLDP_ATTR_ID_SYSTEM_NAME] = LLDP_ATTR_TYPE_STRING, - [LLDP_ATTR_ID_SYSTEM_DESCRIPTION] = LLDP_ATTR_TYPE_STRING, - [LLDP_ATTR_ID_SYSTEM_CAPABILITIES] = LLDP_ATTR_TYPE_UINT32, [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, [LLDP_ATTR_ID_IEEE_802_1_PVID] = LLDP_ATTR_TYPE_UINT32, [LLDP_ATTR_ID_IEEE_802_1_PPVID] = LLDP_ATTR_TYPE_UINT32, @@ -555,24 +543,10 @@ static void _lldp_attrs_parse (LldpAttrs *attrs, sd_lldp_neighbor *neighbor_sd) { - const char *str; - uint16_t data16; uint8_t *data8; gsize len; int r; - if (sd_lldp_neighbor_get_port_description (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_PORT_DESCRIPTION, str); - - if (sd_lldp_neighbor_get_system_name (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_NAME, str); - - if (sd_lldp_neighbor_get_system_description (neighbor_sd, &str) == 0) - _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_SYSTEM_DESCRIPTION, str); - - if (sd_lldp_neighbor_get_system_capabilities (neighbor_sd, &data16) == 0) - _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_SYSTEM_CAPABILITIES, data16); - r = sd_lldp_neighbor_tlv_rewind (neighbor_sd); if (r < 0) { nm_assert_not_reached (); @@ -838,6 +812,7 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) const guint8 *raw_data; gsize raw_len; LldpAttrs attrs; + uint16_t u16; if (neigh->variant) return neigh->variant; @@ -866,6 +841,18 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) if (str) nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_DESTINATION, str); + if (sd_lldp_neighbor_get_port_description (neigh->neighbor_sd, &str) == 0) + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_PORT_DESCRIPTION, str); + + if (sd_lldp_neighbor_get_system_name (neigh->neighbor_sd, &str) == 0) + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_SYSTEM_NAME, str); + + if (sd_lldp_neighbor_get_system_description (neigh->neighbor_sd, &str) == 0) + nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_SYSTEM_DESCRIPTION, str); + + if (sd_lldp_neighbor_get_system_capabilities (neigh->neighbor_sd, &u16) == 0) + nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_SYSTEM_CAPABILITIES, u16); + attrs = (LldpAttrs) { }; _lldp_attrs_parse (&attrs, neigh->neighbor_sd); From cf4763207f426e7d97f6be9684ead30a2077152c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 17:58:00 +0200 Subject: [PATCH 20/28] lldp: add LLDP attributes to GVariant builder without intermediate parsing (2) --- src/devices/nm-lldp-listener.c | 649 ++++++++++----------------------- 1 file changed, 188 insertions(+), 461 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 65f37c3130..3323125c7e 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -23,48 +23,6 @@ #define LLDP_MAC_NEAREST_NON_TPMR_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03 })) #define LLDP_MAC_NEAREST_CUSTOMER_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 })) -typedef enum { - LLDP_ATTR_TYPE_NONE, - LLDP_ATTR_TYPE_UINT32, - LLDP_ATTR_TYPE_STRING, - LLDP_ATTR_TYPE_VARIANT, - LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, -} LldpAttrType; - -typedef enum { - /* the order of the enum values determines the order of the fields in - * the variant. */ - LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, - LLDP_ATTR_ID_IEEE_802_1_PVID, - LLDP_ATTR_ID_IEEE_802_1_PPVID, - LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, - LLDP_ATTR_ID_IEEE_802_1_PPVIDS, - LLDP_ATTR_ID_IEEE_802_1_VID, - LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, - LLDP_ATTR_ID_IEEE_802_1_VLANS, - LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, - LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, - LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, - _LLDP_ATTR_ID_COUNT, -} LldpAttrId; - -typedef struct { - LldpAttrType attr_type; - union { - guint32 v_uint32; - char *v_string; - GVariant *v_variant; - struct { - union { - GVariant **arr; - GVariant *arr1; - }; - guint len; - guint alloc; - } v_variant_list; - }; -} LldpAttrData; - /*****************************************************************************/ NM_GOBJECT_PROPERTIES_DEFINE (NMLldpListener, @@ -99,10 +57,6 @@ G_DEFINE_TYPE (NMLldpListener, nm_lldp_listener, G_TYPE_OBJECT) /*****************************************************************************/ -typedef struct { - LldpAttrData a[_LLDP_ATTR_ID_COUNT]; -} LldpAttrs; - typedef struct { GVariant *variant; char *chassis_id; @@ -161,207 +115,6 @@ ether_addr_equal (const struct ether_addr *a1, const struct ether_addr *a2) /*****************************************************************************/ -static const char * -_lldp_attr_id_to_name (LldpAttrId attr_id) -{ - static const char *const names[_LLDP_ATTR_ID_COUNT] = { - [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = NM_LLDP_ATTR_MANAGEMENT_ADDRESSES, - [LLDP_ATTR_ID_IEEE_802_1_PVID] = NM_LLDP_ATTR_IEEE_802_1_PVID, - [LLDP_ATTR_ID_IEEE_802_1_PPVID] = NM_LLDP_ATTR_IEEE_802_1_PPVID, - [LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS] = NM_LLDP_ATTR_IEEE_802_1_PPVID_FLAGS, - [LLDP_ATTR_ID_IEEE_802_1_PPVIDS] = NM_LLDP_ATTR_IEEE_802_1_PPVIDS, - [LLDP_ATTR_ID_IEEE_802_1_VID] = NM_LLDP_ATTR_IEEE_802_1_VID, - [LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME] = NM_LLDP_ATTR_IEEE_802_1_VLAN_NAME, - [LLDP_ATTR_ID_IEEE_802_1_VLANS] = NM_LLDP_ATTR_IEEE_802_1_VLANS, - [LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF] = NM_LLDP_ATTR_IEEE_802_3_MAC_PHY_CONF, - [LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI] = NM_LLDP_ATTR_IEEE_802_3_POWER_VIA_MDI, - [LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE] = NM_LLDP_ATTR_IEEE_802_3_MAX_FRAME_SIZE, - }; - - if (NM_MORE_ASSERT_ONCE (5)) - nm_assert (nm_utils_strv_find_first ((char **) names, G_N_ELEMENTS (names), NULL) == -1); - nm_assert (_NM_INT_NOT_NEGATIVE (attr_id)); - nm_assert (attr_id < G_N_ELEMENTS (names)); - return names[attr_id]; -}; - -static LldpAttrType -_lldp_attr_id_to_type (LldpAttrId attr_id) -{ - static const LldpAttrType types[_LLDP_ATTR_ID_COUNT] = { - [LLDP_ATTR_ID_MANAGEMENT_ADDRESSES] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, - [LLDP_ATTR_ID_IEEE_802_1_PVID] = LLDP_ATTR_TYPE_UINT32, - [LLDP_ATTR_ID_IEEE_802_1_PPVID] = LLDP_ATTR_TYPE_UINT32, - [LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS] = LLDP_ATTR_TYPE_UINT32, - [LLDP_ATTR_ID_IEEE_802_1_PPVIDS] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, - [LLDP_ATTR_ID_IEEE_802_1_VID] = LLDP_ATTR_TYPE_UINT32, - [LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME] = LLDP_ATTR_TYPE_STRING, - [LLDP_ATTR_ID_IEEE_802_1_VLANS] = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS, - [LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF] = LLDP_ATTR_TYPE_VARIANT, - [LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI] = LLDP_ATTR_TYPE_VARIANT, - [LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE] = LLDP_ATTR_TYPE_UINT32, - }; - - if (NM_MORE_ASSERT_ONCE (5)) { - int i; - - for (i = 0; i < G_N_ELEMENTS (types); i++) - nm_assert (types[i] != 0); - } - nm_assert (_NM_INT_NOT_NEGATIVE (attr_id)); - nm_assert (attr_id < G_N_ELEMENTS (types)); - return types[attr_id]; -} - -/*****************************************************************************/ - -static void -_lldp_attrs_set_str (LldpAttrs *attrs, LldpAttrId attr_id, const char *v_string) -{ - LldpAttrData *pdata; - - nm_assert (attrs); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_STRING); - - pdata = &attrs->a[attr_id]; - - /* we ignore duplicate fields silently. */ - if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) - return; - pdata->attr_type = LLDP_ATTR_TYPE_STRING; - pdata->v_string = g_strdup (v_string ?: ""); -} - -static void -_lldp_attrs_set_str_take (LldpAttrs *attrs, LldpAttrId attr_id, char *str) -{ - LldpAttrData *pdata; - - nm_assert (attrs); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_STRING); - - pdata = &attrs->a[attr_id]; - - /* we ignore duplicate fields silently. */ - if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) { - g_free (str); - return; - } - - pdata->attr_type = LLDP_ATTR_TYPE_STRING; - pdata->v_string = str; -} - -static void -_lldp_attrs_set_uint32 (LldpAttrs *attrs, LldpAttrId attr_id, guint32 v_uint32) -{ - LldpAttrData *pdata; - - nm_assert (attrs); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_UINT32); - - pdata = &attrs->a[attr_id]; - - /* we ignore duplicate fields silently. */ - if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) - return; - pdata->attr_type = LLDP_ATTR_TYPE_UINT32; - pdata->v_uint32 = v_uint32; -} - -static void -_lldp_attrs_set_variant (LldpAttrs *attrs, LldpAttrId attr_id, GVariant *variant) -{ - LldpAttrData *pdata; - - nm_assert (attrs); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_VARIANT); - - pdata = &attrs->a[attr_id]; - - /* we ignore duplicate fields silently */ - if (pdata->attr_type != LLDP_ATTR_TYPE_NONE) { - nm_g_variant_unref_floating (variant); - return; - } - - pdata->attr_type = LLDP_ATTR_TYPE_VARIANT; - pdata->v_variant = g_variant_ref_sink (variant); -} - -static void -_lldp_attr_add_variant (LldpAttrs *attrs, LldpAttrId attr_id, GVariant *variant) -{ - LldpAttrData *pdata; - - nm_assert (attrs); - nm_assert (_lldp_attr_id_to_type (attr_id) == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); - - g_variant_ref_sink (variant); - - pdata = &attrs->a[attr_id]; - - if (pdata->attr_type == LLDP_ATTR_TYPE_NONE) { - pdata->attr_type = LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS; - pdata->v_variant_list.arr1 = variant; - pdata->v_variant_list.len = 1; - pdata->v_variant_list.alloc = 1; - return; - } - - nm_assert (pdata->attr_type == LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS); - - if (pdata->v_variant_list.len == pdata->v_variant_list.alloc) { - if (pdata->v_variant_list.alloc == 1) { - GVariant *arr1 = pdata->v_variant_list.arr1; - - pdata->v_variant_list.alloc = 4; - pdata->v_variant_list.arr = g_new (GVariant *, 4); - pdata->v_variant_list.arr[0] = arr1; - } else { - pdata->v_variant_list.alloc *= 2u; - pdata->v_variant_list.arr = g_realloc (pdata->v_variant_list.arr, - pdata->v_variant_list.alloc); - } - } - pdata->v_variant_list.arr[pdata->v_variant_list.len++] = g_variant_ref_sink (variant); -} - -/*****************************************************************************/ - -static void -_lldp_attrs_clear (LldpAttrs *attrs) -{ - LldpAttrId attr_id; - guint i; - - for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - LldpAttrData *pdata = &attrs->a[attr_id]; - - switch (pdata->attr_type) { - case LLDP_ATTR_TYPE_STRING: - g_free (pdata->v_string); - break; - case LLDP_ATTR_TYPE_VARIANT: - g_variant_unref (pdata->v_variant); - break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: - if (pdata->v_variant_list.alloc == 1) - g_variant_unref (pdata->v_variant_list.arr1); - else { - for (i = 0; i < pdata->v_variant_list.len; i++) - g_variant_unref (pdata->v_variant_list.arr[i]); - g_free (pdata->v_variant_list.arr); - } - break; - default: - ; - } - } -} - -/*****************************************************************************/ - static void lldp_neighbor_get_raw (LldpNeighbor *neigh, const guint8 **out_raw_data, @@ -539,181 +292,6 @@ err: return NULL; } -static void -_lldp_attrs_parse (LldpAttrs *attrs, - sd_lldp_neighbor *neighbor_sd) -{ - uint8_t *data8; - gsize len; - int r; - - r = sd_lldp_neighbor_tlv_rewind (neighbor_sd); - if (r < 0) { - nm_assert_not_reached (); - return; - } - do { - guint8 oui[3]; - guint8 type, subtype; - GVariant *variant; - - if (sd_lldp_neighbor_tlv_get_type (neighbor_sd, &type) < 0) - continue; - - if (sd_lldp_neighbor_tlv_get_raw (neighbor_sd, (void *) &data8, &len) < 0) - continue; - - switch (type) { - case SD_LLDP_TYPE_MGMT_ADDRESS: - variant = parse_management_address_tlv (data8, len); - if (variant) { - _lldp_attr_add_variant (attrs, - LLDP_ATTR_ID_MANAGEMENT_ADDRESSES, - variant); - } - continue; - case SD_LLDP_TYPE_PRIVATE: - break; - default: - continue; - } - - r = sd_lldp_neighbor_tlv_get_oui (neighbor_sd, oui, &subtype); - if (r < 0) { - if (r == -ENXIO) - continue; - - /* in other cases, something is seriously wrong. Abort, but - * keep what we parsed so far. */ - return; - } - - if ( memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) != 0 - && memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) != 0) - continue; - - /* skip over leading TLV, OUI and subtype */ -#if NM_MORE_ASSERTS > 5 - { - guint8 check_hdr[] = { - 0xfe | (((len - 2) >> 8) & 0x01), ((len - 2) & 0xFF), - oui[0], oui[1], oui[2], - subtype - }; - - nm_assert (len > 2 + 3 +1); - nm_assert (memcmp (data8, check_hdr, sizeof check_hdr) == 0); - } -#endif - if (len <= 6) - continue; - data8 += 6; - len -= 6; - - if (memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) == 0) { - GVariantDict dict; - - switch (subtype) { - case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: - if (len != 2) - continue; - _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PVID, - unaligned_read_be16 (data8)); - break; - case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID: - if (len != 3) - continue; - _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID_FLAGS, - data8[0]); - _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_PPVID, - unaligned_read_be16 (&data8[1])); - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); - - _lldp_attr_add_variant (attrs, - LLDP_ATTR_ID_IEEE_802_1_PPVIDS, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { - int l; - guint32 vid; - const char *name; - char *name_to_free; - - if (len <= 3) - continue; - - l = data8[2]; - if (len != 3 + l) - continue; - if (l > 32) - continue; - - name = nm_utils_buf_utf8safe_escape (&data8[3], l, 0, &name_to_free); - vid = unaligned_read_be16 (&data8[0]); - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "vid", "u", vid); - g_variant_dict_insert (&dict, "name", "s", name); - - _lldp_attr_add_variant (attrs, - LLDP_ATTR_ID_IEEE_802_1_VLANS, - g_variant_dict_end (&dict)); - - _lldp_attrs_set_uint32 (attrs, LLDP_ATTR_ID_IEEE_802_1_VID, vid); - if (name_to_free) - _lldp_attrs_set_str_take (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name_to_free); - else - _lldp_attrs_set_str (attrs, LLDP_ATTR_ID_IEEE_802_1_VLAN_NAME, name); - break; - } - default: - continue; - } - } else if (memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) == 0) { - GVariantDict dict; - - switch (subtype) { - case SD_LLDP_OUI_802_3_SUBTYPE_MAC_PHY_CONFIG_STATUS: - if (len != 5) - continue; - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "autoneg", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); - - _lldp_attrs_set_variant (attrs, - LLDP_ATTR_ID_IEEE_802_3_MAC_PHY_CONF, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: - if (len != 3) - continue; - - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "mdi-power-support", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); - g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); - - _lldp_attrs_set_variant (attrs, - LLDP_ATTR_ID_IEEE_802_3_POWER_VIA_MDI, - g_variant_dict_end (&dict)); - break; - case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: - if (len != 2) - continue; - _lldp_attrs_set_uint32 (attrs, - LLDP_ATTR_ID_IEEE_802_3_MAX_FRAME_SIZE, - unaligned_read_be16 (data8)); - break; - } - } - } while (sd_lldp_neighbor_tlv_next (neighbor_sd) > 0); -} - static LldpNeighbor * lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) { @@ -808,11 +386,12 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) { GVariantBuilder builder; const char *str; - LldpAttrId attr_id; const guint8 *raw_data; gsize raw_len; - LldpAttrs attrs; uint16_t u16; + uint8_t *data8; + gsize len; + int r; if (neigh->variant) return neigh->variant; @@ -853,51 +432,199 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) if (sd_lldp_neighbor_get_system_capabilities (neigh->neighbor_sd, &u16) == 0) nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_SYSTEM_CAPABILITIES, u16); - attrs = (LldpAttrs) { }; + r = sd_lldp_neighbor_tlv_rewind (neigh->neighbor_sd); + if (r < 0) + nm_assert_not_reached (); + else { + gboolean v_management_addresses_has = FALSE; + GVariantBuilder v_management_addresses; + GVariant *v_ieee_802_1_pvid = NULL; + GVariant *v_ieee_802_1_ppvid = NULL; + GVariant *v_ieee_802_1_ppvid_flags = NULL; + GVariantBuilder v_ieee_802_1_ppvids; + GVariant *v_ieee_802_1_vid = NULL; + GVariant *v_ieee_802_1_vlan_name = NULL; + GVariantBuilder v_ieee_802_1_vlans; + GVariant *v_ieee_802_3_mac_phy_conf = NULL; + GVariant *v_ieee_802_3_power_via_mdi = NULL; + GVariant *v_ieee_802_3_max_frame_size = NULL; + GVariant *tmp_variant; - _lldp_attrs_parse (&attrs, neigh->neighbor_sd); + do { + guint8 oui[3]; + guint8 type; + guint8 subtype; - for (attr_id = 0; attr_id < _LLDP_ATTR_ID_COUNT; attr_id++) { - const LldpAttrData *pdata = &attrs.a[attr_id]; + if (sd_lldp_neighbor_tlv_get_type (neigh->neighbor_sd, &type) < 0) + continue; - nm_assert (NM_IN_SET (pdata->attr_type, _lldp_attr_id_to_type (attr_id), LLDP_ATTR_TYPE_NONE)); - switch (pdata->attr_type) { - case LLDP_ATTR_TYPE_UINT32: - nm_g_variant_builder_add_sv_uint32 (&builder, - _lldp_attr_id_to_name (attr_id), - pdata->v_uint32); - break; - case LLDP_ATTR_TYPE_STRING: - nm_g_variant_builder_add_sv_str (&builder, - _lldp_attr_id_to_name (attr_id), - pdata->v_string); - break; - case LLDP_ATTR_TYPE_VARIANT: - nm_g_variant_builder_add_sv (&builder, - _lldp_attr_id_to_name (attr_id), - pdata->v_variant); - break; - case LLDP_ATTR_TYPE_ARRAY_OF_VARIANTS: { - GVariant *const*variants; + if (sd_lldp_neighbor_tlv_get_raw (neigh->neighbor_sd, (void *) &data8, &len) < 0) + continue; - if (pdata->v_variant_list.alloc == 1) - variants = &pdata->v_variant_list.arr1; - else - variants = pdata->v_variant_list.arr; - nm_g_variant_builder_add_sv (&builder, - _lldp_attr_id_to_name (attr_id), - g_variant_new_array (G_VARIANT_TYPE ("a{sv}"), - variants, - pdata->v_variant_list.len)); - break; + switch (type) { + case SD_LLDP_TYPE_MGMT_ADDRESS: + tmp_variant = parse_management_address_tlv (data8, len); + if (tmp_variant) { + if (!v_management_addresses_has) { + v_management_addresses_has = TRUE; + g_variant_builder_init (&v_management_addresses, G_VARIANT_TYPE ("aa{sv}")); + } + g_variant_builder_add_value (&v_management_addresses, tmp_variant); + } + continue; + case SD_LLDP_TYPE_PRIVATE: + break; + default: + continue; + } + + r = sd_lldp_neighbor_tlv_get_oui (neigh->neighbor_sd, oui, &subtype); + if (r < 0) { + if (r == -ENXIO) + continue; + + /* in other cases, something is seriously wrong. Abort, but + * keep what we parsed so far. */ + break; + } + + if ( memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) != 0 + && memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) != 0) + continue; + + /* skip over leading TLV, OUI and subtype */ +#if NM_MORE_ASSERTS > 5 + { + guint8 check_hdr[] = { + 0xfe | (((len - 2) >> 8) & 0x01), ((len - 2) & 0xFF), + oui[0], oui[1], oui[2], + subtype + }; + + nm_assert (len > 2 + 3 +1); + nm_assert (memcmp (data8, check_hdr, sizeof check_hdr) == 0); + } +#endif + if (len <= 6) + continue; + data8 += 6; + len -= 6; + + if (memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) == 0) { + GVariantDict dict; + + switch (subtype) { + case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: + if (len != 2) + continue; + if (!v_ieee_802_1_pvid) + v_ieee_802_1_pvid = g_variant_new_uint32 (unaligned_read_be16 (data8)); + break; + case SD_LLDP_OUI_802_1_SUBTYPE_PORT_PROTOCOL_VLAN_ID: + if (len != 3) + continue; + if (!v_ieee_802_1_ppvid) { + v_ieee_802_1_ppvid_flags = g_variant_new_uint32 (data8[0]); + v_ieee_802_1_ppvid = g_variant_new_uint32 (unaligned_read_be16 (&data8[1])); + g_variant_builder_init (&v_ieee_802_1_ppvids, G_VARIANT_TYPE ("aa{sv}")); + } + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); + g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); + g_variant_builder_add_value (&v_ieee_802_1_ppvids, g_variant_dict_end (&dict)); + break; + case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { + gs_free char *name_to_free = NULL; + const char *name; + guint32 vid; + int l; + + if (len <= 3) + continue; + + l = data8[2]; + if (len != 3 + l) + continue; + if (l > 32) + continue; + + name = nm_utils_buf_utf8safe_escape (&data8[3], l, 0, &name_to_free); + vid = unaligned_read_be16 (&data8[0]); + + if (!v_ieee_802_1_vid) { + v_ieee_802_1_vid = g_variant_new_uint32 (vid); + v_ieee_802_1_vlan_name = g_variant_new_string (name); + g_variant_builder_init (&v_ieee_802_1_vlans, G_VARIANT_TYPE ("aa{sv}")); + } + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "vid", "u", vid); + g_variant_dict_insert (&dict, "name", "s", name); + g_variant_builder_add_value (&v_ieee_802_1_vlans, g_variant_dict_end (&dict)); + break; + } + default: + continue; + } + } else if (memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) == 0) { + GVariantDict dict; + + switch (subtype) { + case SD_LLDP_OUI_802_3_SUBTYPE_MAC_PHY_CONFIG_STATUS: + if (len != 5) + continue; + + if (!v_ieee_802_3_mac_phy_conf) { + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "autoneg", "u", (guint32) data8[0]); + g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); + g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); + v_ieee_802_3_mac_phy_conf = g_variant_dict_end (&dict); + } + break; + case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: + if (len != 3) + continue; + + if (!v_ieee_802_3_power_via_mdi) { + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "mdi-power-support", "u", (guint32) data8[0]); + g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); + g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); + v_ieee_802_3_power_via_mdi = g_variant_dict_end (&dict); + } + break; + case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: + if (len != 2) + continue; + if (!v_ieee_802_3_max_frame_size) + v_ieee_802_3_max_frame_size = g_variant_new_uint32 (unaligned_read_be16 (data8)); + break; + } + } + } while (sd_lldp_neighbor_tlv_next (neigh->neighbor_sd) > 0); + + if (v_management_addresses_has) + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_MANAGEMENT_ADDRESSES, g_variant_builder_end (&v_management_addresses)); + if (v_ieee_802_1_pvid) + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_PVID, v_ieee_802_1_pvid); + if (v_ieee_802_1_ppvid) { + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_PPVID, v_ieee_802_1_ppvid); + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_PPVID_FLAGS, v_ieee_802_1_ppvid_flags); + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_PPVIDS, g_variant_builder_end (&v_ieee_802_1_ppvids)); } - case LLDP_ATTR_TYPE_NONE: - break; + if (v_ieee_802_1_vid) { + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_VID, v_ieee_802_1_vid); + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_VLAN_NAME, v_ieee_802_1_vlan_name); + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_1_VLANS, g_variant_builder_end (&v_ieee_802_1_vlans)); } + if (v_ieee_802_3_mac_phy_conf) + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_3_MAC_PHY_CONF, v_ieee_802_3_mac_phy_conf); + if (v_ieee_802_3_power_via_mdi) + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_3_POWER_VIA_MDI, v_ieee_802_3_power_via_mdi); + if (v_ieee_802_3_max_frame_size) + nm_g_variant_builder_add_sv (&builder, NM_LLDP_ATTR_IEEE_802_3_MAX_FRAME_SIZE, v_ieee_802_3_max_frame_size); } - _lldp_attrs_clear (&attrs); - return (neigh->variant = g_variant_ref_sink (g_variant_builder_end (&builder))); } From ae0da6dd6084622d25de33c436f4694038ed99e7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Jun 2020 18:15:15 +0200 Subject: [PATCH 21/28] lldp: use GVariantBuilder instead of GVariantDict GVariantDict is basically a GHashTable, and during g_variant_dict_end() it uses a GVariantBuilder to create the variant. This is totally unnecessary in this case. It's probably unnecessary in most use cases, because commonly we construct variants in a determined series of steps and don't need to add/remove keys. Aside the overhead, GHashTable also does not give a stable sort order, which seems a pretty bad property in this case. Note that the code changes the order in which we call g_variant_builder_add() for the fields in code, to preserve the previous order that GVariantDict actually created (at least, with my version of glib). --- src/devices/nm-lldp-listener.c | 96 +++++++++++++++++----------------- 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 3323125c7e..5d9bb42158 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -224,11 +224,17 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) } static GVariant * -parse_management_address_tlv (uint8_t *data, gsize len) +parse_management_address_tlv (const uint8_t *data, gsize len) { - GVariantDict dict; - GVariant *variant; - gsize addr_len, oid_len; + GVariantBuilder builder; + gsize addr_len; + const guint8 *v_object_id_arr; + gsize v_object_id_len; + const guint8 *v_address_arr; + gsize v_address_len; + guint32 v_interface_number; + guint32 v_interface_number_subtype; + guint32 v_address_subtype; /* 802.1AB-2009 - Figure 8-11 * @@ -243,7 +249,7 @@ parse_management_address_tlv (uint8_t *data, gsize len) */ if (len < 11) - goto err; + return NULL; nm_assert ((data[0] >> 1) == SD_LLDP_TYPE_MGMT_ADDRESS); nm_assert ((((data[0] & 1) << 8) + data[1]) + 2 == len); @@ -253,43 +259,42 @@ parse_management_address_tlv (uint8_t *data, gsize len) addr_len = *data; /* length of (address subtype + address) */ if (addr_len < 2 || addr_len > 32) - goto err; + return NULL; if (len < ( 1 /* address stringth length */ + addr_len /* address subtype + address */ + 5 /* interface */ + 1)) /* oid */ - goto err; - - g_variant_dict_init (&dict, NULL); + return NULL; data++; len--; - g_variant_dict_insert (&dict, "address-subtype", "u", (guint32) *data); - variant = g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, data + 1, addr_len - 1, 1); - g_variant_dict_insert_value (&dict, "address", variant); + v_address_subtype = *data; + v_address_arr = &data[1]; + v_address_len = addr_len - 1; data += addr_len; len -= addr_len; - g_variant_dict_insert (&dict, "interface-number-subtype", "u", (guint32) *data); + v_interface_number_subtype = *data; data++; len--; - g_variant_dict_insert (&dict, "interface-number", "u", unaligned_read_be32 (data)); + v_interface_number = unaligned_read_be32 (data); data += 4; len -= 4; - oid_len = *data; - - if (len < (1 + oid_len)) - goto err; - + v_object_id_len = *data; + if (len < (1 + v_object_id_len)) + return NULL; data++; - variant = g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, data, oid_len, 1); - g_variant_dict_insert_value (&dict, "object-id", variant); - return g_variant_dict_end (&dict); -err: - g_variant_dict_clear (&dict); - return NULL; + v_object_id_arr = data; + + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_uint32 (&builder, "address-subtype", v_address_subtype); + nm_g_variant_builder_add_sv_bytearray (&builder, "object-id", v_object_id_arr, v_object_id_len); + nm_g_variant_builder_add_sv_uint32 (&builder, "interface-number", v_interface_number); + nm_g_variant_builder_add_sv_bytearray (&builder, "address", v_address_arr, v_address_len); + nm_g_variant_builder_add_sv_uint32 (&builder, "interface-number-subtype", v_interface_number_subtype); + return g_variant_builder_end (&builder); } static LldpNeighbor * @@ -448,6 +453,7 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) GVariant *v_ieee_802_3_mac_phy_conf = NULL; GVariant *v_ieee_802_3_power_via_mdi = NULL; GVariant *v_ieee_802_3_max_frame_size = NULL; + GVariantBuilder tmp_builder; GVariant *tmp_variant; do { @@ -511,8 +517,6 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) len -= 6; if (memcmp (oui, SD_LLDP_OUI_802_1, sizeof (oui)) == 0) { - GVariantDict dict; - switch (subtype) { case SD_LLDP_OUI_802_1_SUBTYPE_PORT_VLAN_ID: if (len != 2) @@ -528,10 +532,10 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) v_ieee_802_1_ppvid = g_variant_new_uint32 (unaligned_read_be16 (&data8[1])); g_variant_builder_init (&v_ieee_802_1_ppvids, G_VARIANT_TYPE ("aa{sv}")); } - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "ppvid", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "flags", "u", (guint32) data8[0]); - g_variant_builder_add_value (&v_ieee_802_1_ppvids, g_variant_dict_end (&dict)); + g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "flags", data8[0]); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "ppvid", unaligned_read_be16 (&data8[1])); + g_variant_builder_add_value (&v_ieee_802_1_ppvids, g_variant_builder_end (&tmp_builder)); break; case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { gs_free char *name_to_free = NULL; @@ -556,29 +560,27 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) v_ieee_802_1_vlan_name = g_variant_new_string (name); g_variant_builder_init (&v_ieee_802_1_vlans, G_VARIANT_TYPE ("aa{sv}")); } - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "vid", "u", vid); - g_variant_dict_insert (&dict, "name", "s", name); - g_variant_builder_add_value (&v_ieee_802_1_vlans, g_variant_dict_end (&dict)); + g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "vid", vid); + nm_g_variant_builder_add_sv_str (&tmp_builder, "name", name); + g_variant_builder_add_value (&v_ieee_802_1_vlans, g_variant_builder_end (&tmp_builder)); break; } default: continue; } } else if (memcmp (oui, SD_LLDP_OUI_802_3, sizeof (oui)) == 0) { - GVariantDict dict; - switch (subtype) { case SD_LLDP_OUI_802_3_SUBTYPE_MAC_PHY_CONFIG_STATUS: if (len != 5) continue; if (!v_ieee_802_3_mac_phy_conf) { - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "autoneg", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pmd-autoneg-cap", "u", (guint32) unaligned_read_be16 (&data8[1])); - g_variant_dict_insert (&dict, "operational-mau-type", "u", (guint32) unaligned_read_be16 (&data8[3])); - v_ieee_802_3_mac_phy_conf = g_variant_dict_end (&dict); + g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "operational-mau-type", unaligned_read_be16 (&data8[3])); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "autoneg", data8[0]); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "pmd-autoneg-cap", unaligned_read_be16 (&data8[1])); + v_ieee_802_3_mac_phy_conf = g_variant_builder_end (&tmp_builder); } break; case SD_LLDP_OUI_802_3_SUBTYPE_POWER_VIA_MDI: @@ -586,11 +588,11 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) continue; if (!v_ieee_802_3_power_via_mdi) { - g_variant_dict_init (&dict, NULL); - g_variant_dict_insert (&dict, "mdi-power-support", "u", (guint32) data8[0]); - g_variant_dict_insert (&dict, "pse-power-pair", "u", (guint32) data8[1]); - g_variant_dict_insert (&dict, "power-class", "u", (guint32) data8[2]); - v_ieee_802_3_power_via_mdi = g_variant_dict_end (&dict); + g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "pse-power-pair", data8[1]); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "mdi-power-support", data8[0]); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "power-class", data8[2]); + v_ieee_802_3_power_via_mdi = g_variant_builder_end (&tmp_builder); } break; case SD_LLDP_OUI_802_3_SUBTYPE_MAXIMUM_FRAME_SIZE: From d2313bef5eebf15e4d8838080066cea4b624ec20 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 09:31:31 +0200 Subject: [PATCH 22/28] lldp: change order of dictionary fields for LLDP neighbor variant This changes the order to what the code did previously, before switching from GVariantDict to GVariantBuilder. But it changes the actually serialized order in the variant. --- src/devices/nm-lldp-listener.c | 10 +++++----- src/devices/tests/test-lldp.c | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 5d9bb42158..91aee3b3bc 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -290,10 +290,10 @@ parse_management_address_tlv (const uint8_t *data, gsize len) g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}")); nm_g_variant_builder_add_sv_uint32 (&builder, "address-subtype", v_address_subtype); - nm_g_variant_builder_add_sv_bytearray (&builder, "object-id", v_object_id_arr, v_object_id_len); - nm_g_variant_builder_add_sv_uint32 (&builder, "interface-number", v_interface_number); nm_g_variant_builder_add_sv_bytearray (&builder, "address", v_address_arr, v_address_len); nm_g_variant_builder_add_sv_uint32 (&builder, "interface-number-subtype", v_interface_number_subtype); + nm_g_variant_builder_add_sv_uint32 (&builder, "interface-number", v_interface_number); + nm_g_variant_builder_add_sv_bytearray (&builder, "object-id", v_object_id_arr, v_object_id_len); return g_variant_builder_end (&builder); } @@ -533,8 +533,8 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) g_variant_builder_init (&v_ieee_802_1_ppvids, G_VARIANT_TYPE ("aa{sv}")); } g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); - nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "flags", data8[0]); nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "ppvid", unaligned_read_be16 (&data8[1])); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "flags", data8[0]); g_variant_builder_add_value (&v_ieee_802_1_ppvids, g_variant_builder_end (&tmp_builder)); break; case SD_LLDP_OUI_802_1_SUBTYPE_VLAN_NAME: { @@ -577,9 +577,9 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) if (!v_ieee_802_3_mac_phy_conf) { g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); - nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "operational-mau-type", unaligned_read_be16 (&data8[3])); nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "autoneg", data8[0]); nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "pmd-autoneg-cap", unaligned_read_be16 (&data8[1])); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "operational-mau-type", unaligned_read_be16 (&data8[3])); v_ieee_802_3_mac_phy_conf = g_variant_builder_end (&tmp_builder); } break; @@ -589,8 +589,8 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) if (!v_ieee_802_3_power_via_mdi) { g_variant_builder_init (&tmp_builder, G_VARIANT_TYPE ("a{sv}")); - nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "pse-power-pair", data8[1]); nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "mdi-power-support", data8[0]); + nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "pse-power-pair", data8[1]); nm_g_variant_builder_add_sv_uint32 (&tmp_builder, "power-class", data8[2]); v_ieee_802_3_power_via_mdi = g_variant_builder_end (&tmp_builder); } diff --git a/src/devices/tests/test-lldp.c b/src/devices/tests/test-lldp.c index 4d563c6040..e8ab0586f2 100644 --- a/src/devices/tests/test-lldp.c +++ b/src/devices/tests/test-lldp.c @@ -170,7 +170,7 @@ TEST_RECV_DATA_DEFINE (_test_recv_data0_twice, 1, _test_recv_data0_check, &_tes TEST_RECV_FRAME_DEFINE (_test_recv_data1_frame0, /* lldp.detailed.pcap from * https://wiki.wireshark.org/SampleCaptures#Link_Layer_Discovery_Protocol_.28LLDP.29 */ - "{'raw': <[byte 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x88, 0xcc, 0x02, 0x07, 0x04, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x04, 0x04, 0x05, 0x31, 0x2f, 0x31, 0x06, 0x02, 0x00, 0x78, 0x08, 0x17, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x2d, 0x50, 0x6f, 0x72, 0x74, 0x20, 0x31, 0x30, 0x30, 0x31, 0x00, 0x0a, 0x0d, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x00, 0x0c, 0x4c, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x20, 0x2d, 0x20, 0x56, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x20, 0x37, 0x2e, 0x34, 0x65, 0x2e, 0x31, 0x20, 0x28, 0x42, 0x75, 0x69, 0x6c, 0x64, 0x20, 0x35, 0x29, 0x20, 0x62, 0x79, 0x20, 0x52, 0x65, 0x6c, 0x65, 0x61, 0x73, 0x65, 0x5f, 0x4d, 0x61, 0x73, 0x74, 0x65, 0x72, 0x20, 0x30, 0x35, 0x2f, 0x32, 0x37, 0x2f, 0x30, 0x35, 0x20, 0x30, 0x34, 0x3a, 0x35, 0x33, 0x3a, 0x31, 0x31, 0x00, 0x0e, 0x04, 0x00, 0x14, 0x00, 0x14, 0x10, 0x0e, 0x07, 0x06, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x02, 0x00, 0x00, 0x03, 0xe9, 0x00, 0xfe, 0x07, 0x00, 0x12, 0x0f, 0x02, 0x07, 0x01, 0x00, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x01, 0x03, 0x6c, 0x00, 0x00, 0x10, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x03, 0x01, 0x00, 0x00, 0x00, 0x00, 0xfe, 0x06, 0x00, 0x12, 0x0f, 0x04, 0x05, 0xf2, 0xfe, 0x06, 0x00, 0x80, 0xc2, 0x01, 0x01, 0xe8, 0xfe, 0x07, 0x00, 0x80, 0xc2, 0x02, 0x01, 0x00, 0x00, 0xfe, 0x16, 0x00, 0x80, 0xc2, 0x03, 0x01, 0xe8, 0x0f, 0x76, 0x32, 0x2d, 0x30, 0x34, 0x38, 0x38, 0x2d, 0x30, 0x33, 0x2d, 0x30, 0x35, 0x30, 0x35, 0xfe, 0x05, 0x00, 0x80, 0xc2, 0x04, 0x00, 0x00, 0x00]>, 'chassis-id-type': , 'chassis-id': <'00:01:30:F9:AD:A0'>, 'port-id-type': , 'port-id': <'1/1'>, 'destination': <'nearest-bridge'>, 'port-description': <'Summit300-48-Port 1001'>, 'system-name': <'Summit300-48'>, 'system-description': <'Summit300-48 - Version 7.4e.1 (Build 5) by Release_Master 05/27/05 04:53:11'>, 'system-capabilities': , 'management-addresses': <[{'address-subtype': , 'object-id': <@ay []>, 'interface-number': , 'address': <[byte 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0]>, 'interface-number-subtype': }]>, 'ieee-802-1-pvid': , 'ieee-802-1-ppvid': , 'ieee-802-1-ppvid-flags': , 'ieee-802-1-ppvids': <[{'flags': , 'ppvid': }]>, 'ieee-802-1-vid': , 'ieee-802-1-vlan-name': <'v2-0488-03-0505'>, 'ieee-802-1-vlans': <[{'vid': , 'name': <'v2-0488-03-0505'>}]>, 'ieee-802-3-mac-phy-conf': <{'operational-mau-type': , 'autoneg': , 'pmd-autoneg-cap': }>, 'ieee-802-3-power-via-mdi': <{'pse-power-pair': , 'mdi-power-support': , 'power-class': }>, 'ieee-802-3-max-frame-size': }", + "{'raw': <[byte 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x88, 0xcc, 0x02, 0x07, 0x04, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x04, 0x04, 0x05, 0x31, 0x2f, 0x31, 0x06, 0x02, 0x00, 0x78, 0x08, 0x17, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x2d, 0x50, 0x6f, 0x72, 0x74, 0x20, 0x31, 0x30, 0x30, 0x31, 0x00, 0x0a, 0x0d, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x00, 0x0c, 0x4c, 0x53, 0x75, 0x6d, 0x6d, 0x69, 0x74, 0x33, 0x30, 0x30, 0x2d, 0x34, 0x38, 0x20, 0x2d, 0x20, 0x56, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x20, 0x37, 0x2e, 0x34, 0x65, 0x2e, 0x31, 0x20, 0x28, 0x42, 0x75, 0x69, 0x6c, 0x64, 0x20, 0x35, 0x29, 0x20, 0x62, 0x79, 0x20, 0x52, 0x65, 0x6c, 0x65, 0x61, 0x73, 0x65, 0x5f, 0x4d, 0x61, 0x73, 0x74, 0x65, 0x72, 0x20, 0x30, 0x35, 0x2f, 0x32, 0x37, 0x2f, 0x30, 0x35, 0x20, 0x30, 0x34, 0x3a, 0x35, 0x33, 0x3a, 0x31, 0x31, 0x00, 0x0e, 0x04, 0x00, 0x14, 0x00, 0x14, 0x10, 0x0e, 0x07, 0x06, 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, 0x02, 0x00, 0x00, 0x03, 0xe9, 0x00, 0xfe, 0x07, 0x00, 0x12, 0x0f, 0x02, 0x07, 0x01, 0x00, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x01, 0x03, 0x6c, 0x00, 0x00, 0x10, 0xfe, 0x09, 0x00, 0x12, 0x0f, 0x03, 0x01, 0x00, 0x00, 0x00, 0x00, 0xfe, 0x06, 0x00, 0x12, 0x0f, 0x04, 0x05, 0xf2, 0xfe, 0x06, 0x00, 0x80, 0xc2, 0x01, 0x01, 0xe8, 0xfe, 0x07, 0x00, 0x80, 0xc2, 0x02, 0x01, 0x00, 0x00, 0xfe, 0x16, 0x00, 0x80, 0xc2, 0x03, 0x01, 0xe8, 0x0f, 0x76, 0x32, 0x2d, 0x30, 0x34, 0x38, 0x38, 0x2d, 0x30, 0x33, 0x2d, 0x30, 0x35, 0x30, 0x35, 0xfe, 0x05, 0x00, 0x80, 0xc2, 0x04, 0x00, 0x00, 0x00]>, 'chassis-id-type': , 'chassis-id': <'00:01:30:F9:AD:A0'>, 'port-id-type': , 'port-id': <'1/1'>, 'destination': <'nearest-bridge'>, 'port-description': <'Summit300-48-Port 1001'>, 'system-name': <'Summit300-48'>, 'system-description': <'Summit300-48 - Version 7.4e.1 (Build 5) by Release_Master 05/27/05 04:53:11'>, 'system-capabilities': , 'management-addresses': <[{'address-subtype': , 'address': <[byte 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0]>, 'interface-number-subtype': , 'interface-number': , 'object-id': <@ay []>}]>, 'ieee-802-1-pvid': , 'ieee-802-1-ppvid': , 'ieee-802-1-ppvid-flags': , 'ieee-802-1-ppvids': <[{'ppvid': , 'flags': }]>, 'ieee-802-1-vid': , 'ieee-802-1-vlan-name': <'v2-0488-03-0505'>, 'ieee-802-1-vlans': <[{'vid': , 'name': <'v2-0488-03-0505'>}]>, 'ieee-802-3-mac-phy-conf': <{'autoneg': , 'pmd-autoneg-cap': , 'operational-mau-type': }>, 'ieee-802-3-power-via-mdi': <{'mdi-power-support': , 'pse-power-pair': , 'power-class': }>, 'ieee-802-3-max-frame-size': }", /* ethernet header */ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e, /* destination mac */ 0x00, 0x01, 0x30, 0xf9, 0xad, 0xa0, /* source mac */ From a7aa2a5e01189622ff2776a25bba3ec1c8346a8c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 13:18:13 +0200 Subject: [PATCH 23/28] lldp: delay change notification for LLDP neighbor events We already rate limit change events by two seconds. When we notice that something changed, we call data_changed_schedule(). Previously, that would immediately issue the change notification, if ratelimiting currently was not in effect. That means, if we happen go receive two LLDP neighbor events in short succession, then the first one will trigger the change notification right away, while the second will be rate limited. Avoid that by always issue scheduling the change notification in the background. And if we currently are not rate limited, with an idle handler with low priority. --- src/devices/nm-lldp-listener.c | 36 +++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 91aee3b3bc..ff18975724 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -16,8 +16,8 @@ #include "systemd/nm-sd.h" -#define MAX_NEIGHBORS 128 -#define MIN_UPDATE_INTERVAL_NS (2 * NM_UTILS_NSEC_PER_SEC) +#define MAX_NEIGHBORS 128 +#define MIN_UPDATE_INTERVAL_NSEC (2 * NM_UTILS_NSEC_PER_SEC) #define LLDP_MAC_NEAREST_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e })) #define LLDP_MAC_NEAREST_NON_TPMR_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03 })) @@ -36,7 +36,7 @@ typedef struct { GHashTable *lldp_neighbors; /* the timestamp in nsec until which we delay updates. */ - gint64 ratelimit_next; + gint64 ratelimit_next_nsec; guint ratelimit_id; GVariant *variant; @@ -678,7 +678,7 @@ data_changed_timeout (gpointer user_data) priv = NM_LLDP_LISTENER_GET_PRIVATE (self); priv->ratelimit_id = 0; - priv->ratelimit_next = nm_utils_get_monotonic_timestamp_nsec() + MIN_UPDATE_INTERVAL_NS; + priv->ratelimit_next_nsec = nm_utils_get_monotonic_timestamp_nsec() + MIN_UPDATE_INTERVAL_NSEC; data_changed_notify (self, priv); return G_SOURCE_REMOVE; } @@ -687,18 +687,25 @@ static void data_changed_schedule (NMLldpListener *self) { NMLldpListenerPrivate *priv = NM_LLDP_LISTENER_GET_PRIVATE (self); - gint64 now; + gint64 now_nsec; - now = nm_utils_get_monotonic_timestamp_nsec (); - if (now < priv->ratelimit_next) { - if (!priv->ratelimit_id) - priv->ratelimit_id = g_timeout_add (NM_UTILS_NSEC_TO_MSEC_CEIL (priv->ratelimit_next - now), data_changed_timeout, self); + if (priv->ratelimit_id != 0) + return; + + now_nsec = nm_utils_get_monotonic_timestamp_nsec (); + if (now_nsec < priv->ratelimit_next_nsec) { + priv->ratelimit_id = g_timeout_add_full (G_PRIORITY_LOW, + NM_UTILS_NSEC_TO_MSEC_CEIL (priv->ratelimit_next_nsec - now_nsec), + data_changed_timeout, + self, + NULL); return; } - nm_clear_g_source (&priv->ratelimit_id); - priv->ratelimit_next = now + MIN_UPDATE_INTERVAL_NS; - data_changed_notify (self, priv); + priv->ratelimit_id = g_idle_add_full (G_PRIORITY_LOW, + data_changed_timeout, + self, + NULL); } static void @@ -856,12 +863,13 @@ nm_lldp_listener_stop (NMLldpListener *self) size = g_hash_table_size (priv->lldp_neighbors); g_hash_table_remove_all (priv->lldp_neighbors); - if (size || priv->ratelimit_id) + if ( size > 0 + || priv->ratelimit_id != 0) changed = TRUE; } nm_clear_g_source (&priv->ratelimit_id); - priv->ratelimit_next = 0; + priv->ratelimit_next_nsec = 0; priv->ifindex = 0; if (changed) From 0d41abea2ee24192647df9545ef210a99fd977b6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 13:28:24 +0200 Subject: [PATCH 24/28] lldp: only have GHashTable instance for LLDP neighbors when running When the instance is not running (after creation or after stop), there is no need to keep the GHashTable around. Create it when needed (during start) and clear it during stop. This makes it slightly cheaper to keep a NMLldpListener instance around, if it's currently not running. NMDevice already keeps the NMLldpListener around, even after stopping it. It's not clear whether the instance will be started again, so also clear the GHashTable. Also, one effect is that if you initially were in a network with many LLDP neibors, after stop and start, the GHashTable now gets recreated and may not need to allocate a large internal array as before. --- src/devices/nm-device.c | 6 ++++-- src/devices/nm-lldp-listener.c | 21 ++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ef4b5cf8d0..4ce27f76fc 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -3845,7 +3845,8 @@ nm_device_update_dynamic_ip_setup (NMDevice *self) /* FIXME: todo */ } - if (priv->lldp_listener && nm_lldp_listener_is_running (priv->lldp_listener)) { + if ( priv->lldp_listener + && nm_lldp_listener_is_running (priv->lldp_listener)) { nm_lldp_listener_stop (priv->lldp_listener); if (!nm_lldp_listener_start (priv->lldp_listener, nm_device_get_ifindex (self), &error)) { _LOGD (LOGD_DEVICE, "LLDP listener %p could not be restarted: %s", @@ -7157,7 +7158,8 @@ lldp_init (NMDevice *self, gboolean restart) gs_free_error GError *error = NULL; if (priv->lldp_listener) { - if (restart && nm_lldp_listener_is_running (priv->lldp_listener)) + if ( restart + && nm_lldp_listener_is_running (priv->lldp_listener)) nm_lldp_listener_stop (priv->lldp_listener); } else { priv->lldp_listener = nm_lldp_listener_new (); diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index ff18975724..ee443d002c 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -30,16 +30,15 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMLldpListener, ); typedef struct { - char *iface; - int ifindex; sd_lldp *lldp_handle; GHashTable *lldp_neighbors; + GVariant *variant; /* the timestamp in nsec until which we delay updates. */ gint64 ratelimit_next_nsec; guint ratelimit_id; - GVariant *variant; + int ifindex; } NMLldpListenerPrivate; struct _NMLldpListener { @@ -724,6 +723,8 @@ process_lldp_neighbor (NMLldpListener *self, sd_lldp_neighbor *neighbor_sd, gboo g_return_if_fail (priv->lldp_handle); g_return_if_fail (neighbor_sd); + nm_assert (priv->lldp_neighbors); + p_parse_error = _LOGT_ENABLED () ? &parse_error : NULL; neigh = lldp_neighbor_new (neighbor_sd, p_parse_error); @@ -831,6 +832,10 @@ nm_lldp_listener_start (NMLldpListener *self, int ifindex, GError **error) goto err; } + priv->lldp_neighbors = g_hash_table_new_full (lldp_neighbor_id_hash, + lldp_neighbor_id_equal, + (GDestroyNotify) lldp_neighbor_free, NULL); + _LOGD ("start"); return TRUE; @@ -863,6 +868,7 @@ nm_lldp_listener_stop (NMLldpListener *self) size = g_hash_table_size (priv->lldp_neighbors); g_hash_table_remove_all (priv->lldp_neighbors); + nm_clear_pointer (&priv->lldp_neighbors, g_hash_table_unref); if ( size > 0 || priv->ratelimit_id != 0) changed = TRUE; @@ -897,8 +903,8 @@ nm_lldp_listener_get_neighbors (NMLldpListener *self) priv = NM_LLDP_LISTENER_GET_PRIVATE (self); if (G_UNLIKELY (!priv->variant)) { - GVariantBuilder array_builder; gs_free LldpNeighbor **neighbors = NULL; + GVariantBuilder array_builder; guint i, n; g_variant_builder_init (&array_builder, G_VARIANT_TYPE ("aa{sv}")); @@ -932,12 +938,6 @@ get_property (GObject *object, guint prop_id, static void nm_lldp_listener_init (NMLldpListener *self) { - NMLldpListenerPrivate *priv = NM_LLDP_LISTENER_GET_PRIVATE (self); - - priv->lldp_neighbors = g_hash_table_new_full (lldp_neighbor_id_hash, - lldp_neighbor_id_equal, - (GDestroyNotify) lldp_neighbor_free, NULL); - _LOGT ("lldp listener created"); } @@ -962,7 +962,6 @@ finalize (GObject *object) NMLldpListenerPrivate *priv = NM_LLDP_LISTENER_GET_PRIVATE (self); nm_lldp_listener_stop (self); - g_hash_table_unref (priv->lldp_neighbors); nm_clear_g_variant (&priv->variant); From e7955f577e668e2f3214c1f187ae179a4adbdd47 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 13:47:21 +0200 Subject: [PATCH 25/28] lldp: use nm_utils_ether_addr_equal() instead of re-implementation --- src/devices/nm-lldp-listener.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index ee443d002c..6db59d47e8 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -19,9 +19,9 @@ #define MAX_NEIGHBORS 128 #define MIN_UPDATE_INTERVAL_NSEC (2 * NM_UTILS_NSEC_PER_SEC) -#define LLDP_MAC_NEAREST_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e })) -#define LLDP_MAC_NEAREST_NON_TPMR_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03 })) -#define LLDP_MAC_NEAREST_CUSTOMER_BRIDGE ((const struct ether_addr *) ((uint8_t[ETH_ALEN]) { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 })) +#define LLDP_MAC_NEAREST_BRIDGE (&((struct ether_addr) { .ether_addr_octet = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x0e } })) +#define LLDP_MAC_NEAREST_NON_TPMR_BRIDGE (&((struct ether_addr) { .ether_addr_octet = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03 } })) +#define LLDP_MAC_NEAREST_CUSTOMER_BRIDGE (&((struct ether_addr) { .ether_addr_octet = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 } })) /*****************************************************************************/ @@ -102,18 +102,6 @@ typedef struct { /*****************************************************************************/ -static gboolean -ether_addr_equal (const struct ether_addr *a1, const struct ether_addr *a2) -{ - nm_assert (a1); - nm_assert (a2); - - G_STATIC_ASSERT_EXPR (sizeof (*a1) == ETH_ALEN); - return memcmp (a1, a2, ETH_ALEN) == 0; -} - -/*****************************************************************************/ - static void lldp_neighbor_get_raw (LldpNeighbor *neigh, const guint8 **out_raw_data, @@ -216,7 +204,7 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) nm_assert ( !equal || ( a->chassis_id_type == b->chassis_id_type && a->port_id_type == b->port_id_type - && ether_addr_equal (&a->destination_address, &b->destination_address) + && nm_utils_ether_addr_equal (&a->destination_address, &b->destination_address) && nm_streq0 (a->chassis_id, b->chassis_id) && nm_streq0 (a->port_id, b->port_id))); return equal; @@ -413,11 +401,11 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_PORT_ID_TYPE, neigh->port_id_type); nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_PORT_ID, neigh->port_id); - if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_BRIDGE)) + if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_BRIDGE)) str = NM_LLDP_DEST_NEAREST_BRIDGE; - else if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_NON_TPMR_BRIDGE)) + else if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_NON_TPMR_BRIDGE)) str = NM_LLDP_DEST_NEAREST_NON_TPMR_BRIDGE; - else if (ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_CUSTOMER_BRIDGE)) + else if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_CUSTOMER_BRIDGE)) str = NM_LLDP_DEST_NEAREST_CUSTOMER_BRIDGE; else str = NULL; From bf3bd1cff1b4f0f9209f5e5cf2b20cfad3a3f9e9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 15:21:55 +0200 Subject: [PATCH 26/28] lldp: parse destination-address on demand An invalid destination address doesn't need to break the LLDL neighbor entirely. In fact, systemd will already filter out such addresses. So in practice, the neighbor always has a valid destination address. There is thus no need to parse it already during lldp_neighbor_new(). --- src/devices/nm-lldp-listener.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 6db59d47e8..855585cb05 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -63,8 +63,6 @@ typedef struct { sd_lldp_neighbor *neighbor_sd; - struct ether_addr destination_address; - guint8 chassis_id_type; guint8 port_id_type; @@ -204,7 +202,6 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) nm_assert ( !equal || ( a->chassis_id_type == b->chassis_id_type && a->port_id_type == b->port_id_type - && nm_utils_ether_addr_equal (&a->destination_address, &b->destination_address) && nm_streq0 (a->chassis_id, b->chassis_id) && nm_streq0 (a->port_id, b->port_id))); return equal; @@ -326,13 +323,6 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) .neighbor_sd = sd_lldp_neighbor_ref (neighbor_sd), }; - r = sd_lldp_neighbor_get_destination_address (neighbor_sd, &neigh->destination_address); - if (r < 0) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failed getting destination address: %s", nm_strerror_native (-r)); - goto out; - } - switch (chassis_id_type) { case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_ALIAS: case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME: @@ -376,6 +366,7 @@ out: static GVariant * lldp_neighbor_to_variant (LldpNeighbor *neigh) { + struct ether_addr destination_address; GVariantBuilder builder; const char *str; const guint8 *raw_data; @@ -401,11 +392,14 @@ lldp_neighbor_to_variant (LldpNeighbor *neigh) nm_g_variant_builder_add_sv_uint32 (&builder, NM_LLDP_ATTR_PORT_ID_TYPE, neigh->port_id_type); nm_g_variant_builder_add_sv_str (&builder, NM_LLDP_ATTR_PORT_ID, neigh->port_id); - if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_BRIDGE)) + r = sd_lldp_neighbor_get_destination_address (neigh->neighbor_sd, &destination_address); + if (r < 0) + str = NULL; + else if (nm_utils_ether_addr_equal (&destination_address, LLDP_MAC_NEAREST_BRIDGE)) str = NM_LLDP_DEST_NEAREST_BRIDGE; - else if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_NON_TPMR_BRIDGE)) + else if (nm_utils_ether_addr_equal (&destination_address, LLDP_MAC_NEAREST_NON_TPMR_BRIDGE)) str = NM_LLDP_DEST_NEAREST_NON_TPMR_BRIDGE; - else if (nm_utils_ether_addr_equal (&neigh->destination_address, LLDP_MAC_NEAREST_CUSTOMER_BRIDGE)) + else if (nm_utils_ether_addr_equal (&destination_address, LLDP_MAC_NEAREST_CUSTOMER_BRIDGE)) str = NM_LLDP_DEST_NEAREST_CUSTOMER_BRIDGE; else str = NULL; From 58f738fa3bf99accc9a85cc7e8146c42b3c221db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 15:18:27 +0200 Subject: [PATCH 27/28] lldp: use full chassis-id/port-id as ID for LLDP neighbor For the ID of LLDP neighbors follow what systemd does (w.r.t. what it consideres equality of two neighbors). Note that previously we did almost the same thing. Except, we compared priv->chassis_id and priv->port_id, but these values are string representations of the original (binary value). Don't use the pretty strings as ID but the original binary value. --- src/devices/nm-lldp-listener.c | 191 ++++++++++++++++++++------------- 1 file changed, 114 insertions(+), 77 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 855585cb05..77cf44d120 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -58,15 +58,11 @@ G_DEFINE_TYPE (NMLldpListener, nm_lldp_listener, G_TYPE_OBJECT) typedef struct { GVariant *variant; + sd_lldp_neighbor *neighbor_sd; char *chassis_id; char *port_id; - - sd_lldp_neighbor *neighbor_sd; - guint8 chassis_id_type; guint8 port_id_type; - - bool valid:1; } LldpNeighbor; /*****************************************************************************/ @@ -121,29 +117,96 @@ lldp_neighbor_get_raw (LldpNeighbor *neigh, *out_raw_len = raw_len; } +static gboolean +lldp_neighbor_id_get (struct sd_lldp_neighbor *neighbor_sd, + guint8 *out_chassis_id_type, + const guint8 **out_chassis_id, + gsize *out_chassis_id_len, + guint8 *out_port_id_type, + const guint8 **out_port_id, + gsize *out_port_id_len) +{ + int r; + + r = sd_lldp_neighbor_get_chassis_id (neighbor_sd, + out_chassis_id_type, + (gconstpointer *) out_chassis_id, + out_chassis_id_len); + if (r < 0) + return FALSE; + + r = sd_lldp_neighbor_get_port_id (neighbor_sd, + out_port_id_type, + (gconstpointer *) out_port_id, + out_port_id_len); + if (r < 0) + return FALSE; + + return TRUE; +} + static guint lldp_neighbor_id_hash (gconstpointer ptr) { const LldpNeighbor *neigh = ptr; + guint8 chassis_id_type; + guint8 port_id_type; + const guint8 *chassis_id; + const guint8 *port_id; + gsize chassis_id_len; + gsize port_id_len; NMHashState h; + if (!lldp_neighbor_id_get (neigh->neighbor_sd, &chassis_id_type, &chassis_id, &chassis_id_len, &port_id_type, &port_id, &port_id_len)) { + nm_assert_not_reached (); + return 0; + } + nm_hash_init (&h, 23423423u); - nm_hash_update_str0 (&h, neigh->chassis_id); - nm_hash_update_str0 (&h, neigh->port_id); nm_hash_update_vals (&h, - neigh->chassis_id_type, - neigh->port_id_type); + chassis_id_len, + port_id_len, + chassis_id_type, + port_id_type); + nm_hash_update (&h, chassis_id, chassis_id_len); + nm_hash_update (&h, port_id, port_id_len); return nm_hash_complete (&h); } static int -lldp_neighbor_id_cmp (const LldpNeighbor *x, const LldpNeighbor *y) +lldp_neighbor_id_cmp (const LldpNeighbor *a, const LldpNeighbor *b) { - NM_CMP_SELF (x, y); - NM_CMP_FIELD (x, y, chassis_id_type); - NM_CMP_FIELD (x, y, port_id_type); - NM_CMP_FIELD_STR0 (x, y, chassis_id); - NM_CMP_FIELD_STR0 (x, y, port_id); + guint8 a_chassis_id_type; + guint8 b_chassis_id_type; + guint8 a_port_id_type; + guint8 b_port_id_type; + const guint8 *a_chassis_id; + const guint8 *b_chassis_id; + const guint8 *a_port_id; + const guint8 *b_port_id; + gsize a_chassis_id_len; + gsize b_chassis_id_len; + gsize a_port_id_len; + gsize b_port_id_len; + + NM_CMP_SELF (a, b); + + if (!lldp_neighbor_id_get (a->neighbor_sd, &a_chassis_id_type, &a_chassis_id, &a_chassis_id_len, &a_port_id_type, &a_port_id, &a_port_id_len)) { + nm_assert_not_reached (); + return FALSE; + } + + if (!lldp_neighbor_id_get (b->neighbor_sd, &b_chassis_id_type, &b_chassis_id, &b_chassis_id_len, &b_port_id_type, &b_port_id, &b_port_id_len)) { + nm_assert_not_reached (); + return FALSE; + } + + NM_CMP_DIRECT (a_chassis_id_type, b_chassis_id_type); + NM_CMP_DIRECT (a_port_id_type, b_port_id_type); + NM_CMP_DIRECT (a_chassis_id_len, b_chassis_id_len); + NM_CMP_DIRECT (a_port_id_len, b_port_id_len); + NM_CMP_DIRECT_MEMCMP (a_chassis_id, b_chassis_id, a_chassis_id_len); + NM_CMP_DIRECT_MEMCMP (a_port_id, b_port_id, a_port_id_len); return 0; } @@ -186,25 +249,14 @@ lldp_neighbor_equal (LldpNeighbor *a, LldpNeighbor *b) const guint8 *raw_data_b; gsize raw_len_a; gsize raw_len_b; - gboolean equal; if (a->neighbor_sd == b->neighbor_sd) return TRUE; lldp_neighbor_get_raw (a, &raw_data_a, &raw_len_a); lldp_neighbor_get_raw (b, &raw_data_b, &raw_len_b); - - if (raw_len_a != raw_len_b) - return FALSE; - - equal = (memcmp (raw_data_a, raw_data_b, raw_len_a) == 0); - - nm_assert ( !equal - || ( a->chassis_id_type == b->chassis_id_type - && a->port_id_type == b->port_id_type - && nm_streq0 (a->chassis_id, b->chassis_id) - && nm_streq0 (a->port_id, b->port_id))); - return equal; + return raw_len_a == raw_len_b + && (memcmp (raw_data_a, raw_data_b, raw_len_a) == 0); } static GVariant * @@ -284,60 +336,43 @@ parse_management_address_tlv (const uint8_t *data, gsize len) static LldpNeighbor * lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) { - nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL; - uint8_t chassis_id_type, port_id_type; - const void *chassis_id, *port_id; - gsize chassis_id_len, port_id_len; - int r; + LldpNeighbor *neigh; + guint8 chassis_id_type; + guint8 port_id_type; + const guint8 *chassis_id; + const guint8 *port_id; + gsize chassis_id_len; + gsize port_id_len; + gs_free char *s_chassis_id = NULL; + gs_free char *s_port_id = NULL; - r = sd_lldp_neighbor_get_chassis_id (neighbor_sd, &chassis_id_type, - &chassis_id, &chassis_id_len); - if (r < 0) { + if (!lldp_neighbor_id_get (neighbor_sd, + &chassis_id_type, + &chassis_id, + &chassis_id_len, + &port_id_type, + &port_id, + &port_id_len)) { g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failed reading chassis-id: %s", nm_strerror_native (-r)); + "invalid LLDP neighbor"); return NULL; } - if (chassis_id_len < 1) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "empty chassis-id"); - return NULL; - } - - r = sd_lldp_neighbor_get_port_id (neighbor_sd, &port_id_type, - &port_id, &port_id_len); - if (r < 0) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "failed reading port-id: %s", nm_strerror_native (-r)); - return NULL; - } - if (port_id_len < 1) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "empty port-id"); - return NULL; - } - - neigh = g_slice_new (LldpNeighbor); - *neigh = (LldpNeighbor) { - .chassis_id_type = chassis_id_type, - .port_id_type = port_id_type, - .neighbor_sd = sd_lldp_neighbor_ref (neighbor_sd), - }; switch (chassis_id_type) { case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_ALIAS: case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME: case SD_LLDP_CHASSIS_SUBTYPE_LOCALLY_ASSIGNED: case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT: - neigh->chassis_id = nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) - ?: g_new0 (char, 1); + s_chassis_id = nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) + ?: g_new0 (char, 1); break; case SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS: - neigh->chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len); + s_chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len); break; default: g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, "unsupported chassis-id type %d", chassis_id_type); - goto out; + return NULL; } switch (port_id_type) { @@ -345,22 +380,27 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) case SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME: case SD_LLDP_PORT_SUBTYPE_LOCALLY_ASSIGNED: case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT: - neigh->port_id = nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) - ?: g_new0 (char, 1); + s_port_id = nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) + ?: g_new0 (char, 1); break; case SD_LLDP_PORT_SUBTYPE_MAC_ADDRESS: - neigh->port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len); + s_port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len); break; default: g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, "unsupported port-id type %d", port_id_type); - goto out; + return NULL; } - neigh->valid = TRUE; - -out: - return g_steal_pointer (&neigh); + neigh = g_slice_new (LldpNeighbor); + *neigh = (LldpNeighbor) { + .neighbor_sd = sd_lldp_neighbor_ref (neighbor_sd), + .chassis_id_type = chassis_id_type, + .chassis_id = g_steal_pointer (&s_chassis_id), + .port_id_type = port_id_type, + .port_id = g_steal_pointer (&s_port_id), + }; + return neigh; } static GVariant * @@ -715,9 +755,6 @@ process_lldp_neighbor (NMLldpListener *self, sd_lldp_neighbor *neighbor_sd, gboo return; } - if (!neigh->valid) - neighbor_valid = FALSE; - neigh_old = g_hash_table_lookup (priv->lldp_neighbors, neigh); if (neigh_old) { if (!neighbor_valid) { From 0bc6f6a197ad6aef0ec27a7de060fb91714f78d0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 8 Jun 2020 16:55:40 +0200 Subject: [PATCH 28/28] lldp: accept all chassis-id/port-id types and support network-address Do what systemd does with sd_lldp_neighbor_get_chassis_id_as_string() and sd_lldp_neighbor_get_port_id_as_string(). Maybe we should use the systemd functions directly, however that is not done because the way how we convert the values to string is part of our stable API. Let's not rely on systemd for that. Also, support SD_LLDP_CHASSIS_SUBTYPE_NETWORK_ADDRESS and SD_LLDP_PORT_SUBTYPE_NETWORK_ADDRESS types. Use the same formatting scheme as systemd ([1]) and lldpd ([2]). [1] https://github.com/systemd/systemd/blob/a07e962549bc900365627482834896ea98996ff4/src/libsystemd-network/lldp-neighbor.c#L422 [2] https://github.com/vincentbernat/lldpd/blob/d21599d2e6fa08dcf4a0b49e0b9bc35c52311286/src/lib/atoms/chassis.c#L125 Also, in case we don't support the type or the type contains unexpected data, fallback to still expose the LLDP neighbor, and convert the value to a hex string (like systemd does). This means, lldp_neighbor_new() in practice can no longer fail and the error handling for that can be dropped. There is one tiny problem: now as fallback we expose the chassis-id/port-id as hex string. That means, if we in the future recognize a new type, we will have to change API for those types. The alternative would be to either hide the neighbor completely from the D-Bus API (as previously done), or not expose the hex strings on D-Bus. Neither seems very attractive, so expose the value (and reserve the right to change API in the future). --- src/devices/nm-lldp-listener.c | 107 +++++++++++++++++++-------------- 1 file changed, 63 insertions(+), 44 deletions(-) diff --git a/src/devices/nm-lldp-listener.c b/src/devices/nm-lldp-listener.c index 77cf44d120..e642305cea 100644 --- a/src/devices/nm-lldp-listener.c +++ b/src/devices/nm-lldp-listener.c @@ -91,8 +91,8 @@ typedef struct { } \ } G_STMT_END \ -#define LOG_NEIGH_FMT "CHASSIS=%s%s%s PORT=%s%s%s" -#define LOG_NEIGH_ARG(neigh) NM_PRINT_FMT_QUOTE_STRING ((neigh)->chassis_id), NM_PRINT_FMT_QUOTE_STRING ((neigh)->port_id) +#define LOG_NEIGH_FMT "CHASSIS=%u/%s PORT=%u/%s" +#define LOG_NEIGH_ARG(neigh) (neigh)->chassis_id_type, (neigh)->chassis_id, (neigh)->port_id_type, (neigh)->port_id /*****************************************************************************/ @@ -333,8 +333,28 @@ parse_management_address_tlv (const uint8_t *data, gsize len) return g_variant_builder_end (&builder); } +static char * +format_network_address (const guint8 *data, gsize sz) +{ + NMIPAddr a; + int family; + + if ( sz == 5 + && data[0] == 1 /* LLDP_MGMT_ADDR_IP4 */) { + memcpy (&a, &data[1], sizeof (a.addr4)); + family = AF_INET; + } else if ( sz == 17 + && data[0] == 2 /* LLDP_MGMT_ADDR_IP6 */) { + memcpy (&a, &data[1], sizeof (a.addr6)); + family = AF_INET6; + } else + return NULL; + + return nm_utils_inet_ntop_dup (family, &a); +} + static LldpNeighbor * -lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) +lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd) { LldpNeighbor *neigh; guint8 chassis_id_type; @@ -352,44 +372,50 @@ lldp_neighbor_new (sd_lldp_neighbor *neighbor_sd, GError **error) &chassis_id_len, &port_id_type, &port_id, - &port_id_len)) { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "invalid LLDP neighbor"); + &port_id_len)) return NULL; - } switch (chassis_id_type) { + case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT: case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_ALIAS: + case SD_LLDP_CHASSIS_SUBTYPE_PORT_COMPONENT: case SD_LLDP_CHASSIS_SUBTYPE_INTERFACE_NAME: case SD_LLDP_CHASSIS_SUBTYPE_LOCALLY_ASSIGNED: - case SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT: - s_chassis_id = nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) - ?: g_new0 (char, 1); + s_chassis_id = nm_utils_buf_utf8safe_escape_cp (chassis_id, chassis_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII); break; case SD_LLDP_CHASSIS_SUBTYPE_MAC_ADDRESS: s_chassis_id = nm_utils_hwaddr_ntoa (chassis_id, chassis_id_len); break; - default: - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "unsupported chassis-id type %d", chassis_id_type); - return NULL; + case SD_LLDP_CHASSIS_SUBTYPE_NETWORK_ADDRESS: + s_chassis_id = format_network_address (chassis_id, chassis_id_len); + break; + } + if (!s_chassis_id) { + /* Invalid/unsupported chassis_id? Expose as hex string. This format is not stable, and + * in the future we may add a better string representation for these case (thus + * changing the API). */ + s_chassis_id = nm_utils_bin2hexstr_full (chassis_id, chassis_id_len, '\0', FALSE, NULL); } switch (port_id_type) { case SD_LLDP_PORT_SUBTYPE_INTERFACE_ALIAS: + case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT: case SD_LLDP_PORT_SUBTYPE_INTERFACE_NAME: case SD_LLDP_PORT_SUBTYPE_LOCALLY_ASSIGNED: - case SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT: - s_port_id = nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII) - ?: g_new0 (char, 1); + s_port_id = nm_utils_buf_utf8safe_escape_cp (port_id, port_id_len, NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_CTRL | NM_UTILS_STR_UTF8_SAFE_FLAG_ESCAPE_NON_ASCII); break; case SD_LLDP_PORT_SUBTYPE_MAC_ADDRESS: s_port_id = nm_utils_hwaddr_ntoa (port_id, port_id_len); break; - default: - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "unsupported port-id type %d", port_id_type); - return NULL; + case SD_LLDP_PORT_SUBTYPE_NETWORK_ADDRESS: + s_port_id = format_network_address (port_id, port_id_len); + break; + } + if (!s_port_id) { + /* Invalid/unsupported port_id? Expose as hex string. This format is not stable, and + * in the future we may add a better string representation for these case (thus + * changing the API). */ + s_port_id = nm_utils_bin2hexstr_full (port_id, port_id_len, '\0', FALSE, NULL); } neigh = g_slice_new (LldpNeighbor); @@ -659,7 +685,6 @@ nmtst_lldp_parse_from_raw (const guint8 *raw_data, { nm_auto (sd_lldp_neighbor_unrefp) sd_lldp_neighbor *neighbor_sd = NULL; nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL; - gs_free_error GError *error = NULL; GVariant *variant; int r; @@ -669,9 +694,8 @@ nmtst_lldp_parse_from_raw (const guint8 *raw_data, r = sd_lldp_neighbor_from_raw (&neighbor_sd, raw_data, raw_len); g_assert (r >= 0); - neigh = lldp_neighbor_new (neighbor_sd, &error); + neigh = lldp_neighbor_new (neighbor_sd); g_assert (neigh); - g_assert (!error); variant = lldp_neighbor_to_variant (neigh); g_assert (variant); @@ -730,13 +754,11 @@ data_changed_schedule (NMLldpListener *self) } static void -process_lldp_neighbor (NMLldpListener *self, sd_lldp_neighbor *neighbor_sd, gboolean neighbor_valid) +process_lldp_neighbor (NMLldpListener *self, sd_lldp_neighbor *neighbor_sd, gboolean remove) { NMLldpListenerPrivate *priv; nm_auto (lldp_neighbor_freep) LldpNeighbor *neigh = NULL; LldpNeighbor *neigh_old; - gs_free_error GError *parse_error = NULL; - GError **p_parse_error; g_return_if_fail (NM_IS_LLDP_LISTENER (self)); @@ -747,32 +769,29 @@ process_lldp_neighbor (NMLldpListener *self, sd_lldp_neighbor *neighbor_sd, gboo nm_assert (priv->lldp_neighbors); - p_parse_error = _LOGT_ENABLED () ? &parse_error : NULL; - - neigh = lldp_neighbor_new (neighbor_sd, p_parse_error); + neigh = lldp_neighbor_new (neighbor_sd); if (!neigh) { - _LOGT ("process: failed to parse neighbor: %s", parse_error->message); + _LOGT ("process: failed to parse neighbor"); return; } neigh_old = g_hash_table_lookup (priv->lldp_neighbors, neigh); - if (neigh_old) { - if (!neighbor_valid) { - _LOGT ("process: %s neigh: "LOG_NEIGH_FMT"%s%s%s", - "remove", LOG_NEIGH_ARG (neigh), - NM_PRINT_FMT_QUOTED (parse_error, " (failed to parse: ", parse_error->message, ")", "")); + + if (remove) { + if (neigh_old) { + _LOGT ("process: %s neigh: "LOG_NEIGH_FMT, + "remove", LOG_NEIGH_ARG (neigh)); g_hash_table_remove (priv->lldp_neighbors, neigh_old); goto handle_changed; } - if (lldp_neighbor_equal (neigh_old, neigh)) - return; - } else if (!neighbor_valid) { - if (parse_error) - _LOGT ("process: failed to parse neighbor: %s", parse_error->message); return; } + if ( neigh_old + && lldp_neighbor_equal (neigh_old, neigh)) + return; + _LOGD ("process: %s neigh: "LOG_NEIGH_FMT, neigh_old ? "update" : "new", LOG_NEIGH_ARG (neigh)); @@ -788,9 +807,9 @@ lldp_event_handler (sd_lldp *lldp, sd_lldp_event event, sd_lldp_neighbor *n, voi { process_lldp_neighbor (userdata, n, - NM_IN_SET (event, SD_LLDP_EVENT_ADDED, - SD_LLDP_EVENT_UPDATED, - SD_LLDP_EVENT_REFRESHED)); + !NM_IN_SET (event, SD_LLDP_EVENT_ADDED, + SD_LLDP_EVENT_UPDATED, + SD_LLDP_EVENT_REFRESHED)); } gboolean