From 0d296cea583c8714cb9434ab005ef822cd046a90 Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 7 Feb 2023 15:29:36 +0000 Subject: [PATCH 1/5] Do not set port to be a bridge port while it is still a router port --- orchagent/portsorch.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 8c9e14608c..a2f8d4f09b 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4879,6 +4879,12 @@ bool PortsOrch::addBridgePort(Port &port) return true; } + if (port.m_rif_id != 0) + { + SWSS_LOG_NOTICE("Interface %s is still a router port", port.m_alias.c_str()); + return false; + } + sai_attribute_t attr; vector attrs; From 0f5e17dcc20e4b4987baa5e168cd0ff089742f53 Mon Sep 17 00:00:00 2001 From: liora Date: Wed, 8 Feb 2023 17:56:26 +0000 Subject: [PATCH 2/5] Add unitest for bridge port fix --- tests/mock_tests/mock_sai_bridge.h | 34 +++++++++++++++++ tests/mock_tests/portsorch_ut.cpp | 60 ++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 tests/mock_tests/mock_sai_bridge.h diff --git a/tests/mock_tests/mock_sai_bridge.h b/tests/mock_tests/mock_sai_bridge.h new file mode 100644 index 0000000000..f85ffa78d4 --- /dev/null +++ b/tests/mock_tests/mock_sai_bridge.h @@ -0,0 +1,34 @@ +// Define classes and functions to mock SAI bridge functions. +#pragma once + +#include + +extern "C" +{ +#include "sai.h" +} + +// Mock class including mock functions mapping to SAI bridge functions. +class MockSaiBridge +{ + public: + MOCK_METHOD4(create_bridge_port, sai_status_t(_Out_ sai_object_id_t *bridge_port_id, + _In_ sai_object_id_t switch_id, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list)); +}; + +// Note that before mock functions below are used, mock_sai_bridge must be +// initialized to point to an instance of MockSaiBridge. +MockSaiBridge *mock_sai_bridge; + +sai_status_t mock_create_bridge_port(_Out_ sai_object_id_t *bridge_port_id, + _In_ sai_object_id_t switch_id, + _In_ uint32_t attr_count, + _In_ const sai_attribute_t *attr_list) +{ + return mock_sai_bridge->create_bridge_port(bridge_port_id, switch_id, attr_count, attr_list); +} + + + diff --git a/tests/mock_tests/portsorch_ut.cpp b/tests/mock_tests/portsorch_ut.cpp index 836e862d55..23dd50ee68 100644 --- a/tests/mock_tests/portsorch_ut.cpp +++ b/tests/mock_tests/portsorch_ut.cpp @@ -7,6 +7,7 @@ #include "mock_orchagent_main.h" #include "mock_table.h" #include "notifier.h" +#include "mock_sai_bridge.h" #define private public #include "pfcactionhandler.h" #undef private @@ -14,6 +15,8 @@ #include extern redisReply *mockReply; +using ::testing::_; +using ::testing::StrictMock; namespace portsorch_test { @@ -116,6 +119,21 @@ namespace portsorch_test sai_queue_api = pold_sai_queue_api; } + sai_bridge_api_t ut_sai_bridge_api; + sai_bridge_api_t *org_sai_bridge_api; + + void _hook_sai_bridge_api() + { + ut_sai_bridge_api = *sai_bridge_api; + org_sai_bridge_api = sai_bridge_api; + sai_bridge_api = &ut_sai_bridge_api; + } + + void _unhook_sai_bridge_api() + { + sai_bridge_api = org_sai_bridge_api; + } + struct PortsOrchTest : public ::testing::Test { shared_ptr m_app_db; @@ -310,6 +328,48 @@ namespace portsorch_test ASSERT_FALSE(gPortsOrch->getPort(port.m_port_id, port)); } + /** + * Test case: PortsOrch::addBridgePort() does not add router port to .1Q bridge + */ + TEST_F(PortsOrchTest, addBridgePortOnRouterPort) + { + _hook_sai_bridge_api(); + + StrictMock mock_sai_bridge_; + mock_sai_bridge = &mock_sai_bridge_; + sai_bridge_api->create_bridge_port = mock_create_bridge_port; + + Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME); + + // Get SAI default ports to populate DB + auto ports = ut_helper::getInitialSaiPorts(); + + // Populate port table with SAI ports + for (const auto &it : ports) + { + portTable.set(it.first, it.second); + } + + // Set PortConfigDone, PortInitDone + portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } }); + portTable.set("PortInitDone", { { "lanes", "0" } }); + + // refill consumer + gPortsOrch->addExistingData(&portTable); + // Apply configuration : create ports + static_cast(gPortsOrch)->doTask(); + + // Get first port and set its rif id to simulate it is router port + Port port; + gPortsOrch->getPort("Ethernet0", port); + port.m_rif_id = 1; + + ASSERT_FALSE(gPortsOrch->addBridgePort(port)); + EXPECT_CALL(mock_sai_bridge_, create_bridge_port(_, _, _, _)).Times(0); + + _unhook_sai_bridge_api(); + } + TEST_F(PortsOrchTest, PortSupportedFecModes) { _hook_sai_port_api(); From c9ee57bd30c487f787c1a8998be2e3f0c7173b3a Mon Sep 17 00:00:00 2001 From: liora Date: Sun, 12 Feb 2023 08:58:11 +0000 Subject: [PATCH 3/5] Remove SAL annotation --- tests/mock_tests/mock_sai_bridge.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/mock_tests/mock_sai_bridge.h b/tests/mock_tests/mock_sai_bridge.h index f85ffa78d4..8141ca66bb 100644 --- a/tests/mock_tests/mock_sai_bridge.h +++ b/tests/mock_tests/mock_sai_bridge.h @@ -12,20 +12,20 @@ extern "C" class MockSaiBridge { public: - MOCK_METHOD4(create_bridge_port, sai_status_t(_Out_ sai_object_id_t *bridge_port_id, - _In_ sai_object_id_t switch_id, - _In_ uint32_t attr_count, - _In_ const sai_attribute_t *attr_list)); + MOCK_METHOD4(create_bridge_port, sai_status_t(sai_object_id_t *bridge_port_id, + sai_object_id_t switch_id, + uint32_t attr_count, + const sai_attribute_t *attr_list)); }; // Note that before mock functions below are used, mock_sai_bridge must be // initialized to point to an instance of MockSaiBridge. MockSaiBridge *mock_sai_bridge; -sai_status_t mock_create_bridge_port(_Out_ sai_object_id_t *bridge_port_id, - _In_ sai_object_id_t switch_id, - _In_ uint32_t attr_count, - _In_ const sai_attribute_t *attr_list) +sai_status_t mock_create_bridge_port(sai_object_id_t *bridge_port_id, + sai_object_id_t switch_id, + uint32_t attr_count, + const sai_attribute_t *attr_list) { return mock_sai_bridge->create_bridge_port(bridge_port_id, switch_id, attr_count, attr_list); } From 7617c6d73fb940ce7147443c68005d99d8854cbc Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 14 Feb 2023 07:03:59 +0000 Subject: [PATCH 4/5] Fix review comment --- orchagent/portsorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index a2f8d4f09b..e6575e1b9f 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4881,7 +4881,7 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_rif_id != 0) { - SWSS_LOG_NOTICE("Interface %s is still a router port", port.m_alias.c_str()); + SWSS_LOG_NOTICE("Interface %s is a router port", port.m_alias.c_str()); return false; } From 288c44919299473b87058b6396775952220aed1a Mon Sep 17 00:00:00 2001 From: liora Date: Sun, 19 Feb 2023 07:52:32 +0000 Subject: [PATCH 5/5] Fix more review comment --- orchagent/portsorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index e6575e1b9f..4e81dae598 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4881,7 +4881,7 @@ bool PortsOrch::addBridgePort(Port &port) if (port.m_rif_id != 0) { - SWSS_LOG_NOTICE("Interface %s is a router port", port.m_alias.c_str()); + SWSS_LOG_NOTICE("Cannot create bridge port, interface %s is a router port", port.m_alias.c_str()); return false; }