add handler logic to handle SysCableCheck
Add handler logic to handler for SysCableCheck such that it splits the
true IPMI processing from the business logic.
Tested: Only ran unit-tests (added new ones).
Change-Id: Ieec35cc8839dcd3cfb864b68ffbd1a45d1326fee
Signed-off-by: Patrick Venture <venture@google.com>
diff --git a/cable.cpp b/cable.cpp
index b00b668..dd499f2 100644
--- a/cable.cpp
+++ b/cable.cpp
@@ -16,21 +16,18 @@
#include "cable.hpp"
+#include "errors.hpp"
+#include "handler.hpp"
#include "main.hpp"
#include <cstdint>
#include <cstring>
-#include <filesystem>
-#include <fstream>
-#include <sstream>
#include <string>
-#include <system_error>
namespace google
{
namespace ipmi
{
-namespace fs = std::filesystem;
struct CableRequest
{
@@ -39,13 +36,8 @@
uint8_t if_name[0];
} __attribute__((packed));
-struct CableReply
-{
- uint8_t subcommand;
- uint8_t value;
-} __attribute__((packed));
-
-ipmi_ret_t CableCheck(const uint8_t* reqBuf, uint8_t* replyBuf, size_t* dataLen)
+ipmi_ret_t CableCheck(const uint8_t* reqBuf, uint8_t* replyBuf, size_t* dataLen,
+ const HandlerInterface* handler)
{
// There is an IPMI LAN channel statistics command which could be used for
// this type of check, however, we're not able to wait for the OpenBMC
@@ -83,45 +75,18 @@
// Maximum length one can specify, plus null terminator.
char nameBuf[256] = {};
- std::ostringstream opath;
-
// Copy the string out of the request buffer.
std::memcpy(&nameBuf[0], request->if_name, request->if_name_len);
std::string name = nameBuf;
+ int64_t count;
- // Minor sanity & security check (of course, I'm less certain if unicode
- // comes into play here.
- //
- // Basically you can't easily inject ../ or /../ into the path below.
- if (name.find("/") != std::string::npos)
- {
- std::fprintf(stderr, "Invalid or illegal name: '%s'\n", nameBuf);
- return IPMI_CC_INVALID_FIELD_REQUEST;
- }
-
- opath << "/sys/class/net/" << name << "/statistics/rx_packets";
- std::string path = opath.str();
-
- std::error_code ec;
- if (!fs::exists(path, ec))
- {
- 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.
- int64_t count = 0;
- std::ifstream ifs;
- ifs.exceptions(std::ifstream::failbit);
try
{
- ifs.open(path);
- ifs >> count;
+ count = handler->getRxPackets(name);
}
- catch (std::ios_base::failure& fail)
+ catch (const IpmiException& e)
{
- return IPMI_CC_UNSPECIFIED_ERROR;
+ return e.getIpmiError();
}
struct CableReply reply;
diff --git a/cable.hpp b/cable.hpp
index ce20226..1075b41 100644
--- a/cable.hpp
+++ b/cable.hpp
@@ -1,5 +1,7 @@
#pragma once
+#include "handler.hpp"
+
#include <ipmid/api.h>
namespace google
@@ -7,11 +9,17 @@
namespace ipmi
{
+struct CableReply
+{
+ uint8_t subcommand;
+ uint8_t value;
+} __attribute__((packed));
+
//
// Handle the cablecheck. Sys must supply which ethernet device they're
// interested in.
-ipmi_ret_t CableCheck(const uint8_t* reqBuf, uint8_t* replyBuf,
- size_t* dataLen);
+ipmi_ret_t CableCheck(const uint8_t* reqBuf, uint8_t* replyBuf, size_t* dataLen,
+ const HandlerInterface* handler = &handlerImpl);
} // namespace ipmi
} // namespace google
diff --git a/errors.hpp b/errors.hpp
new file mode 100644
index 0000000..7d392a7
--- /dev/null
+++ b/errors.hpp
@@ -0,0 +1,40 @@
+#pragma once
+
+#include <exception>
+#include <string>
+
+namespace google
+{
+namespace ipmi
+{
+
+/**
+ * This can be used by the Handler object to throw an exception and suggest an
+ * IPMI return code to use for the error.
+ */
+class IpmiException : public std::exception
+{
+ public:
+ explicit IpmiException(int ipmicc) :
+ _message("IPMI Code Received: " + std::to_string(ipmicc)),
+ _ipmicc(ipmicc)
+ {
+ }
+
+ virtual const char* what() const noexcept override
+ {
+ return _message.c_str();
+ }
+
+ int getIpmiError() const
+ {
+ return _ipmicc;
+ }
+
+ private:
+ std::string _message;
+ int _ipmicc;
+};
+
+} // namespace ipmi
+} // namespace google
diff --git a/handler.cpp b/handler.cpp
index 6a346ba..8d06f36 100644
--- a/handler.cpp
+++ b/handler.cpp
@@ -16,6 +16,17 @@
#include "handler.hpp"
+#include "errors.hpp"
+
+#include <ipmid/api.h>
+
+#include <cstdio>
+#include <filesystem>
+#include <fstream>
+#include <sstream>
+#include <string>
+#include <tuple>
+
// The phosphor-host-ipmi daemon requires a configuration that maps
// the if_name to the IPMI LAN channel. However, that doesn't strictly
// define which is meant to be used for NCSI.
@@ -36,12 +47,53 @@
{
namespace ipmi
{
+namespace fs = std::filesystem;
std::tuple<std::uint8_t, std::string> Handler::getEthDetails() const
{
return std::make_tuple(NCSI_IPMI_CHANNEL, NCSI_IF_NAME_STR);
}
+std::int64_t Handler::getRxPackets(const std::string& name) const
+{
+ std::ostringstream opath;
+ opath << "/sys/class/net/" << name << "/statistics/rx_packets";
+ std::string path = opath.str();
+
+ // Minor sanity & security check (of course, I'm less certain if unicode
+ // comes into play here.
+ //
+ // Basically you can't easily inject ../ or /../ into the path below.
+ if (name.find("/") != std::string::npos)
+ {
+ std::fprintf(stderr, "Invalid or illegal name: '%s'\n", name.c_str());
+ throw IpmiException(IPMI_CC_INVALID_FIELD_REQUEST);
+ }
+
+ std::error_code ec;
+ if (!fs::exists(path, ec))
+ {
+ std::fprintf(stderr, "Path: '%s' doesn't exist.\n", path.c_str());
+ throw IpmiException(IPMI_CC_INVALID_FIELD_REQUEST);
+ }
+ // We're uninterested in the state of ec.
+
+ int64_t count = 0;
+ std::ifstream ifs;
+ ifs.exceptions(std::ifstream::failbit);
+ try
+ {
+ ifs.open(path);
+ ifs >> count;
+ }
+ catch (std::ios_base::failure& fail)
+ {
+ throw IpmiException(IPMI_CC_UNSPECIFIED_ERROR);
+ }
+
+ return count;
+}
+
Handler handlerImpl;
} // namespace ipmi
diff --git a/handler.hpp b/handler.hpp
index cad2916..e2e10fa 100644
--- a/handler.hpp
+++ b/handler.hpp
@@ -20,6 +20,15 @@
* @return tuple of ethernet details (channel, if name).
*/
virtual std::tuple<std::uint8_t, std::string> getEthDetails() const = 0;
+
+ /**
+ * Return the value of rx_packets, given a if_name.
+ *
+ * @param[in] name, the interface name.
+ * @return the number of packets received.
+ * @throw IpmiException on failure.
+ */
+ virtual std::int64_t getRxPackets(const std::string& name) const = 0;
};
class Handler : public HandlerInterface
@@ -29,6 +38,7 @@
~Handler() = default;
std::tuple<std::uint8_t, std::string> getEthDetails() const override;
+ std::int64_t getRxPackets(const std::string& name) const override;
};
extern Handler handlerImpl;
diff --git a/test/Makefile.am b/test/Makefile.am
index fea204b..90e42e1 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -22,3 +22,7 @@
check_PROGRAMS += eth_unittest
eth_unittest_SOURCES = eth_unittest.cpp
eth_unittest_LDADD = $(top_builddir)/libsyscmds_common.la
+
+check_PROGRAMS += cable_unittest
+cable_unittest_SOURCES = cable_unittest.cpp
+cable_unittest_LDADD = $(top_builddir)/libsyscmds_common.la
diff --git a/test/cable_unittest.cpp b/test/cable_unittest.cpp
new file mode 100644
index 0000000..ed45c1b
--- /dev/null
+++ b/test/cable_unittest.cpp
@@ -0,0 +1,89 @@
+#include "cable.hpp"
+#include "handler_mock.hpp"
+#include "main.hpp"
+
+#include <cstdint>
+#include <cstring>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#define MAX_IPMI_BUFFER 64
+
+using ::testing::Return;
+using ::testing::StrEq;
+
+namespace google
+{
+namespace ipmi
+{
+
+TEST(CableCommandTest, RequestTooSmall)
+{
+ std::vector<std::uint8_t> request = {SysOEMCommands::SysCableCheck};
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ HandlerMock hMock;
+
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ CableCheck(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(CableCommandTest, FailsLengthSanityCheck)
+{
+ // Minimum is three bytes, but a length of zero for the string is invalid.
+ std::vector<std::uint8_t> request = {SysOEMCommands::SysCableCheck, 0x00,
+ 'a'};
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ HandlerMock hMock;
+
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ CableCheck(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(CableCommandTest, LengthTooLongForPacket)
+{
+ // The length of a the string, as specified is longer than string provided.
+ std::vector<std::uint8_t> request = {SysOEMCommands::SysCableCheck, 0x02,
+ 'a'};
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ HandlerMock hMock;
+
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ CableCheck(request.data(), reply, &dataLen, &hMock));
+}
+
+TEST(CableCommandTest, ValidRequestValidReturn)
+{
+ std::vector<std::uint8_t> request = {SysOEMCommands::SysCableCheck, 0x01,
+ 'a'};
+
+ size_t dataLen = request.size();
+ std::uint8_t reply[MAX_IPMI_BUFFER];
+
+ HandlerMock hMock;
+
+ EXPECT_CALL(hMock, getRxPackets(StrEq("a"))).WillOnce(Return(0));
+ EXPECT_EQ(IPMI_CC_OK, CableCheck(request.data(), reply, &dataLen, &hMock));
+
+ // Check results.
+ struct CableReply expectedReply, actualReply;
+ expectedReply.subcommand = SysOEMCommands::SysCableCheck;
+ expectedReply.value = 0;
+
+ EXPECT_EQ(sizeof(expectedReply), dataLen);
+ std::memcpy(&actualReply, reply, dataLen);
+
+ EXPECT_EQ(expectedReply.subcommand, actualReply.subcommand);
+ EXPECT_EQ(expectedReply.value, actualReply.value);
+}
+
+} // namespace ipmi
+} // namespace google
diff --git a/test/handler_mock.hpp b/test/handler_mock.hpp
index 8507f50..a99f131 100644
--- a/test/handler_mock.hpp
+++ b/test/handler_mock.hpp
@@ -16,6 +16,7 @@
~HandlerMock() = default;
MOCK_CONST_METHOD0(getEthDetails, std::tuple<std::uint8_t, std::string>());
+ MOCK_CONST_METHOD1(getRxPackets, std::int64_t(const std::string&));
};
} // namespace ipmi
diff --git a/test/handler_unittest.cpp b/test/handler_unittest.cpp
index 05edc0a..c1f4093 100644
--- a/test/handler_unittest.cpp
+++ b/test/handler_unittest.cpp
@@ -1,3 +1,4 @@
+#include "errors.hpp"
#include "handler.hpp"
#include <string>
@@ -20,6 +21,12 @@
EXPECT_STREQ("eth0", std::get<1>(result).c_str());
}
+TEST(HandlerTest, CableCheckIllegalPath)
+{
+ Handler h;
+ EXPECT_THROW(h.getRxPackets("eth0/../../"), IpmiException);
+}
+
// TODO: Add checks for other functions of handler.
} // namespace ipmi