PEL: Don't allow duplicate callouts
When adding callouts, check if the callout being added already exists.
If it does, don't add it and rather just update the priority of the
existing one to be the highest of the two.
Callouts are considered to be equal if their location code matches, or
if they have the same maintenance procedure, or if they have the same
symbolic FRU.
The change entails adding an operator== on the Callout object, as well
as an operator> so that the callout with the highest priority can be
determined. Also use this new operator> in the sort of the callout list
that happens after a callout is added or changed.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
Change-Id: I5ee148cc12e92f67fb3da233c3615e9665e95355
diff --git a/extensions/openpower-pels/callout.cpp b/extensions/openpower-pels/callout.cpp
index 0c45950..705b1e4 100644
--- a/extensions/openpower-pels/callout.cpp
+++ b/extensions/openpower-pels/callout.cpp
@@ -205,6 +205,54 @@
}
}
+bool Callout::operator==(const Callout& right) const
+{
+ if ((_locationCodeSize != 0) || (right.locationCodeSize() != 0))
+ {
+ return locationCode() == right.locationCode();
+ }
+
+ if (!_fruIdentity || !right.fruIdentity())
+ {
+ return false;
+ }
+
+ auto myProc = _fruIdentity->getMaintProc();
+ auto otherProc = right.fruIdentity()->getMaintProc();
+ if (myProc)
+ {
+ if (otherProc)
+ {
+ return *myProc == *otherProc;
+ }
+ return false;
+ }
+
+ auto myPN = _fruIdentity->getPN();
+ auto otherPN = right.fruIdentity()->getPN();
+ if (myPN && otherPN)
+ {
+ return *myPN == *otherPN;
+ }
+
+ return false;
+}
+
+bool Callout::operator>(const Callout& right) const
+{
+ // Treat all of the mediums the same
+ const std::map<std::uint8_t, int> priorities = {
+ {'H', 10}, {'M', 9}, {'A', 9}, {'B', 9}, {'C', 9}, {'L', 8}};
+
+ if (!priorities.contains(priority()) ||
+ !priorities.contains(right.priority()))
+ {
+ return false;
+ }
+
+ return priorities.at(priority()) > priorities.at(right.priority());
+}
+
} // namespace src
} // namespace pels
} // namespace openpower
diff --git a/extensions/openpower-pels/callout.hpp b/extensions/openpower-pels/callout.hpp
index 9c99494..7ac0727 100644
--- a/extensions/openpower-pels/callout.hpp
+++ b/extensions/openpower-pels/callout.hpp
@@ -198,6 +198,16 @@
}
/**
+ * @brief Set the priority of the callout
+ *
+ * @param[in] priority - The priority value
+ */
+ void setPriority(uint8_t priority)
+ {
+ _priority = priority;
+ }
+
+ /**
* @brief Returns the location code of the callout
*
* @return std::string - The location code
@@ -253,6 +263,25 @@
return _mru;
}
+ /**
+ * @brief Operator == used for finding duplicate callouts
+ *
+ * Checks if the location codes then maintenance procedure
+ * value, then symbolic FRU value match.
+ *
+ * @param[in] right - The callout to compare to
+ * @return bool - true if they are the same
+ */
+ bool operator==(const Callout& right) const;
+
+ /**
+ * @brief Operator > used for sorting callouts by priority
+ *
+ * @param[in] right - The callout to compare to
+ * @return bool - true if callout has higher priority than other
+ */
+ bool operator>(const Callout& right) const;
+
private:
/**
* @brief Sets the location code field
diff --git a/extensions/openpower-pels/callouts.cpp b/extensions/openpower-pels/callouts.cpp
index 3ba3459..0cb1f7e 100644
--- a/extensions/openpower-pels/callouts.cpp
+++ b/extensions/openpower-pels/callouts.cpp
@@ -18,7 +18,6 @@
#include <phosphor-logging/log.hpp>
#include <algorithm>
-#include <map>
namespace openpower
{
@@ -53,28 +52,41 @@
void Callouts::addCallout(std::unique_ptr<Callout> callout)
{
- if (_callouts.size() < maxNumberOfCallouts)
- {
- _callouts.push_back(std::move(callout));
+ bool shouldAdd = true;
- _subsectionWordLength += _callouts.back()->flattenedSize() / 4;
- }
- else
+ // Check if there is already a callout for this FRU
+ auto it = std::ranges::find_if(
+ _callouts, [&callout](const auto& c) { return *callout == *c; });
+
+ // If the callout already exists, but the new one has a higher
+ // priority, change the existing callout's priority to this
+ // new value and don't add the new one.
+ if (it != _callouts.end())
{
- using namespace phosphor::logging;
- log<level::INFO>("Dropping PEL callout because at max");
+ if (*callout > **it)
+ {
+ (*it)->setPriority(callout->priority());
+ }
+ shouldAdd = false;
}
- // Mapping including the 3 Medium levels as A,B and C
- const std::map<std::uint8_t, int> priorities = {
- {'H', 10}, {'M', 9}, {'A', 8}, {'B', 7}, {'C', 6}, {'L', 5}};
+ if (shouldAdd)
+ {
+ if (_callouts.size() < maxNumberOfCallouts)
+ {
+ _callouts.push_back(std::move(callout));
- auto sortPriority = [&priorities](const std::unique_ptr<Callout>& p1,
- const std::unique_ptr<Callout>& p2) {
- return priorities.at(p1->priority()) > priorities.at(p2->priority());
- };
+ _subsectionWordLength += _callouts.back()->flattenedSize() / 4;
+ }
+ else
+ {
+ using namespace phosphor::logging;
+ log<level::INFO>("Dropping PEL callout because at max");
+ }
+ }
- std::sort(_callouts.begin(), _callouts.end(), sortPriority);
+ std::ranges::sort(_callouts,
+ [](const auto& c1, const auto& c2) { return *c1 > *c2; });
}
} // namespace src
diff --git a/test/openpower-pels/pel_test.cpp b/test/openpower-pels/pel_test.cpp
index a996358..cf54e9c 100644
--- a/test/openpower-pels/pel_test.cpp
+++ b/test/openpower-pels/pel_test.cpp
@@ -958,7 +958,8 @@
ffdcFile.version = 1;
// Write these callouts to a JSON file and pass it into
- // the PEL as an FFDC file.
+ // the PEL as an FFDC file. Also has a duplicate that
+ // will be removed.
auto inputJSON = R"([
{
"Priority": "H",
@@ -967,6 +968,10 @@
{
"Priority": "M",
"Procedure": "PROCEDURE"
+ },
+ {
+ "Priority": "L",
+ "Procedure": "PROCEDURE"
}
])"_json;
diff --git a/test/openpower-pels/src_callout_test.cpp b/test/openpower-pels/src_callout_test.cpp
index daa2ab6..a03b9b0 100644
--- a/test/openpower-pels/src_callout_test.cpp
+++ b/test/openpower-pels/src_callout_test.cpp
@@ -432,3 +432,128 @@
}
}
}
+
+TEST(CalloutTest, OperatorEqualTest)
+{
+ {
+ Callout c1{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ Callout c2{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ EXPECT_EQ(c1, c2);
+ }
+
+ {
+ // Different location code
+ Callout c1{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ Callout c2{CalloutPriority::high, "A2", "1234567", "ABCD",
+ "123456789ABC"};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // maintenance procedure
+ Callout c1{CalloutPriority::medium, "bmc_code"};
+ Callout c2{CalloutPriority::medium, "bmc_code"};
+ EXPECT_EQ(c1, c2);
+ }
+
+ {
+ // different maintenance procedures
+ Callout c1{CalloutPriority::medium, "bmc_code"};
+ Callout c2{CalloutPriority::medium, "sbe_code"};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // symbolic FRU
+ Callout c1{CalloutPriority::high, "service_docs", "", false};
+ Callout c2{CalloutPriority::high, "service_docs", "", false};
+ EXPECT_EQ(c1, c2);
+ }
+
+ {
+ // different symbolic FRUs
+ Callout c1{CalloutPriority::high, "service_docs", "", false};
+ Callout c2{CalloutPriority::high, "air_mover", "", false};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // HW callout vs symbolic FRU
+ Callout c1{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ Callout c2{CalloutPriority::high, "service_docs", "", false};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // HW callout vs maintenance procedure
+ Callout c1{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ Callout c2{CalloutPriority::medium, "bmc_code"};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // symbolic FRU vs maintenance procedure
+ Callout c1{CalloutPriority::high, "service_docs", "", false};
+ Callout c2{CalloutPriority::medium, "bmc_code"};
+ EXPECT_NE(c1, c2);
+ }
+
+ {
+ // HW callout vs symbolic FRU is still considered equal if
+ // the location code is the same
+ Callout c1{CalloutPriority::high, "A1", "1234567", "ABCD",
+ "123456789ABC"};
+ Callout c2{CalloutPriority::high, "service_docs", "A1", true};
+ EXPECT_EQ(c1, c2);
+ }
+}
+
+TEST(CalloutTest, OperatorGreaterThanTest)
+{
+ {
+ Callout c1{CalloutPriority::high, "bmc_code"};
+ Callout c2{CalloutPriority::medium, "bmc_code"};
+ EXPECT_TRUE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::high, "bmc_code"};
+ Callout c2{CalloutPriority::low, "bmc_code"};
+ EXPECT_TRUE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::medium, "bmc_code"};
+ Callout c2{CalloutPriority::low, "bmc_code"};
+ EXPECT_TRUE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::mediumGroupA, "bmc_code"};
+ Callout c2{CalloutPriority::low, "bmc_code"};
+ EXPECT_TRUE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::medium, "bmc_code"};
+ Callout c2{CalloutPriority::high, "bmc_code"};
+ EXPECT_FALSE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::high, "bmc_code"};
+ Callout c2{CalloutPriority::high, "bmc_code"};
+ EXPECT_FALSE(c1 > c2);
+ }
+ {
+ Callout c1{CalloutPriority::low, "bmc_code"};
+ Callout c2{CalloutPriority::high, "bmc_code"};
+ EXPECT_FALSE(c1 > c2);
+ }
+ {
+ // Treat the different mediums the same
+ Callout c1{CalloutPriority::medium, "bmc_code"};
+ Callout c2{CalloutPriority::mediumGroupA, "bmc_code"};
+ EXPECT_FALSE(c1 > c2);
+ }
+}
diff --git a/test/openpower-pels/src_callouts_test.cpp b/test/openpower-pels/src_callouts_test.cpp
index 69c26b8..f13462d 100644
--- a/test/openpower-pels/src_callouts_test.cpp
+++ b/test/openpower-pels/src_callouts_test.cpp
@@ -16,6 +16,8 @@
#include "extensions/openpower-pels/callouts.hpp"
#include "pel_utils.hpp"
+#include <format>
+
#include <gtest/gtest.h>
using namespace openpower::pels;
@@ -99,8 +101,9 @@
for (size_t i = 0; i < maxNumberOfCallouts; i++)
{
+ auto loc = std::format("U1-P{}", i);
auto callout = std::make_unique<Callout>(
- CalloutPriority::high, "U1-P1", "1234567", "ABCD", "123456789ABC");
+ CalloutPriority::high, loc, "1234567", "ABCD", "123456789ABC");
auto calloutSize = callout->flattenedSize();
callouts.addCallout(std::move(callout));
@@ -200,3 +203,65 @@
EXPECT_EQ(calloutObjects[9]->locationCode(), "U1-P10");
EXPECT_EQ(calloutObjects[9]->priority(), 'L');
}
+
+TEST(CalloutsTest, TestDupCallouts)
+{
+ {
+ // Duplicate callouts, keep the high priority one
+ Callouts callouts;
+ auto c0 = std::make_unique<Callout>(CalloutPriority::medium, "U1-P1",
+ "1234567", "ABC", "123456789ABC");
+ callouts.addCallout(std::move(c0));
+
+ auto c1 = std::make_unique<Callout>(CalloutPriority::high, "U1-P1",
+ "1234567", "ABCD", "123456789ABC");
+ callouts.addCallout(std::move(c1));
+
+ EXPECT_EQ(callouts.callouts().size(), 1);
+ const auto& calloutObjects = callouts.callouts();
+ EXPECT_EQ(calloutObjects[0]->priority(), 'H');
+ }
+
+ {
+ // Different callouts, keep them both
+ Callouts callouts;
+ auto c0 = std::make_unique<Callout>(CalloutPriority::high, "U1-P1",
+ "1234567", "ABC", "123456789ABC");
+ callouts.addCallout(std::move(c0));
+
+ auto c1 = std::make_unique<Callout>(CalloutPriority::medium, "U1-P2",
+ "1234567", "ABCD", "123456789ABC");
+ callouts.addCallout(std::move(c1));
+
+ EXPECT_EQ(callouts.callouts().size(), 2);
+ }
+
+ {
+ // Two duplicates and two unique. Needs sorting.
+ Callouts callouts;
+ auto c0 = std::make_unique<Callout>(CalloutPriority::low, "U1-P9",
+ "1234567", "ABCD", "123456789ABC");
+ callouts.addCallout(std::move(c0));
+
+ auto c1 = std::make_unique<Callout>(CalloutPriority::low, "U1-P1",
+ "1234567", "ABC", "123456789ABC");
+ callouts.addCallout(std::move(c1));
+
+ auto c2 = std::make_unique<Callout>(CalloutPriority::high, "U1-P1",
+ "1234567", "ABC", "123456789ABC");
+ callouts.addCallout(std::move(c2));
+
+ auto c3 = std::make_unique<Callout>(CalloutPriority::medium, "U1-P5",
+ "1234567", "ABCD", "123456789ABC");
+ callouts.addCallout(std::move(c3));
+
+ const auto& calloutObjects = callouts.callouts();
+ EXPECT_EQ(callouts.callouts().size(), 3);
+ EXPECT_EQ(calloutObjects[0]->priority(), 'H');
+ EXPECT_EQ(calloutObjects[0]->locationCode(), "U1-P1");
+ EXPECT_EQ(calloutObjects[1]->priority(), 'M');
+ EXPECT_EQ(calloutObjects[1]->locationCode(), "U1-P5");
+ EXPECT_EQ(calloutObjects[2]->priority(), 'L');
+ EXPECT_EQ(calloutObjects[2]->locationCode(), "U1-P9");
+ }
+}