UT coverage to 95%, code improve and minor fix (#34)

* global mock, ut coverage to 95%, code cleanup and refine

.azure-pipelines/build.yml

@@ -53,7 +53,13 @@ jobs:
     displayName: "Install dependencies"
   - checkout: self
     clean: true
-    submodules: true
+    submodules: recursive
+  - script: |
+      git submodule foreach --recursive 'git clean -xfdf || true'
+      git submodule foreach --recursive 'git reset --hard || true'
+      git submodule foreach --recursive 'git remote update || true'
+      git submodule update --init --recursive
+    displayName: 'Reset submodules'
   - task: DownloadPipelineArtifact@2
     inputs:
       source: specific

.gitmodules

@@ -0,0 +1,3 @@
+[submodule "gmock_global"]
+	path = gmock_global
+	url = https://github.com/apriorit/gmock-global.git

Makefile

@@ -1,3 +1,6 @@
+.ONESHELL:
+SHELL = /bin/bash
+
 RM := rm -rf
 BUILD_DIR := build
 BUILD_TEST_DIR := build-test
@@ -12,7 +15,7 @@ override LDLIBS += -levent -lhiredis -lswsscommon -pthread -lboost_thread -lboos
 override CPPFLAGS += -Wall -std=c++17 -fPIE -I/usr/include/swss
 override CPPFLAGS += -MMD -MP -MF"$(@:%.o=%.d)" -MT"$(@)"
 CPPFLAGS_TEST := --coverage -fprofile-arcs -ftest-coverage -fprofile-generate -fsanitize=address
-LDLIBS_TEST := --coverage -lgtest -pthread -lstdc++fs -fsanitize=address
+LDLIBS_TEST := --coverage -lgtest -lgmock -pthread -lstdc++fs -fsanitize=address
 PWD := $(shell pwd)
 
 all: $(DHCP6RELAY_TARGET) $(DHCP6RELAY_TEST_TARGET)

azurepipelines-coverage.yml

@@ -2,4 +2,4 @@ coverage:
   status:           # Code coverage status will be posted to pull requests based on targets defined below.
     comments: on    # Off by default. When on, details about coverage for each file changed will be posted as a pull request comment. 
     diff:           # Diff coverage is code coverage only for the lines changed in a pull request.
-      target: 60%   # Set this to a desired percentage. Default is 70 percent
+      target: 90%   # Set this to a desired percentage. Default is 70 percent

src/configInterface.cpp → src/config_interface.cpp

@@ -1,12 +1,11 @@
 #include <sstream>
 #include <syslog.h>
 #include <algorithm>
-#include "configInterface.h"
+#include "config_interface.h"
 
 constexpr auto DEFAULT_TIMEOUT_MSEC = 1000;
 
 bool pollSwssNotifcation = true;
-std::shared_ptr<boost::thread> mSwssThreadPtr;
 swss::Select swssSelect;
 
 /**
@@ -29,6 +28,17 @@ void initialize_swss(std::unordered_map<std::string, relay_config> &vlans)
     }
 }
 
+/**
+*@code      stopSwssNotificationPoll
+*
+*@brief     stop SWSS listening thread
+*
+*@return    none
+*/
+static void stopSwssNotificationPoll() {
+    pollSwssNotifcation = false;
+};
+
 /**
  * @code                void deinitialize_swss()
  * 
@@ -39,12 +49,8 @@ void initialize_swss(std::unordered_map<std::string, relay_config> &vlans)
 void deinitialize_swss()
 {
     stopSwssNotificationPoll();
-    if (mSwssThreadPtr != nullptr) {
-        mSwssThreadPtr->interrupt();
-    }
 }
 
-
 /**
 
  * @code                void get_dhcp(std::unordered_map<std::string, relay_config> &vlans, swss::SubscriberStateTable *ipHelpersTable, bool dynamic)
@@ -58,6 +64,7 @@ void get_dhcp(std::unordered_map<std::string, relay_config> &vlans, swss::Subscr
     int ret = swssSelect.select(&selectable, DEFAULT_TIMEOUT_MSEC);
     if (ret == swss::Select::ERROR) {
         syslog(LOG_WARNING, "Select: returned ERROR");
+        return;
     } else if (ret == swss::Select::TIMEOUT) {
     } 
     if (selectable == static_cast<swss::Selectable *> (ipHelpersTable)) {
@@ -69,21 +76,6 @@ void get_dhcp(std::unordered_map<std::string, relay_config> &vlans, swss::Subscr
         }
     }
 }
-/**
- * @code                void handleSwssNotification(std::vector<relay_config> *vlans)
- * 
- * @brief               main thread for handling SWSS notification
- *
- * @param context       map of vlans/argument config that contains strings of server and option
- *
- * @return              none
- */
-void handleSwssNotification(swssNotification test)
-{
-    while (pollSwssNotifcation) {
-        get_dhcp(test.vlans, test.ipHelpersTable, true);
-    }
-}
 
 /**
  * @code                    void handleRelayNotification(swss::SubscriberStateTable &ipHelpersTable, std::unordered_map<std::string, relay_config> &vlans)
@@ -152,14 +144,3 @@ void processRelayNotification(std::deque<swss::KeyOpFieldsValuesTuple> &entries,
         vlans[vlan] = intf;
     }
 }
-
-/**
-*@code      stopSwssNotificationPoll
-*
-*@brief     stop SWSS listening thread
-*
-*@return    none
-*/
-void stopSwssNotificationPoll() {
-    pollSwssNotifcation = false;
-};

src/configInterface.h → src/config_interface.h

@@ -37,16 +37,6 @@ void deinitialize_swss();
  * @return              none
  */
 void get_dhcp(std::unordered_map<std::string, relay_config> &vlans, swss::SubscriberStateTable *ipHelpersTable, bool dynamic);
-/**
- * @code                void swssNotification test
- * 
- * @brief               main thread for handling SWSS notification
- *
- * @param test          swssNotification that includes list of vlans/argument config that contains strings of server and option
- *
- * @return              none
- */
-void handleSwssNotification(swssNotification test);
 
 /**
  * @code                    void handleRelayNotification(swss::SubscriberStateTable &ipHelpersTable, std::unordered_map<std::string, relay_config> &vlans)
@@ -71,12 +61,3 @@ void handleRelayNotification(swss::SubscriberStateTable &ipHelpersTable, std::un
  * @return                  none
  */
 void processRelayNotification(std::deque<swss::KeyOpFieldsValuesTuple> &entries, std::unordered_map<std::string, relay_config> &vlans);
-
-/**
-*@code      stopSwssNotificationPoll
-*
-*@brief     stop SWSS listening thread
-*
-*@return    none
-*/
-void stopSwssNotificationPoll();

src/main.cpp

@@ -1,7 +1,7 @@
 #include <stdlib.h>
 #include <syslog.h>
 #include <unordered_map>
-#include "configInterface.h"
+#include "config_interface.h"
 
 bool dual_tor_sock = false;
 

src/relay.cpp

@@ -9,7 +9,7 @@
 #include "configdb.h"
 #include "sonicv2connector.h"
 #include "dbconnector.h" 
-#include "configInterface.h"
+#include "config_interface.h"
 
 struct event *listen_event;
 struct event *server_listen_event;
@@ -163,6 +163,12 @@ uint8_t *RelayMsg::MarshalBinary(uint16_t &len) {
 
     auto opt = m_option_list.MarshalBinary();
     if (opt && !opt->empty()) {
+        if (opt->size() + sizeof(dhcpv6_relay_msg) > BUFFER_SIZE) {
+            syslog(LOG_WARNING, "Failed to marshal relay msg, packet size %lu over limit\n",
+                   opt->size() + sizeof(dhcpv6_relay_msg));
+            len = 0;
+            return nullptr;
+        }
         std::memcpy(ptr + sizeof(dhcpv6_relay_msg), opt->data(), opt->size());
         len += opt->size();
     }
@@ -210,6 +216,12 @@ uint8_t *DHCPv6Msg::MarshalBinary(uint16_t &len) {
 
     auto opt = m_option_list.MarshalBinary();
     if (opt && !opt->empty()) {
+        if (opt->size() + sizeof(dhcpv6_msg) > BUFFER_SIZE) {
+            syslog(LOG_WARNING, "Failed to marshal dhcpv6 msg, packet size %lu over limit\n",
+                   opt->size() + sizeof(dhcpv6_msg));
+            len = 0;
+            return nullptr;
+        }
         std::memcpy(ptr + sizeof(dhcpv6_msg), opt->data(), opt->size());
         len += opt->size();
     }
@@ -637,7 +649,7 @@ void relay_client(int sock, const uint8_t *msg, uint16_t len, const ip6_hdr *ip_
 
     uint16_t relay_pkt_len = 0;
     auto relay_pkt = relay.MarshalBinary(relay_pkt_len);
-    if (!relay_pkt_len) {
+    if (!relay_pkt_len || !relay_pkt) {
         char addr_str[INET6_ADDRSTRLEN];
         inet_ntop(AF_INET6, &ip_hdr->ip6_src, addr_str, INET6_ADDRSTRLEN);
         syslog(LOG_ERR, "Relay-forward marshal error, client dhcpv6 from %s", addr_str);
@@ -683,10 +695,10 @@ void relay_relay_forw(int sock, const uint8_t *msg, int32_t len, const ip6_hdr *
 
     /* add relay-msg option */
     relay.m_option_list.Add(OPTION_RELAY_MSG, msg, len);
-    
+
     uint16_t send_buffer_len = 0;
     auto send_buffer = relay.MarshalBinary(send_buffer_len);
-    if (!send_buffer_len) {
+    if (!send_buffer_len || !send_buffer) {
         char addr_str[INET6_ADDRSTRLEN];
         inet_ntop(AF_INET6, &ip_hdr->ip6_src, addr_str, INET6_ADDRSTRLEN);
         syslog(LOG_ERR, "Marshal relay-forward message from %s error", addr_str);
@@ -929,7 +941,7 @@ void server_callback(evutil_socket_t fd, short event, void *arg) {
         }
 
         if (buffer_sz < (int32_t)sizeof(struct dhcpv6_msg)) {
-            syslog(LOG_WARNING, "Invalid DHCPv6 packet length %ld, no space for dhcpv6 msg header\n", buffer_sz);
+            syslog(LOG_WARNING, "Invalid DHCPv6 packet length %zd, no space for dhcpv6 msg header\n", buffer_sz);
             continue;
         }
 
@@ -955,7 +967,7 @@ void server_callback(evutil_socket_t fd, short event, void *arg) {
  */
 int signal_init() {
     int rv = -1;
-     do {
+    do {
         ev_sigint = evsignal_new(base, SIGINT, signal_callback, base);
         if (ev_sigint == NULL) {
             syslog(LOG_ERR, "Could not create SIGINT libevent signal\n");
@@ -1043,13 +1055,14 @@ void loop_relay(std::unordered_map<std::string, relay_config> &vlans) {
     base = event_base_new();
     if(base == NULL) {
         syslog(LOG_ERR, "libevent: Failed to create base\n");
+        exit(EXIT_FAILURE);
     }
 
     std::shared_ptr<swss::DBConnector> state_db = std::make_shared<swss::DBConnector> ("STATE_DB", 0);
     std::shared_ptr<swss::DBConnector> config_db = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
     std::shared_ptr<swss::Table> mStateDbMuxTablePtr = std::make_shared<swss::Table> (
-            state_db.get(), "HW_MUX_CABLE_TABLE"
-        );
+        state_db.get(), "HW_MUX_CABLE_TABLE"
+    );
 
     auto filter = sock_open(&ether_relay_fprog);
     if (filter != -1) {
@@ -1102,23 +1115,23 @@ void loop_relay(std::unordered_map<std::string, relay_config> &vlans) {
         syslog(LOG_INFO, "libevent: Add server listen socket for %s\n", vlan.first.c_str());
     }
 
-    if((signal_init() == 0) && signal_start() == 0) {
-        shutdown();
-        for(std::size_t i = 0; i<sockets.size(); i++) {
+    if(signal_init() == 0 && signal_start() == 0) {
+        shutdown_relay();
+        for(std::size_t i = 0; i < sockets.size(); i++) {
             close(sockets.at(i));
         }
     }
 }
 
 /**
- * @code shutdown();
+ * @code shutdown_relay();
  *
  * @brief free signals and terminate threads
  */
-void shutdown() {
+void shutdown_relay() {
     event_del(ev_sigint);
     event_del(ev_sigterm);
-    event_free(ev_sigint); 
+    event_free(ev_sigint);
     event_free(ev_sigterm);
     event_base_free(base);
     deinitialize_swss();

src/relay.h

@@ -285,7 +285,7 @@ void signal_callback(evutil_socket_t fd, short event, void *arg);
  *
  * @brief free signals and terminate threads
  */
-void shutdown();
+void shutdown_relay();
 
 /**
  * @code                void initialize_counter(std::shared_ptr<swss::Table> state_db, std::string counterVlan);

src/subdir.mk

@@ -1,5 +1,5 @@
 SRCS += \
 src/sender.cpp \
 src/relay.cpp \
-src/configInterface.cpp \
+src/config_interface.cpp \
 src/main.cpp

test/mock_config_interface.cpp

@@ -0,0 +1,67 @@
+#include "mock_config_interface.h"
+
+using namespace ::testing;
+
+TEST(configInterface, initialize_swss) {
+  std::shared_ptr<swss::DBConnector> config_db = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_servers@", "fc02:2000::1,fc02:2000::2,fc02:2000::3,fc02:2000::4");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|rfc6939_support", "false");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|interface_id", "true");
+  std::unordered_map<std::string, relay_config> vlans;
+  ASSERT_NO_THROW(initialize_swss(vlans));
+  EXPECT_EQ(vlans.size(), 1);
+}
+
+TEST(configInterface, deinitialize_swss) {
+  ASSERT_NO_THROW(deinitialize_swss());
+}
+
+TEST(configInterface, get_dhcp) {
+  std::shared_ptr<swss::DBConnector> config_db = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_servers@", "fc02:2000::1,fc02:2000::2,fc02:2000::3,fc02:2000::4");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|rfc6939_support", "false");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|interface_id", "true");
+  swss::SubscriberStateTable ipHelpersTable(config_db.get(), "DHCP_RELAY");
+  std::unordered_map<std::string, relay_config> vlans;
+
+  ASSERT_NO_THROW(get_dhcp(vlans, &ipHelpersTable, false));
+  EXPECT_EQ(vlans.size(), 0);
+
+  swssSelect.addSelectable(&ipHelpersTable);
+
+  ASSERT_NO_THROW(get_dhcp(vlans, &ipHelpersTable, false));
+  EXPECT_EQ(vlans.size(), 1);
+}
+
+TEST(configInterface, handleRelayNotification) {
+  std::shared_ptr<swss::DBConnector> cfg_db = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
+  swss::SubscriberStateTable ipHelpersTable(cfg_db.get(), "DHCP_RELAY");
+  std::unordered_map<std::string, relay_config> vlans;
+  handleRelayNotification(ipHelpersTable, vlans);
+}
+
+TEST(configInterface, processRelayNotification) {  
+  std::shared_ptr<swss::DBConnector> config_db = std::make_shared<swss::DBConnector> ("CONFIG_DB", 0);
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_servers@", "fc02:2000::1,fc02:2000::2,fc02:2000::3,fc02:2000::4");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|rfc6939_support", "false");
+  config_db->hset("DHCP_RELAY|Vlan1000", "dhcpv6_option|interface_id", "true");
+  swss::SubscriberStateTable ipHelpersTable(config_db.get(), "DHCP_RELAY");
+  swssSelect.addSelectable(&ipHelpersTable);
+  std::deque<swss::KeyOpFieldsValuesTuple> entries;
+  ipHelpersTable.pops(entries);
+  std::unordered_map<std::string, relay_config> vlans;
+
+  processRelayNotification(entries, vlans);
+
+  EXPECT_EQ(vlans.size(), 1);
+  EXPECT_FALSE(vlans["Vlan1000"].is_option_79);
+  EXPECT_TRUE(vlans["Vlan1000"].is_interface_id);
+  EXPECT_FALSE(vlans["Vlan1000"].state_db);
+}
+
+MOCK_GLOBAL_FUNC0(stopSwssNotificationPoll, void(void));
+
+TEST(configInterface, stopSwssNotificationPoll) {
+  EXPECT_GLOBAL_CALL(stopSwssNotificationPoll, stopSwssNotificationPoll()).Times(1);
+  ASSERT_NO_THROW(stopSwssNotificationPoll());
+}

test/mock_config_interface.h

@@ -0,0 +1,12 @@
+#pragma once
+
+#include "../src/config_interface.h"
+#include "mock_send.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include "../gmock_global/include/gmock-global/gmock-global.h"
+#include <new>
+#include <future>
+
+extern bool pollSwssNotifcation;
+extern swss::Select swssSelect;