Separate IPMI and legacy snoop code to different files
snoopd contains two implementations that have very little in common.
Separate the IPMI code (functions and global objects) into its own file
to reduce #ifdefs and make it easier to understand. Better separate the
cmdline options and help text to only be used in the applicable build
configuration.
Tested: Built with -Dsnoop=enabled and verified only -h option is
accepted. Built without -Dsnoop and verified -d/-r/-b options are
accepted.
Change-Id: I7f14469fafd9f050f6b9f356882ce9e26fb7f206
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
diff --git a/ipmisnoop/ipmisnoop.cpp b/ipmisnoop/ipmisnoop.cpp
new file mode 100644
index 0000000..76ba660
--- /dev/null
+++ b/ipmisnoop/ipmisnoop.cpp
@@ -0,0 +1,186 @@
+#include "ipmisnoop.hpp"
+
+std::vector<std::unique_ptr<IpmiPostReporter>> reporters;
+bool sevenSegmentLedEnabled = true;
+std::vector<gpiod::line> led_lines;
+
+uint32_t getSelectorPosition(sdbusplus::bus_t& bus)
+{
+ const std::string propertyName = "Position";
+
+ auto method = bus.new_method_call(selectorService.c_str(),
+ selectorObject.c_str(),
+ "org.freedesktop.DBus.Properties", "Get");
+ method.append(selectorIface.c_str(), propertyName);
+
+ try
+ {
+ std::variant<uint32_t> value{};
+ auto reply = bus.call(method);
+ reply.read(value);
+ return std::get<uint32_t>(value);
+ }
+ catch (const sdbusplus::exception_t& ex)
+ {
+ std::cerr << "GetProperty call failed. " << ex.what() << std::endl;
+ return 0;
+ }
+}
+
+// Configure the seven segment display connected GPIOs direction
+static int configGPIODirOutput()
+{
+ std::string gpioStr;
+ // Need to define gpio names LED_POST_CODE_0 to 8 in dts file
+ std::string gpioName = "LED_POST_CODE_";
+ const int value = 0;
+
+ for (int iteration = 0; iteration < 8; iteration++)
+ {
+ gpioStr = gpioName + std::to_string(iteration);
+ gpiod::line gpioLine = gpiod::find_line(gpioStr);
+
+ if (!gpioLine)
+ {
+ std::string errMsg = "Failed to find the " + gpioStr + " line";
+ std::cerr << errMsg.c_str() << std::endl;
+
+ /* sevenSegmentLedEnabled flag is unset when GPIO pins are not there
+ * 7 seg display for fewer platforms.
+ */
+ sevenSegmentLedEnabled = false;
+ return -1;
+ }
+
+ led_lines.push_back(gpioLine);
+ // Request GPIO output to specified value
+ try
+ {
+ gpioLine.request({__FUNCTION__,
+ gpiod::line_request::DIRECTION_OUTPUT,
+ gpiod::line_request::FLAG_ACTIVE_LOW},
+ value);
+ }
+ catch (std::exception&)
+ {
+ std::string errMsg = "Failed to request " + gpioStr + " output";
+ std::cerr << errMsg.c_str() << std::endl;
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+// Display the received postcode into seven segment display
+int IpmiPostReporter::postCodeDisplay(uint8_t status)
+{
+ for (int iteration = 0; iteration < 8; iteration++)
+ {
+ // split byte to write into GPIOs
+ int value = !((status >> iteration) & 0x01);
+
+ led_lines[iteration].set_value(value);
+ }
+ return 0;
+}
+
+void IpmiPostReporter::getSelectorPositionSignal(sdbusplus::bus_t& bus)
+{
+ constexpr uint8_t minPositionVal = 0;
+ constexpr uint8_t maxPositionVal = 5;
+
+ size_t posVal = 0;
+
+ static auto matchSignal = std::make_unique<sdbusplus::bus::match_t>(
+ bus,
+ sdbusplus::bus::match::rules::propertiesChanged(selectorObject,
+ selectorIface),
+ [&](sdbusplus::message_t& msg) {
+ std::string objectName;
+ std::map<std::string, Selector::PropertiesVariant> msgData;
+ msg.read(objectName, msgData);
+
+ auto valPropMap = msgData.find("Position");
+ {
+ if (valPropMap == msgData.end())
+ {
+ std::cerr << "Position property not found " << std::endl;
+ return;
+ }
+
+ posVal = std::get<size_t>(valPropMap->second);
+
+ if (posVal > minPositionVal && posVal < maxPositionVal)
+ {
+ std::tuple<uint64_t, secondary_post_code_t> postcodes =
+ reporters[posVal - 1]->value();
+ uint64_t postcode = std::get<uint64_t>(postcodes);
+
+ // write postcode into seven segment display
+ if (postCodeDisplay(postcode) < 0)
+ {
+ fprintf(stderr, "Error in display the postcode\n");
+ }
+ }
+ }
+ });
+}
+
+// handle muti-host D-bus
+int postCodeIpmiHandler(const std::string& snoopObject,
+ const std::string& snoopDbus, sdbusplus::bus_t& bus,
+ std::span<std::string> host)
+{
+ int ret = 0;
+
+ try
+ {
+ for (size_t iteration = 0; iteration < host.size(); iteration++)
+ {
+ std::string objPathInst = snoopObject + host[iteration];
+
+ sdbusplus::server::manager_t m{bus, objPathInst.c_str()};
+
+ /* Create a monitor object and let it do all the rest */
+ reporters.emplace_back(
+ std::make_unique<IpmiPostReporter>(bus, objPathInst.c_str()));
+
+ reporters[iteration]->emit_object_added();
+ }
+
+ bus.request_name(snoopDbus.c_str());
+
+ /* sevenSegmentLedEnabled flag is unset when GPIO pins are not there 7
+ seg display for fewer platforms. So, the code for postcode dispay and
+ Get Selector position can be skipped in those platforms.
+ */
+ if (sevenSegmentLedEnabled)
+ {
+ reporters[0]->getSelectorPositionSignal(bus);
+ }
+ else
+ {
+ reporters.clear();
+ }
+ }
+ catch (const std::exception& e)
+ {
+ fprintf(stderr, "%s\n", e.what());
+ }
+
+ // Configure seven segment dsiplay connected to GPIOs as output
+ ret = configGPIODirOutput();
+ if (ret < 0)
+ {
+ fprintf(stderr, "Failed find the gpio line. Cannot display postcodes "
+ "in seven segment display..\n");
+ }
+
+ while (true)
+ {
+ bus.process_discard();
+ bus.wait();
+ }
+ exit(EXIT_SUCCESS);
+}
diff --git a/ipmisnoop/ipmisnoop.hpp b/ipmisnoop/ipmisnoop.hpp
index 77311b2..d5db5dd 100644
--- a/ipmisnoop/ipmisnoop.hpp
+++ b/ipmisnoop/ipmisnoop.hpp
@@ -14,6 +14,7 @@
#include <filesystem>
#include <iostream>
+#include <span>
const std::string ipmiSnoopObject = "/xyz/openbmc_project/state/boot/raw";
@@ -21,15 +22,13 @@
const int maxPostcode = 255;
const int maxPosition = 4;
-bool sevenSegmentLedEnabled = true;
+extern bool sevenSegmentLedEnabled;
-std::vector<gpiod::line> led_lines;
+extern std::vector<gpiod::line> led_lines;
using Selector =
sdbusplus::xyz::openbmc_project::Chassis::Buttons::server::HostSelector;
-std::unique_ptr<sdbusplus::bus::match_t> matchSignal;
-
const std::string selectorService = "xyz.openbmc_project.Chassis.Buttons";
const std::string selectorObject =
"/xyz/openbmc_project/Chassis/Buttons/HostSelector";
@@ -40,28 +39,11 @@
const std::string rawIface = "xyz.openbmc_project.State.Boot.Raw";
const std::string rawService = "xyz.openbmc_project.State.Boot.Raw";
-uint32_t getSelectorPosition(sdbusplus::bus_t& bus)
-{
- const std::string propertyName = "Position";
+int postCodeIpmiHandler(const std::string& snoopObject,
+ const std::string& snoopDbus, sdbusplus::bus_t& bus,
+ std::span<std::string> host);
- auto method = bus.new_method_call(selectorService.c_str(),
- selectorObject.c_str(),
- "org.freedesktop.DBus.Properties", "Get");
- method.append(selectorIface.c_str(), propertyName);
-
- try
- {
- std::variant<uint32_t> value{};
- auto reply = bus.call(method);
- reply.read(value);
- return std::get<uint32_t>(value);
- }
- catch (const sdbusplus::exception_t& ex)
- {
- std::cerr << "GetProperty call failed. " << ex.what() << std::endl;
- return 0;
- }
-}
+uint32_t getSelectorPosition(sdbusplus::bus_t& bus);
struct IpmiPostReporter : PostObject
{
@@ -147,61 +129,3 @@
int postCodeDisplay(uint8_t);
void getSelectorPositionSignal(sdbusplus::bus_t& bus);
};
-
-// Configure the seven segment display connected GPIOs direction
-int configGPIODirOutput()
-{
- std::string gpioStr;
- // Need to define gpio names LED_POST_CODE_0 to 8 in dts file
- std::string gpioName = "LED_POST_CODE_";
- const int value = 0;
-
- for (int iteration = 0; iteration < 8; iteration++)
- {
- gpioStr = gpioName + std::to_string(iteration);
- gpiod::line gpioLine = gpiod::find_line(gpioStr);
-
- if (!gpioLine)
- {
- std::string errMsg = "Failed to find the " + gpioStr + " line";
- std::cerr << errMsg.c_str() << std::endl;
-
- /* sevenSegmentLedEnabled flag is unset when GPIO pins are not there
- * 7 seg display for fewer platforms.
- */
- sevenSegmentLedEnabled = false;
- return -1;
- }
-
- led_lines.push_back(gpioLine);
- // Request GPIO output to specified value
- try
- {
- gpioLine.request({__FUNCTION__,
- gpiod::line_request::DIRECTION_OUTPUT,
- gpiod::line_request::FLAG_ACTIVE_LOW},
- value);
- }
- catch (std::exception&)
- {
- std::string errMsg = "Failed to request " + gpioStr + " output";
- std::cerr << errMsg.c_str() << std::endl;
- return -1;
- }
- }
-
- return 0;
-}
-
-// Display the received postcode into seven segment display
-int IpmiPostReporter::postCodeDisplay(uint8_t status)
-{
- for (int iteration = 0; iteration < 8; iteration++)
- {
- // split byte to write into GPIOs
- int value = !((status >> iteration) & 0x01);
-
- led_lines[iteration].set_value(value);
- }
- return 0;
-}
diff --git a/main.cpp b/main.cpp
index 53a075e..2c9c7c2 100644
--- a/main.cpp
+++ b/main.cpp
@@ -17,7 +17,6 @@
#ifdef ENABLE_IPMI_SNOOP
#include "ipmisnoop/ipmisnoop.hpp"
#endif
-
#include "lpcsnoop/snoop.hpp"
#include <endian.h>
@@ -41,78 +40,26 @@
#include <functional>
#include <iostream>
#include <optional>
-#include <span>
#include <thread>
-#ifdef ENABLE_IPMI_SNOOP
-#include <xyz/openbmc_project/State/Boot/Raw/server.hpp>
-#endif
-
static size_t codeSize = 1; /* Size of each POST code in bytes */
-const char* defaultHostInstances = "0";
static bool verbose = false;
-#ifdef ENABLE_IPMI_SNOOP
-const uint8_t minPositionVal = 0;
-const uint8_t maxPositionVal = 5;
-#endif
-
-#ifdef ENABLE_IPMI_SNOOP
-std::vector<std::unique_ptr<IpmiPostReporter>> reporters;
-#endif
-
-#ifdef ENABLE_IPMI_SNOOP
-void IpmiPostReporter::getSelectorPositionSignal(sdbusplus::bus_t& bus)
-{
- size_t posVal = 0;
-
- matchSignal = std::make_unique<sdbusplus::bus::match_t>(
- bus,
- sdbusplus::bus::match::rules::propertiesChanged(selectorObject,
- selectorIface),
- [&](sdbusplus::message_t& msg) {
- std::string objectName;
- std::map<std::string, Selector::PropertiesVariant> msgData;
- msg.read(objectName, msgData);
-
- auto valPropMap = msgData.find("Position");
- {
- if (valPropMap == msgData.end())
- {
- std::cerr << "Position property not found " << std::endl;
- return;
- }
-
- posVal = std::get<size_t>(valPropMap->second);
-
- if (posVal > minPositionVal && posVal < maxPositionVal)
- {
- std::tuple<uint64_t, secondary_post_code_t> postcodes =
- reporters[posVal - 1]->value();
- uint64_t postcode = std::get<uint64_t>(postcodes);
-
- // write postcode into seven segment display
- if (postCodeDisplay(postcode) < 0)
- {
- fprintf(stderr, "Error in display the postcode\n");
- }
- }
- }
- });
-}
-#endif
static void usage(const char* name)
{
fprintf(stderr,
- "Usage: %s [-d <DEVICE>]\n"
- " -b, --bytes <SIZE> set POST code length to <SIZE> bytes. "
- "Default is %zu\n"
+ "Usage: %s\n"
+#ifdef ENABLE_IPMI_SNOOP
+ " -h, --host <host instances> Default is '0'\n"
+#else
" -d, --device <DEVICE> use <DEVICE> file.\n"
" -r, --rate-limit=<N> Only process N POST codes from the "
"device per second.\n"
- " -h, --host <host instances> . Default is '%s'\n"
+ " -b, --bytes <SIZE> set POST code length to <SIZE> bytes. "
+ "Default is 1\n"
+#endif
" -v, --verbose Prints verbose information while running\n\n",
- name, codeSize, defaultHostInstances);
+ name);
}
/**
@@ -225,66 +172,6 @@
s.get_event().exit(1);
}
-#ifdef ENABLE_IPMI_SNOOP
-// handle muti-host D-bus
-int postCodeIpmiHandler(const std::string& snoopObject,
- const std::string& snoopDbus, sdbusplus::bus_t& bus,
- std::span<std::string> host)
-{
- int ret = 0;
-
- try
- {
- for (size_t iteration = 0; iteration < host.size(); iteration++)
- {
- std::string objPathInst = snoopObject + host[iteration];
-
- sdbusplus::server::manager_t m{bus, objPathInst.c_str()};
-
- /* Create a monitor object and let it do all the rest */
- reporters.emplace_back(
- std::make_unique<IpmiPostReporter>(bus, objPathInst.c_str()));
-
- reporters[iteration]->emit_object_added();
- }
-
- bus.request_name(snoopDbus.c_str());
-
- /* sevenSegmentLedEnabled flag is unset when GPIO pins are not there 7
- seg display for fewer platforms. So, the code for postcode dispay and
- Get Selector position can be skipped in those platforms.
- */
- if (sevenSegmentLedEnabled)
- {
- reporters[0]->getSelectorPositionSignal(bus);
- }
- else
- {
- reporters.clear();
- }
- }
- catch (const std::exception& e)
- {
- fprintf(stderr, "%s\n", e.what());
- }
-
- // Configure seven segment dsiplay connected to GPIOs as output
- ret = configGPIODirOutput();
- if (ret < 0)
- {
- fprintf(stderr, "Failed find the gpio line. Cannot display postcodes "
- "in seven segment display..\n");
- }
-
- while (true)
- {
- bus.process_discard();
- bus.wait();
- }
- exit(EXIT_SUCCESS);
-}
-#endif
-
/*
* TODO(venture): this only listens one of the possible snoop ports, but
* doesn't share the namespace.
@@ -294,47 +181,41 @@
*/
int main(int argc, char* argv[])
{
-#ifndef ENABLE_IPMI_SNOOP
int postFd = -1;
unsigned int rateLimit = 0;
-#endif
int opt;
-#ifdef ENABLE_IPMI_SNOOP
std::vector<std::string> host;
-#endif
- /*
- * These string constants are only used in this method within this object
- * and this object is the only object feeding into the final binary.
- *
- * If however, another object is added to this binary it would be proper
- * to move these declarations to be global and extern to the other object.
- */
// clang-format off
static const struct option long_options[] = {
- #ifdef ENABLE_IPMI_SNOOP
+#ifdef ENABLE_IPMI_SNOOP
{"host", optional_argument, NULL, 'h'},
- #endif
- {"bytes", required_argument, NULL, 'b'},
- #ifndef ENABLE_IPMI_SNOOP
+#else
{"device", optional_argument, NULL, 'd'},
{"rate-limit", optional_argument, NULL, 'r'},
- #endif
+ {"bytes", required_argument, NULL, 'b'},
+#endif
{"verbose", no_argument, NULL, 'v'},
{0, 0, 0, 0}
};
// clang-format on
- while ((opt = getopt_long(argc, argv, "h:b:d:r:v", long_options, NULL)) !=
- -1)
+ constexpr const char* optstring =
+#ifdef ENABLE_IPMI_SNOOP
+ "h:"
+#else
+ "d:r:b:"
+#endif
+ "v";
+
+ while ((opt = getopt_long(argc, argv, optstring, long_options, NULL)) != -1)
{
switch (opt)
{
case 0:
break;
-#ifdef ENABLE_IPMI_SNOOP
case 'h':
{
std::string_view instances = optarg;
@@ -348,7 +229,6 @@
host.emplace_back(instances);
break;
}
-#endif
case 'b':
{
codeSize = atoi(optarg);
@@ -363,7 +243,6 @@
}
break;
}
-#ifndef ENABLE_IPMI_SNOOP
case 'd':
postFd = open(optarg, O_NONBLOCK);
@@ -395,7 +274,6 @@
argVal);
break;
}
-#endif
case 'v':
verbose = true;
break;
@@ -418,8 +296,6 @@
return 0;
#endif
-#ifndef ENABLE_IPMI_SNOOP
-
bool deferSignals = true;
// Add systemd object manager.
@@ -464,5 +340,4 @@
}
return 0;
-#endif
}
diff --git a/meson.build b/meson.build
index 398693e..b317d1a 100644
--- a/meson.build
+++ b/meson.build
@@ -25,13 +25,14 @@
conf_data.set('bindir', get_option('prefix') / get_option('bindir'))
conf_data.set('SYSTEMD_TARGET', get_option('systemd-target'))
-snoopd_args = '-b ' + get_option('post-code-bytes').to_string()
-if not get_option('snoop').disabled()
- add_project_arguments('-DENABLE_IPMI_SNOOP',language:'cpp')
- snoopd_args += ' -h "' + get_option('host-instances') + '"'
-endif
-
-if get_option('snoop-device') != ''
+snoopd_src = ['main.cpp']
+snoopd_args = ''
+if get_option('snoop').enabled()
+ snoopd_src += 'ipmisnoop/ipmisnoop.cpp'
+ add_project_arguments('-DENABLE_IPMI_SNOOP',language:'cpp')
+ snoopd_args += ' -h "' + get_option('host-instances') + '"'
+elif get_option('snoop-device') != ''
+ snoopd_args += '-b ' + get_option('post-code-bytes').to_string()
snoopd_args += ' -d /dev/' + get_option('snoop-device')
rate_limit = get_option('rate-limit')
if rate_limit > 0
@@ -50,7 +51,7 @@
executable(
'snoopd',
- 'main.cpp',
+ snoopd_src,
dependencies: [
sdbusplus,
sdeventplus,