Add Mutex Guard around open in sendPackage
The `fd` is current thread unsafe and could potentially be
access/overridden by multiple thread. Add the mutex around the open() in
sendPackage to make sure it is only called once.
Tested: Added unit tests to make sure we are thread safe.
Change-Id: I2801bace2c816ed3454c3f79b57609aa8f6ecc33
Signed-off-by: Willy Tu <wltu@google.com>
diff --git a/src/ipmiblob/ipmi_handler.cpp b/src/ipmiblob/ipmi_handler.cpp
index b51718a..3ce5d92 100644
--- a/src/ipmiblob/ipmi_handler.cpp
+++ b/src/ipmiblob/ipmi_handler.cpp
@@ -27,7 +27,9 @@
#include <atomic>
#include <cstdint>
#include <cstring>
+#include <functional>
#include <memory>
+#include <mutex>
#include <sstream>
#include <string>
#include <vector>
@@ -42,11 +44,16 @@
void IpmiHandler::open()
{
- const int device = 0;
- const std::vector<std::string> formats = {"/dev/ipmi", "/dev/ipmi/",
- "/dev/ipmidev/"};
+ std::lock_guard<std::mutex> guard(openMutex);
+ if (fd >= 0)
+ {
+ return;
+ }
- for (const auto& format : formats)
+ constexpr int device = 0;
+ const std::array<std::string, 3> formats = {"/dev/ipmi", "/dev/ipmi/",
+ "/dev/ipmidev/"};
+ for (const std::string& format : formats)
{
std::ostringstream path;
path << format << device;
@@ -69,10 +76,7 @@
IpmiHandler::sendPacket(std::uint8_t netfn, std::uint8_t cmd,
std::vector<std::uint8_t>& data)
{
- if (fd < 0)
- {
- open();
- }
+ open();
constexpr int ipmiOEMLun = 0;
constexpr int fifteenMs = 15 * 1000;
@@ -126,7 +130,7 @@
{
continue;
}
- throw IpmiException("Error occurred.");
+ throw IpmiException("Polling Error occurred.");
}
else if (rc == 0)
{
diff --git a/src/ipmiblob/ipmi_handler.hpp b/src/ipmiblob/ipmi_handler.hpp
index fedc068..2564e37 100644
--- a/src/ipmiblob/ipmi_handler.hpp
+++ b/src/ipmiblob/ipmi_handler.hpp
@@ -5,6 +5,7 @@
#include <atomic>
#include <memory>
+#include <mutex>
#include <vector>
namespace ipmiblob
@@ -49,6 +50,9 @@
int fd = -1;
/* The last IPMI sequence number we used. */
std::atomic_int sequence = 0;
+
+ // Protect the open fd between different threads
+ std::mutex openMutex;
};
} // namespace ipmiblob
diff --git a/test/tools_ipmi_unittest.cpp b/test/tools_ipmi_unittest.cpp
index 872e08b..3890bc8 100644
--- a/test/tools_ipmi_unittest.cpp
+++ b/test/tools_ipmi_unittest.cpp
@@ -1,23 +1,254 @@
#include "internal_sys_mock.hpp"
+#include <linux/ipmi.h>
+#include <sys/ioctl.h>
+
#include <ipmiblob/ipmi_errors.hpp>
#include <ipmiblob/ipmi_handler.hpp>
+#include <chrono>
+#include <cstring>
+#include <thread>
+
namespace ipmiblob
{
+using std::chrono::milliseconds;
using ::testing::_;
+using ::testing::DoAll;
+using ::testing::ElementsAre;
using ::testing::Return;
-TEST(IpmiHandlerTest, OpenAllFails)
+ACTION_TEMPLATE(SetArgNPointeeTo, HAS_1_TEMPLATE_PARAMS(unsigned, uIndex),
+ AND_2_VALUE_PARAMS(pData, uiDataSize))
+{
+ ipmi_recv* reply = reinterpret_cast<ipmi_recv*>(std::get<uIndex>(args));
+ std::memcpy(reply->msg.data, pData, uiDataSize);
+ reply->msg.data_len = uiDataSize;
+}
+
+ACTION_TEMPLATE(SetOpenDelays, HAS_1_TEMPLATE_PARAMS(unsigned, delay),
+ AND_0_VALUE_PARAMS())
+{
+ std::this_thread::sleep_for(milliseconds(delay));
+}
+
+class IpmiHandlerTest : public ::testing::Test
+{
+ protected:
+ IpmiHandlerTest() : sysMock(std::make_unique<internal::InternalSysMock>())
+ {}
+
+ void ExpectOpenError(IpmiHandler& ipmi, std::string_view msg)
+ {
+ EXPECT_THROW(
+ {
+ try
+ {
+ ipmi.open();
+ }
+ catch (const IpmiException& e)
+ {
+ // and this tests that it has the correct message
+ EXPECT_STREQ(msg.data(), e.what());
+ throw;
+ }
+ },
+ IpmiException);
+ }
+
+ void ExpectSendPacketError(IpmiHandler& ipmi, std::string_view msg)
+ {
+ EXPECT_THROW(
+ {
+ try
+ {
+ ipmi.sendPacket(0, 0, data);
+ }
+ catch (const IpmiException& e)
+ {
+ // and this tests that it has the correct message
+ EXPECT_STREQ(msg.data(), e.what());
+ throw;
+ }
+ },
+ IpmiException);
+ }
+
+ std::unique_ptr<internal::InternalSysMock> sysMock;
+ std::vector<std::uint8_t> data;
+ int fd = 1;
+ int badFd = -1;
+};
+
+TEST_F(IpmiHandlerTest, OpenAllFails)
{
/* Open against all device files fail. */
- std::unique_ptr<internal::InternalSysMock> sysMock =
- std::make_unique<internal::InternalSysMock>();
- EXPECT_CALL(*sysMock, open(_, _)).WillRepeatedly(Return(-1));
+ EXPECT_CALL(*sysMock, open(_, _)).WillRepeatedly(Return(badFd));
IpmiHandler ipmi(std::move(sysMock));
- EXPECT_THROW(ipmi.open(), IpmiException);
+ ExpectOpenError(ipmi, "Unable to open any ipmi devices");
+}
+
+TEST_F(IpmiHandlerTest, OpenSucess)
+{
+ // Make sure that we don't throw any exception when getting bad file
+ // descriptor Return valid file descriptor at the last ipmi device for a
+ // succeful open().
+ EXPECT_CALL(*sysMock, open(_, _))
+ .WillOnce(Return(badFd))
+ .WillOnce(Return(badFd))
+ .WillOnce(Return(fd));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ EXPECT_NO_THROW(ipmi.open());
+}
+
+TEST_F(IpmiHandlerTest, SendPacketOpenAllFails)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillRepeatedly(Return(badFd));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Unable to open any ipmi devices");
+ ExpectSendPacketError(ipmi, "Unable to open any ipmi devices");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketRequestFailed)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(badFd));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Unable to send IPMI request.");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketPollingError)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(badFd));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Polling Error occurred.");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketPollTimeout)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(0));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Timeout waiting for reply.");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketBadIpmiReply)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(1));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_RECEIVE_MSG_TRUNC, _))
+ .WillOnce(Return(badFd));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Unable to read reply.");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketNotIpmiOk)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(1));
+
+ std::vector<std::uint8_t> expectedOutput = {2};
+
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_RECEIVE_MSG_TRUNC, _))
+ .WillOnce(DoAll(
+ SetArgNPointeeTo<2>(expectedOutput.data(), expectedOutput.size()),
+ Return(0)));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ ExpectSendPacketError(ipmi, "Received IPMI_CC: 2");
+}
+
+TEST_F(IpmiHandlerTest, SendPacketFailedOpenOnce)
+{
+ EXPECT_CALL(*sysMock, open(_, _))
+ .WillOnce(Return(badFd))
+ .WillOnce(Return(badFd))
+ .WillOnce(Return(badFd))
+ .WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(1));
+
+ std::vector<std::uint8_t> expectedOutput = {0, 1, 2, 3};
+
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_RECEIVE_MSG_TRUNC, _))
+ .WillOnce(DoAll(
+ SetArgNPointeeTo<2>(expectedOutput.data(), expectedOutput.size()),
+ Return(0)));
+
+ IpmiHandler ipmi(std::move(sysMock));
+
+ ExpectSendPacketError(ipmi, "Unable to open any ipmi devices");
+ EXPECT_THAT(ipmi.sendPacket(0, 0, data), ElementsAre(1, 2, 3));
+}
+
+TEST_F(IpmiHandlerTest, SendPacketSucess)
+{
+ EXPECT_CALL(*sysMock, open(_, _)).WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(0));
+ EXPECT_CALL(*sysMock, poll(_, 1, _)).WillOnce(Return(1));
+
+ std::vector<std::uint8_t> expectedOutput = {0, 1, 2, 3};
+
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_RECEIVE_MSG_TRUNC, _))
+ .WillOnce(DoAll(
+ SetArgNPointeeTo<2>(expectedOutput.data(), expectedOutput.size()),
+ Return(0)));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ EXPECT_THAT(ipmi.sendPacket(0, 0, data), ElementsAre(1, 2, 3));
+}
+
+// Tried to call open() in different thread and making sure that both thread
+// tried it and there aree no data race. Expect the first thread to fail to
+// open() and second one pass open(), but failed in the IPMICTL_SEND_COMMAND.
+TEST_F(IpmiHandlerTest, SendPacketTriedOpenInParallel)
+{
+ EXPECT_CALL(*sysMock, open(_, _))
+ // The badFd is expected to be used in testOpenParallel0 and need enough
+ // delay to make sure that testOpenParallel1 starts before
+ // testOpenParallel0 finishes.
+ .WillOnce(DoAll(SetOpenDelays<10>(), Return(badFd)))
+ .WillOnce(DoAll(SetOpenDelays<10>(), Return(badFd)))
+ .WillOnce(DoAll(SetOpenDelays<10>(), Return(badFd)))
+ .WillOnce(Return(fd));
+ EXPECT_CALL(*sysMock, ioctl(fd, IPMICTL_SEND_COMMAND, _))
+ .WillOnce(Return(-1));
+
+ IpmiHandler ipmi(std::move(sysMock));
+ auto testOpenParallel0 = [this, &ipmi]() {
+ ExpectSendPacketError(ipmi, "Unable to open any ipmi devices");
+ };
+
+ auto testOpenParallel1 = [this, &ipmi]() {
+ // Make sure this start after testOpenParallel0 get to the open()
+ std::this_thread::sleep_for(milliseconds(10));
+ ExpectSendPacketError(ipmi, "Unable to send IPMI request.");
+ };
+
+ std::thread t1(testOpenParallel0);
+ std::thread t2(testOpenParallel1);
+ t1.join();
+ t2.join();
}
} // namespace ipmiblob