From dd102e4d0c6551e9d39c3bdb6715eceea57c8761 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Wed, 5 Dec 2018 23:58:58 -0800 Subject: [PATCH 1/2] lldp: add test coverage for sd_lldp_get_neighbors() with multiple neighbors In particular, check that the order of the results is consistent. This test coverage will be useful in order to refactor the compare_func used while sorting the results. Tested: ninja -C build/ test --- src/libsystemd-network/test-lldp.c | 130 +++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/src/libsystemd-network/test-lldp.c b/src/libsystemd-network/test-lldp.c index e0d4d9a858..cb4545d901 100644 --- a/src/libsystemd-network/test-lldp.c +++ b/src/libsystemd-network/test-lldp.c @@ -229,6 +229,135 @@ static void test_receive_oui_packet(sd_event *e) { assert_se(stop_lldp(lldp) == 0); } +static void test_multiple_neighbors_sorted(sd_event *e) { + + static const uint8_t frame1[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x04, 0x01, '1', '/', '2', /* Chassis component: "1/2" */ + 0x04, 0x04, 0x02, '2', '/', '3', /* Port component: "2/3" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const uint8_t frame2[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x04, 0x01, '2', '/', '1', /* Chassis component: "2/1" */ + 0x04, 0x04, 0x02, '1', '/', '3', /* Port component: "1/3" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const uint8_t frame3[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x05, 0x01, '2', '/', '1', '0', /* Chassis component: "2/10" */ + 0x04, 0x04, 0x02, '1', '/', '0', /* Port component: "1/0" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const uint8_t frame4[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x05, 0x01, '2', '/', '1', '9', /* Chassis component: "2/19" */ + 0x04, 0x04, 0x02, '1', '/', '0', /* Port component: "1/0" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const uint8_t frame5[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x04, 0x01, '1', '/', '2', /* Chassis component: "1/2" */ + 0x04, 0x05, 0x02, '2', '/', '1', '0', /* Port component: "2/10" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const uint8_t frame6[] = { + /* Ethernet header */ + 0x01, 0x80, 0xc2, 0x00, 0x00, 0x03, /* Destination MAC */ + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, /* Source MAC */ + 0x88, 0xcc, /* Ethertype */ + /* LLDP mandatory TLVs */ + 0x02, 0x04, 0x01, '1', '/', '2', /* Chassis component: "1/2" */ + 0x04, 0x05, 0x02, '2', '/', '3', '9', /* Port component: "2/10" */ + 0x06, 0x02, 0x00, 0x78, /* TTL: 120 seconds */ + 0x00, 0x00 /* End Of LLDPDU */ + }; + static const char* expected[] = { + /* ordered pairs of Chassis+Port */ + "1/2", "2/10", + "1/2", "2/3", + "1/2", "2/39", + "2/1", "1/3", + "2/10", "1/0", + "2/19", "1/0", + }; + + sd_lldp *lldp; + sd_lldp_neighbor **neighbors; + int i; + uint8_t type; + const void *data; + size_t length, expected_length; + uint16_t ttl; + + lldp_handler_calls = 0; + assert_se(start_lldp(&lldp, e, lldp_handler, NULL) == 0); + + assert_se(write(test_fd[1], frame1, sizeof(frame1)) == sizeof(frame1)); + sd_event_run(e, 0); + assert_se(write(test_fd[1], frame2, sizeof(frame2)) == sizeof(frame2)); + sd_event_run(e, 0); + assert_se(write(test_fd[1], frame3, sizeof(frame3)) == sizeof(frame3)); + sd_event_run(e, 0); + assert_se(write(test_fd[1], frame4, sizeof(frame4)) == sizeof(frame4)); + sd_event_run(e, 0); + assert_se(write(test_fd[1], frame5, sizeof(frame5)) == sizeof(frame5)); + sd_event_run(e, 0); + assert_se(write(test_fd[1], frame6, sizeof(frame6)) == sizeof(frame6)); + sd_event_run(e, 0); + assert_se(lldp_handler_calls == 6); + + assert_se(sd_lldp_get_neighbors(lldp, &neighbors) == 6); + + for (i = 0; i < 6; i++) { + assert_se(sd_lldp_neighbor_get_chassis_id(neighbors[i], &type, &data, &length) == 0); + assert_se(type == SD_LLDP_CHASSIS_SUBTYPE_CHASSIS_COMPONENT); + expected_length = strlen(expected[2 * i]); + assert_se(length == expected_length); + assert_se(memcmp(data, expected[2 * i], expected_length) == 0); + + assert_se(sd_lldp_neighbor_get_port_id(neighbors[i], &type, &data, &length) == 0); + assert_se(type == SD_LLDP_PORT_SUBTYPE_PORT_COMPONENT); + expected_length = strlen(expected[2 * i + 1]); + assert_se(length == expected_length); + assert_se(memcmp(data, expected[2 * i + 1], expected_length) == 0); + + assert_se(sd_lldp_neighbor_get_ttl(neighbors[i], &ttl) == 0); + assert_se(ttl == 120); + } + + for (i = 0; i < 6; i++) + sd_lldp_neighbor_unref(neighbors[i]); + free(neighbors); + + assert_se(stop_lldp(lldp) == 0); +} + int main(int argc, char *argv[]) { _cleanup_(sd_event_unrefp) sd_event *e = NULL; @@ -239,6 +368,7 @@ int main(int argc, char *argv[]) { test_receive_basic_packet(e); test_receive_incomplete_packet(e); test_receive_oui_packet(e); + test_multiple_neighbors_sorted(e); return 0; } From dc6bf94d68c38bd20d0abb94a4440441e13eaba3 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Thu, 6 Dec 2018 00:02:51 -0800 Subject: [PATCH 2/2] lldp: simplify compare_func, using ?: to chain comparisons The ?: operator is very useful for chaining comparison functions (strcmp, memcmp, CMP), since its behavior is to return the result of the comparison function call if non-zero, or continue evaluating the chain of comparison functions. This simplifies the code in that using a temporary `r` variable to store the function results is no longer necessary and the checks for non-zero to return are no longer needed either, resulting in a typical three-fold reduction to the number of lines in the code. Introduce a new memcmp_nn() to compare two memory buffers in lexicographic order, taking length in consideration. Tested: $ ninja -C build/ test All test cases pass. In particular, test_multiple_neighbors_sorted() in test-lldp would catch regressions introduced by this commit. --- src/basic/util.h | 6 ++++++ src/libsystemd-network/lldp-neighbor.c | 18 +++--------------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/basic/util.h b/src/basic/util.h index 85a7a15843..f009d37d4c 100644 --- a/src/basic/util.h +++ b/src/basic/util.h @@ -147,6 +147,12 @@ static inline int memcmp_safe(const void *s1, const void *s2, size_t n) { return memcmp(s1, s2, n); } +/* Compare s1 (length n1) with s2 (length n2) in lexicographic order. */ +static inline int memcmp_nn(const void *s1, size_t n1, const void *s2, size_t n2) { + return memcmp_safe(s1, s2, MIN(n1, n2)) + ?: CMP(n1, n2); +} + int on_ac_power(void); #define memzero(x,l) \ diff --git a/src/libsystemd-network/lldp-neighbor.c b/src/libsystemd-network/lldp-neighbor.c index 952197e714..199d8aee0a 100644 --- a/src/libsystemd-network/lldp-neighbor.c +++ b/src/libsystemd-network/lldp-neighbor.c @@ -9,6 +9,7 @@ #include "lldp-neighbor.h" #include "missing.h" #include "unaligned.h" +#include "util.h" static void lldp_neighbor_id_hash_func(const LLDPNeighborID *id, struct siphash *state) { siphash24_compress(id->chassis_id, id->chassis_id_size, state); @@ -18,21 +19,8 @@ static void lldp_neighbor_id_hash_func(const LLDPNeighborID *id, struct siphash } int lldp_neighbor_id_compare_func(const LLDPNeighborID *x, const LLDPNeighborID *y) { - int r; - - r = memcmp(x->chassis_id, y->chassis_id, MIN(x->chassis_id_size, y->chassis_id_size)); - if (r != 0) - return r; - - r = CMP(x->chassis_id_size, y->chassis_id_size); - if (r != 0) - return r; - - r = memcmp(x->port_id, y->port_id, MIN(x->port_id_size, y->port_id_size)); - if (r != 0) - return r; - - return CMP(x->port_id_size, y->port_id_size); + return memcmp_nn(x->chassis_id, x->chassis_id_size, y->chassis_id, y->chassis_id_size) + ?: memcmp_nn(x->port_id, x->port_id_size, y->port_id, y->port_id_size); } DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR(lldp_neighbor_hash_ops, LLDPNeighborID, lldp_neighbor_id_hash_func, lldp_neighbor_id_compare_func,