for injection, add handler: add unit-tests
Add a handler to enable dependency injection for testing. This is a
required step to use mocks.
Add the unit-tests.
Tested: Verified the new unit-tests ran.
Change-Id: I991849e6df2c3e74f59145a966048fc6825560db
Signed-off-by: Patrick Venture <venture@google.com>
diff --git a/Makefile.am b/Makefile.am
index c0e1f35..2f78a5f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2,7 +2,7 @@
libethstatscmddir = ${libdir}/ipmid-providers
libethstatscmd_LTLIBRARIES = libethstatscmd.la
-libethstatscmd_la_SOURCES = main.cpp ethstats.cpp
+libethstatscmd_la_SOURCES = main.cpp ethstats.cpp handler.cpp
libethstatscmd_la_LDFLAGS = \
$(LIBIPMID_LIBS) \
diff --git a/ethstats.cpp b/ethstats.cpp
index b135331..cc8599d 100644
--- a/ethstats.cpp
+++ b/ethstats.cpp
@@ -16,21 +16,19 @@
#include "ethstats.hpp"
+#include "handler.hpp"
+
#include <ipmid/api.h>
#include <cstdint>
#include <cstdio>
#include <cstring>
-#include <filesystem>
-#include <fstream>
#include <map>
#include <sstream>
#include <string>
namespace ethstats
{
-namespace fs = std::filesystem;
-
// If this changes in the future, there should be some alternative
// source for the information if possible to provide continuined functionality.
static const std::map<std::uint8_t, std::string> statLookup = {
@@ -58,9 +56,17 @@
{TX_WINDOW_ERRORS, "tx_window_errors"},
};
-ipmi_ret_t handleEthStatCommand(ipmi_cmd_t cmd __attribute__((unused)),
- const std::uint8_t* reqBuf,
- std::uint8_t* replyCmdBuf, size_t* dataLen)
+std::string buildPath(const std::string& ifName, const std::string& field)
+{
+ std::ostringstream opath;
+ opath << "/sys/class/net/" << ifName << "/statistics/" << field;
+
+ return opath.str();
+}
+
+ipmi_ret_t handleEthStatCommand(const std::uint8_t* reqBuf,
+ std::uint8_t* replyCmdBuf, size_t* dataLen,
+ const EthStatsInterface* handler)
{
auto reqLength = (*dataLen);
@@ -119,39 +125,16 @@
return IPMI_CC_INVALID_FIELD_REQUEST;
}
- // TODO: Transition to using the netlink api.
- std::ostringstream opath;
- opath << "/sys/class/net/" << name << "/statistics/" << stat->second;
- std::string path = opath.str();
+ std::string fullPath = buildPath(name, stat->second);
- std::error_code ec;
- if (!fs::exists(path, ec))
+ if (!handler->validIfNameAndField(fullPath))
{
- std::fprintf(stderr, "Path: '%s' doesn't exist.\n", path.c_str());
return IPMI_CC_INVALID_FIELD_REQUEST;
}
- // We're uninterested in the state of ec.
-
- // Read the file and check the result.
- // We read the number as int64, then check to make sure it's positive
- // before casting to uint64.
- std::uint64_t value = 0;
- std::ifstream ifs;
- ifs.exceptions(std::ifstream::failbit);
-
- try
- {
- ifs.open(path);
- ifs >> value;
- }
- catch (std::ios_base::failure& fail)
- {
- return IPMI_CC_UNSPECIFIED_ERROR;
- }
struct EthStatReply reply;
reply.statId = request.statId;
- reply.value = value;
+ reply.value = handler->readStatistic(fullPath);
// Store the result.
std::memcpy(&replyCmdBuf[0], &reply, sizeof(reply));
diff --git a/ethstats.hpp b/ethstats.hpp
index bf6b707..984d46d 100644
--- a/ethstats.hpp
+++ b/ethstats.hpp
@@ -1,8 +1,11 @@
#pragma once
+#include "handler.hpp"
+
#include <ipmid/api.h>
#include <cstdint>
+#include <string>
namespace ethstats
{
@@ -54,14 +57,24 @@
/**
* Handle the OEM IPMI EthStat Command.
*
- * @param[in] cmd - the OEM command.
* @param[in] reqBuf - the IPMI request buffer.
* @param[in,out] replyCmdBuf - the IPMI reply buffer.
* @param[in,out] dataLen - the length of the request and reply.
+ * @param[in] handler - pointer to ethstats implementation.
* @return the IPMI result code.
*/
-ipmi_ret_t handleEthStatCommand(ipmi_cmd_t cmd __attribute__((unused)),
- const std::uint8_t* reqBuf,
- std::uint8_t* replyCmdBuf, size_t* dataLen);
+ipmi_ret_t handleEthStatCommand(const std::uint8_t* reqBuf,
+ std::uint8_t* replyCmdBuf, size_t* dataLen,
+ const EthStatsInterface* handler = &handler);
+
+/**
+ * Given an ethernet if_name and a field, build the full path.
+ *
+ * @param[in] ifName - the ethernet interface's name.
+ * @param[in] field - the name of the statistic
+ * @return the full path of the file to read for the statistic for that
+ * interface name.
+ */
+std::string buildPath(const std::string& ifName, const std::string& field);
} // namespace ethstats
diff --git a/handler.cpp b/handler.cpp
new file mode 100644
index 0000000..d34b7fc
--- /dev/null
+++ b/handler.cpp
@@ -0,0 +1,58 @@
+/*
+ * Copyright 2019 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "handler.hpp"
+
+#include <cstdint>
+#include <cstdio>
+#include <filesystem>
+#include <fstream>
+#include <string>
+
+namespace ethstats
+{
+
+bool EthStats::validIfNameAndField(const std::string& path) const
+{
+ namespace fs = std::filesystem;
+
+ // TODO: Transition to using the netlink api.
+ std::error_code ec;
+ if (!fs::exists(path, ec))
+ {
+ std::fprintf(stderr, "Path: '%s' doesn't exist. ec(%d, %s)\n",
+ path.c_str(), ec.value(), ec.message().c_str());
+ return false;
+ }
+
+ return true;
+}
+
+std::uint64_t EthStats::readStatistic(const std::string& path) const
+{
+ // We know the file exists, so just read it.
+ std::uint64_t value = 0;
+ std::ifstream ifs;
+
+ ifs.open(path);
+ ifs >> value;
+
+ return value;
+}
+
+EthStats handler;
+
+} // namespace ethstats
diff --git a/handler.hpp b/handler.hpp
new file mode 100644
index 0000000..77ad55c
--- /dev/null
+++ b/handler.hpp
@@ -0,0 +1,41 @@
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+namespace ethstats
+{
+
+class EthStatsInterface
+{
+ public:
+ virtual ~EthStatsInterface() = default;
+
+ /** Given an ifname and a statistic, validate both.
+ *
+ * @param[in] path - the interface name and statistics field
+ * @return true if both valid, false otherwise.
+ */
+ virtual bool validIfNameAndField(const std::string& path) const = 0;
+
+ /** Given an ifname and a statistic, return the value.
+ *
+ * @param[in] path - the interface name and statistics field
+ * @return the value of that statistic for that interface.
+ */
+ virtual std::uint64_t readStatistic(const std::string& path) const = 0;
+};
+
+class EthStats : public EthStatsInterface
+{
+ public:
+ EthStats() = default;
+ ~EthStats() = default;
+
+ bool validIfNameAndField(const std::string& path) const override;
+ std::uint64_t readStatistic(const std::string& path) const override;
+};
+
+extern EthStats handler;
+
+} // namespace ethstats
diff --git a/main.cpp b/main.cpp
index 59fd9f9..9020809 100644
--- a/main.cpp
+++ b/main.cpp
@@ -23,6 +23,18 @@
#include <ipmid/oemopenbmc.hpp>
#include <ipmid/oemrouter.hpp>
+namespace ethstats
+{
+
+static ipmi_ret_t ethStatCommand(ipmi_cmd_t cmd __attribute__((unused)),
+ const uint8_t* reqBuf, uint8_t* replyCmdBuf,
+ size_t* dataLen)
+{
+ return handleEthStatCommand(reqBuf, replyCmdBuf, dataLen);
+}
+
+} // namespace ethstats
+
void setupGlobalOemEthStats() __attribute__((constructor));
void setupGlobalOemEthStats()
@@ -36,7 +48,7 @@
oem::googOemNumber, oem::Cmd::ethStatsCmd);
oemRouter->registerHandler(oem::googOemNumber, oem::Cmd::ethStatsCmd,
- ethstats::handleEthStatCommand);
+ ethstats::ethStatCommand);
#endif
std::fprintf(stderr,
@@ -44,5 +56,5 @@
oem::obmcOemNumber, oem::Cmd::ethStatsCmd);
oemRouter->registerHandler(oem::obmcOemNumber, oem::Cmd::ethStatsCmd,
- ethstats::handleEthStatCommand);
+ ethstats::ethStatCommand);
}
diff --git a/test/Makefile.am b/test/Makefile.am
index 194e582..e754936 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -10,3 +10,8 @@
check_PROGRAMS =
TESTS = $(check_PROGRAMS)
+
+check_PROGRAMS += ethstats_unittest
+ethstats_unittest_SOURCES = ethstats_unittest.cpp
+ethstats_unittest_LDADD = $(top_builddir)/ethstats.o
+
diff --git a/test/ethstats_unittest.cpp b/test/ethstats_unittest.cpp
new file mode 100644
index 0000000..41a2c62
--- /dev/null
+++ b/test/ethstats_unittest.cpp
@@ -0,0 +1,177 @@
+#include "ethstats.hpp"
+#include "handler_mock.hpp"
+
+#include <cstdint>
+#include <cstring>
+#include <string>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#define MAX_IPMI_BUFFER 64
+
+using ::testing::Return;
+using ::testing::StrEq;
+using ::testing::StrictMock;
+
+namespace ethstats
+{
+
+TEST(EthStatsTest, InvalidStatReturnsFailure)
+{
+ // Verify that the enum of valid statistic IDs is checked.
+
+ std::string ifName = "eth0";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::TX_WINDOW_ERRORS + 1;
+ requestStruct.if_name_len = ifName.length();
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ // Using StrictMock to ensure it isn't called.
+ StrictMock<HandlerMock> hMock;
+
+ EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(EthStatsTest, InvalidIpmiPacketSize)
+{
+ // An IPMI packet for this command has a minimum length.
+
+ std::string ifName = "e";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::RX_BYTES;
+ requestStruct.if_name_len = ifName.length();
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ // The minimum length is a 1-byte ifname - this gives one, but dataLen is
+ // set to smaller.
+ size_t dataLen = request.size() - 1;
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ // Using StrictMock to ensure it isn't called.
+ StrictMock<HandlerMock> hMock;
+
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(EthStatsTest, InvalidIpmiPacketContents)
+{
+ // The packet has a name length and name contents, if the name length is
+ // longer than the packet size, it should fail.
+
+ std::string ifName = "eth0";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::RX_BYTES;
+ requestStruct.if_name_len = ifName.length() + 1;
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ // Using StrictMock to ensure it isn't called.
+ StrictMock<HandlerMock> hMock;
+
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(EthStatsTest, NameHasIllegalCharacters)
+{
+ // The interface name cannot have slashes.
+ std::string ifName = "et/h0";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::RX_BYTES;
+ requestStruct.if_name_len = ifName.length();
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ // Using StrictMock to ensure it isn't called.
+ StrictMock<HandlerMock> hMock;
+
+ EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(EthStatsTest, InvalidNameOrField)
+{
+ // The handler returns failure on the input validity check.
+ std::string ifName = "eth0";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::RX_BYTES;
+ requestStruct.if_name_len = ifName.length();
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ std::string expectedPath = buildPath(ifName, "rx_bytes");
+
+ HandlerMock hMock;
+ EXPECT_CALL(hMock, validIfNameAndField(StrEq(expectedPath)))
+ .WillOnce(Return(false));
+
+ EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(EthStatsTest, EverythingHappy)
+{
+ std::string ifName = "eth0";
+ struct EthStatRequest requestStruct;
+ requestStruct.statId = EthernetStatisticsIds::RX_BYTES;
+ requestStruct.if_name_len = ifName.length();
+
+ std::vector<std::uint8_t> request(sizeof(requestStruct) + ifName.length());
+ std::memcpy(request.data(), &requestStruct, sizeof(requestStruct));
+ std::memcpy(&request[sizeof(requestStruct)], ifName.c_str(),
+ ifName.length());
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ std::string expectedPath = buildPath(ifName, "rx_bytes");
+
+ HandlerMock hMock;
+ EXPECT_CALL(hMock, validIfNameAndField(StrEq(expectedPath)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(hMock, readStatistic(StrEq(expectedPath))).WillOnce(Return(1));
+
+ EXPECT_EQ(IPMI_CC_OK,
+ handleEthStatCommand(request.data(), reply, &dataLen, &hMock));
+
+ struct EthStatReply expectedReply, realReply;
+ expectedReply.statId = EthernetStatisticsIds::RX_BYTES;
+ expectedReply.value = 1;
+
+ std::memcpy(&realReply, reply, sizeof(realReply));
+ EXPECT_EQ(0, std::memcmp(&expectedReply, &realReply, sizeof(realReply)));
+}
+
+} // namespace ethstats
diff --git a/test/handler_mock.hpp b/test/handler_mock.hpp
new file mode 100644
index 0000000..1ef94a4
--- /dev/null
+++ b/test/handler_mock.hpp
@@ -0,0 +1,22 @@
+#pragma once
+
+#include "handler.hpp"
+
+#include <cstdint>
+#include <string>
+
+#include <gmock/gmock.h>
+
+namespace ethstats
+{
+
+class HandlerMock : public EthStatsInterface
+{
+ public:
+ ~HandlerMock() = default;
+
+ MOCK_CONST_METHOD1(validIfNameAndField, bool(const std::string&));
+ MOCK_CONST_METHOD1(readStatistic, std::uint64_t(const std::string&));
+};
+
+} // namespace ethstats