regulators: Support a string or vector for VPD
Add a 'byte_values' alternative to the 'value' entry in the compare VPD
action. This is to support VPD values that are not strings, such as
'HW', a new IBM keyword that describes the version of a piece of
hardware.
To support this, the VPD class now treats all VPD keyword values as
vectors of uint8_ts, including in its data cache. If a compare VPD
action in the JSON contains a string value, it will be converted to the
vector before the CompareVPDAction class is created.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
Change-Id: I3fcabf896f4885feae1b07ee2c3da5929cf8bfa4
diff --git a/phosphor-regulators/docs/config_file/compare_vpd.md b/phosphor-regulators/docs/config_file/compare_vpd.md
index 1c2b924..79927c1 100644
--- a/phosphor-regulators/docs/config_file/compare_vpd.md
+++ b/phosphor-regulators/docs/config_file/compare_vpd.md
@@ -12,6 +12,7 @@
* Manufacturer
* Model
* PartNumber
+* HW
This action can be used in an [if](if.md) condition to execute actions based on
a VPD keyword value. For example, you could set the output voltage only for
@@ -21,14 +22,18 @@
| Name | Required | Type | Description |
| :--- | :------: | :--- | :---------- |
| fru | yes | string | Field-Replaceable Unit (FRU) that contains the VPD. Specify the relative D-Bus inventory path of the FRU. Full inventory paths begin with the root "/xyz/openbmc_project/inventory". Specify the relative path below the root, such as "system/chassis/disk_backplane". |
-| keyword | yes | string | VPD keyword. Specify one of the following: "CCIN", "Manufacturer", "Model", "PartNumber". |
-| value | yes | string | Expected value. |
+| keyword | yes | string | VPD keyword. Specify one of the following: "CCIN", "Manufacturer", "Model", "PartNumber", "HW". |
+| value | see [notes](#notes) | string | Expected value. |
+| byte\_values | see [notes](#notes) | array of strings | One or more expected byte values expressed in hexadecimal. Each value must be prefixed with 0x and surrounded by double quotes. |
+
+### Notes
+* You must specify either "value" or "byte_values".
## Return Value
Returns true if the keyword value equals the expected value, otherwise returns
false.
-## Example
+## Examples
```
{
"comments": [ "Check if disk backplane has CCIN value 2D35" ],
@@ -39,3 +44,13 @@
}
}
```
+```
+{
+ "comments": [ "Check if disk backplane has CCIN value 0x32, 0x44, 0x33, 0x35" ],
+ "compare_vpd": {
+ "fru": "system/chassis/disk_backplane",
+ "keyword": "CCIN",
+ "byte_values": [ "0x32", "0x44", "0x33", "0x35" ]
+ }
+}
+```
diff --git a/phosphor-regulators/schema/config_schema.json b/phosphor-regulators/schema/config_schema.json
index c6ef268..98c692b 100644
--- a/phosphor-regulators/schema/config_schema.json
+++ b/phosphor-regulators/schema/config_schema.json
@@ -164,16 +164,22 @@
"keyword": {"$ref": "#/definitions/keyword" },
- "value": {"$ref": "#/definitions/string_value" }
+ "value": {"$ref": "#/definitions/string_value" },
+
+ "byte_values": {"$ref": "#/definitions/bytes_values" }
},
- "required": ["fru", "keyword", "value"],
+ "required": ["fru", "keyword"],
+ "oneOf": [
+ {"required": ["value"]},
+ {"required": ["byte_values"]}
+ ],
"additionalProperties": false
},
"keyword":
{
"type": "string",
- "enum": ["CCIN", "Manufacturer", "Model", "PartNumber"]
+ "enum": ["CCIN", "Manufacturer", "Model", "PartNumber", "HW"]
},
"string_value":
diff --git a/phosphor-regulators/src/actions/compare_vpd_action.cpp b/phosphor-regulators/src/actions/compare_vpd_action.cpp
index 1049cee..d7cccf0 100644
--- a/phosphor-regulators/src/actions/compare_vpd_action.cpp
+++ b/phosphor-regulators/src/actions/compare_vpd_action.cpp
@@ -30,7 +30,7 @@
try
{
// Get actual VPD keyword value
- std::string actualValue =
+ std::vector<uint8_t> actualValue =
environment.getServices().getVPD().getValue(fru, keyword);
// Check if actual value equals expected value
@@ -51,7 +51,13 @@
ss << "compare_vpd: { ";
ss << "fru: " << fru << ", ";
ss << "keyword: " << keyword << ", ";
- ss << "value: " << value << " }";
+ ss << "value: [ ";
+ ss << std::hex << std::uppercase;
+ for (unsigned int i = 0; i < value.size(); ++i)
+ {
+ ss << ((i > 0) ? ", " : "") << "0x" << static_cast<uint16_t>(value[i]);
+ }
+ ss << " ] }";
return ss.str();
}
diff --git a/phosphor-regulators/src/actions/compare_vpd_action.hpp b/phosphor-regulators/src/actions/compare_vpd_action.hpp
index af7e085..8825220 100644
--- a/phosphor-regulators/src/actions/compare_vpd_action.hpp
+++ b/phosphor-regulators/src/actions/compare_vpd_action.hpp
@@ -18,7 +18,9 @@
#include "action.hpp"
#include "action_environment.hpp"
+#include <cstdint>
#include <string>
+#include <vector>
namespace phosphor::power::regulators
{
@@ -52,7 +54,7 @@
*/
explicit CompareVPDAction(const std::string& fru,
const std::string& keyword,
- const std::string& value) :
+ const std::vector<uint8_t>& value) :
fru{fru},
keyword{keyword}, value{value}
{
@@ -96,7 +98,7 @@
*
* @return value
*/
- const std::string& getValue() const
+ const std::vector<uint8_t>& getValue() const
{
return value;
}
@@ -124,7 +126,7 @@
/**
* Expected value.
*/
- const std::string value{};
+ const std::vector<uint8_t> value{};
};
} // namespace phosphor::power::regulators
diff --git a/phosphor-regulators/src/config_file_parser.cpp b/phosphor-regulators/src/config_file_parser.cpp
index 5238741..0b4673d 100644
--- a/phosphor-regulators/src/config_file_parser.cpp
+++ b/phosphor-regulators/src/config_file_parser.cpp
@@ -274,10 +274,26 @@
std::string keyword = parseString(keywordElement);
++propertyCount;
- // Required value property
- const json& valueElement = getRequiredProperty(element, "value");
- std::string value = parseString(valueElement);
- ++propertyCount;
+ // Either value or byte_values required property
+ auto valueIt = element.find("value");
+ std::vector<uint8_t> value{};
+ auto byteValuesIt = element.find("byte_values");
+ if ((valueIt != element.end()) && (byteValuesIt == element.end()))
+ {
+ std::string stringValue = parseString(*valueIt);
+ value.insert(value.begin(), stringValue.begin(), stringValue.end());
+ ++propertyCount;
+ }
+ else if ((valueIt == element.end()) && (byteValuesIt != element.end()))
+ {
+ value = parseHexByteArray(*byteValuesIt);
+ ++propertyCount;
+ }
+ else
+ {
+ throw std::invalid_argument{
+ "Invalid property: Must contain either value or byte_values"};
+ }
// Verify no invalid properties exist
verifyPropertyCount(element, propertyCount);
diff --git a/phosphor-regulators/src/vpd.cpp b/phosphor-regulators/src/vpd.cpp
index fc99904..5f5454e 100644
--- a/phosphor-regulators/src/vpd.cpp
+++ b/phosphor-regulators/src/vpd.cpp
@@ -22,10 +22,10 @@
namespace phosphor::power::regulators
{
-std::string DBusVPD::getValue(const std::string& inventoryPath,
- const std::string& keyword)
+std::vector<uint8_t> DBusVPD::getValue(const std::string& inventoryPath,
+ const std::string& keyword)
{
- std::string value{};
+ std::vector<uint8_t> value{};
// Get cached keywords for the inventory path
KeywordMap& cachedKeywords = cache[inventoryPath];
@@ -38,12 +38,28 @@
}
else
{
- // Get keyword value from D-Bus interface/property. The property name
- // is normally the same as the VPD keyword name. However, the CCIN
- // keyword is stored in the Model property.
- std::string property{(keyword == "CCIN") ? "Model" : keyword};
- util::getProperty(ASSET_IFACE, property, inventoryPath,
- INVENTORY_MGR_IFACE, bus, value);
+ if (keyword == "HW")
+ {
+ // HW is a vector<uint8_t>, the others are a string.
+ util::getProperty("com.ibm.ipzvpd.VINI", "HW", inventoryPath,
+ INVENTORY_MGR_IFACE, bus, value);
+ }
+ else
+ {
+ // Get keyword value from D-Bus interface/property. The property
+ // name is normally the same as the VPD keyword name. However, the
+ // CCIN keyword is stored in the Model property.
+ std::string property{(keyword == "CCIN") ? "Model" : keyword};
+ std::string stringValue;
+ util::getProperty(ASSET_IFACE, property, inventoryPath,
+ INVENTORY_MGR_IFACE, bus, stringValue);
+
+ if (!stringValue.empty())
+ {
+ value.insert(value.begin(), stringValue.begin(),
+ stringValue.end());
+ }
+ }
// Cache keyword value
cachedKeywords[keyword] = value;
diff --git a/phosphor-regulators/src/vpd.hpp b/phosphor-regulators/src/vpd.hpp
index 3222d50..8ae1314 100644
--- a/phosphor-regulators/src/vpd.hpp
+++ b/phosphor-regulators/src/vpd.hpp
@@ -17,8 +17,10 @@
#include <sdbusplus/bus.hpp>
+#include <cstdint>
#include <map>
#include <string>
+#include <vector>
namespace phosphor::power::regulators
{
@@ -60,8 +62,8 @@
* @param keyword VPD keyword
* @return VPD keyword value
*/
- virtual std::string getValue(const std::string& inventoryPath,
- const std::string& keyword) = 0;
+ virtual std::vector<uint8_t> getValue(const std::string& inventoryPath,
+ const std::string& keyword) = 0;
};
/**
@@ -96,14 +98,14 @@
}
/** @copydoc VPD::getValue() */
- virtual std::string getValue(const std::string& inventoryPath,
- const std::string& keyword) override;
+ virtual std::vector<uint8_t> getValue(const std::string& inventoryPath,
+ const std::string& keyword) override;
private:
/**
* Type alias for map from keyword names to values.
*/
- using KeywordMap = std::map<std::string, std::string>;
+ using KeywordMap = std::map<std::string, std::vector<uint8_t>>;
/**
* D-Bus bus object.
diff --git a/phosphor-regulators/test/actions/compare_vpd_action_tests.cpp b/phosphor-regulators/test/actions/compare_vpd_action_tests.cpp
index 1cc7782..26361b9 100644
--- a/phosphor-regulators/test/actions/compare_vpd_action_tests.cpp
+++ b/phosphor-regulators/test/actions/compare_vpd_action_tests.cpp
@@ -36,13 +36,14 @@
TEST(CompareVPDActionTests, Constructor)
{
+ std::vector<uint8_t> value{0x32, 0x44, 0x33, 0x35}; // "2D35"
CompareVPDAction action{
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane", "CCIN",
- "2D35"};
+ value};
EXPECT_EQ(action.getFRU(),
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane");
EXPECT_EQ(action.getKeyword(), "CCIN");
- EXPECT_EQ(action.getValue(), "2D35");
+ EXPECT_EQ(action.getValue(), value);
}
TEST(CompareVPDActionTests, Execute)
@@ -51,39 +52,35 @@
{
std::string fru{"/xyz/openbmc_project/inventory/system"};
std::string keyword{"Model"};
+ std::vector<uint8_t> abcdValue{0x41, 0x42, 0x43, 0x44};
// Create MockServices object. VPD service will return "ABCD" as VPD
- // value 4 times.
+ // value 3 times.
MockServices services{};
MockVPD& vpd = services.getMockVPD();
EXPECT_CALL(vpd, getValue(fru, keyword))
- .Times(4)
- .WillRepeatedly(Return("ABCD"));
+ .Times(3)
+ .WillRepeatedly(Return(abcdValue));
IDMap idMap{};
ActionEnvironment environment{idMap, "", services};
// Test where returns true: actual value == expected value
{
- CompareVPDAction action{fru, keyword, "ABCD"};
+ CompareVPDAction action{fru, keyword, abcdValue};
EXPECT_TRUE(action.execute(environment));
}
// Test where returns false: actual value != expected value
{
- CompareVPDAction action{fru, keyword, "BEEF"};
+ CompareVPDAction action{fru, keyword,
+ std::vector<uint8_t>{1, 2, 3, 4}};
EXPECT_FALSE(action.execute(environment));
}
- // Test where returns false: expected value differs by case
+ // Test where returns false: expected value is empty
{
- CompareVPDAction action{fru, keyword, "abcd"};
- EXPECT_FALSE(action.execute(environment));
- }
-
- // Test where returns false: expected value is an empty string
- {
- CompareVPDAction action{fru, keyword, ""};
+ CompareVPDAction action{fru, keyword, std::vector<uint8_t>{}};
EXPECT_FALSE(action.execute(environment));
}
}
@@ -92,6 +89,7 @@
{
std::string fru{"/xyz/openbmc_project/inventory/system"};
std::string keyword{"Model"};
+ std::vector<uint8_t> emptyValue{};
// Create MockServices object. VPD service will return "" as VPD value
// 2 times.
@@ -99,20 +97,21 @@
MockVPD& vpd = services.getMockVPD();
EXPECT_CALL(vpd, getValue(fru, keyword))
.Times(2)
- .WillRepeatedly(Return(""));
+ .WillRepeatedly(Return(emptyValue));
IDMap idMap{};
ActionEnvironment environment{idMap, "", services};
// Test where returns true: actual value == expected value
{
- CompareVPDAction action{fru, keyword, ""};
+ CompareVPDAction action{fru, keyword, emptyValue};
EXPECT_TRUE(action.execute(environment));
}
// Test where returns false: actual value != expected value
{
- CompareVPDAction action{fru, keyword, "ABCD"};
+ CompareVPDAction action{fru, keyword,
+ std::vector<uint8_t>{1, 2, 3}};
EXPECT_FALSE(action.execute(environment));
}
}
@@ -135,15 +134,17 @@
try
{
- CompareVPDAction action{fru, keyword, "ABCD"};
+ CompareVPDAction action{fru, keyword,
+ std::vector<uint8_t>{1, 2, 3}};
action.execute(environment);
ADD_FAILURE() << "Should not have reached this line.";
}
catch (const ActionError& e)
{
- EXPECT_STREQ(e.what(), "ActionError: compare_vpd: { fru: "
- "/xyz/openbmc_project/inventory/system, "
- "keyword: Model, value: ABCD }");
+ EXPECT_STREQ(e.what(),
+ "ActionError: compare_vpd: { fru: "
+ "/xyz/openbmc_project/inventory/system, "
+ "keyword: Model, value: [ 0x1, 0x2, 0x3 ] }");
try
{
// Re-throw inner exception
@@ -170,7 +171,7 @@
{
CompareVPDAction action{
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane", "CCIN",
- "2D35"};
+ std::vector<uint8_t>{1, 2, 3, 4}};
EXPECT_EQ(action.getFRU(),
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane");
}
@@ -179,7 +180,7 @@
{
CompareVPDAction action{
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane", "CCIN",
- "2D35"};
+ std::vector<uint8_t>{1, 2, 3, 4}};
EXPECT_EQ(action.getKeyword(), "CCIN");
}
@@ -187,17 +188,29 @@
{
CompareVPDAction action{
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane", "CCIN",
- "2D35"};
- EXPECT_EQ(action.getValue(), "2D35");
+ std::vector<uint8_t>{1, 2, 3, 4}};
+ EXPECT_EQ(action.getValue(), (std::vector<uint8_t>{0x1, 0x2, 0x3, 0x4}));
}
TEST(CompareVPDActionTests, ToString)
{
- CompareVPDAction action{
- "/xyz/openbmc_project/inventory/system/chassis/disk_backplane", "CCIN",
- "2D35"};
- EXPECT_EQ(action.toString(), "compare_vpd: { fru: "
- "/xyz/openbmc_project/inventory/system/"
- "chassis/disk_backplane, keyword: "
- "CCIN, value: 2D35 }");
+ {
+ CompareVPDAction action{
+ "/xyz/openbmc_project/inventory/system/chassis/disk_backplane",
+ "CCIN", std::vector<uint8_t>{0x01, 0xA3, 0x0, 0xFF}};
+ EXPECT_EQ(action.toString(), "compare_vpd: { fru: "
+ "/xyz/openbmc_project/inventory/system/"
+ "chassis/disk_backplane, keyword: "
+ "CCIN, value: [ 0x1, 0xA3, 0x0, 0xFF ] }");
+ }
+
+ {
+ CompareVPDAction action{
+ "/xyz/openbmc_project/inventory/system/chassis/disk_backplane",
+ "CCIN", std::vector<uint8_t>{}};
+ EXPECT_EQ(action.toString(), "compare_vpd: { fru: "
+ "/xyz/openbmc_project/inventory/system/"
+ "chassis/disk_backplane, keyword: "
+ "CCIN, value: [ ] }");
+ }
}
diff --git a/phosphor-regulators/test/config_file_parser_tests.cpp b/phosphor-regulators/test/config_file_parser_tests.cpp
index c2cf08e..3249465 100644
--- a/phosphor-regulators/test/config_file_parser_tests.cpp
+++ b/phosphor-regulators/test/config_file_parser_tests.cpp
@@ -1085,7 +1085,7 @@
TEST(ConfigFileParserTests, ParseCompareVPD)
{
- // Test where works
+ // Test where works, using "value"
{
const json element = R"(
{
@@ -1099,7 +1099,25 @@
action->getFRU(),
"/xyz/openbmc_project/inventory/system/chassis/disk_backplane");
EXPECT_EQ(action->getKeyword(), "CCIN");
- EXPECT_EQ(action->getValue(), "2D35");
+ EXPECT_EQ(action->getValue(),
+ (std::vector<uint8_t>{0x32, 0x44, 0x33, 0x35}));
+ }
+
+ // Test where works, using "byte_values"
+ {
+ const json element = R"(
+ {
+ "fru": "system/chassis/disk_backplane",
+ "keyword": "CCIN",
+ "byte_values": ["0x11", "0x22", "0x33"]
+ }
+ )"_json;
+ std::unique_ptr<CompareVPDAction> action = parseCompareVPD(element);
+ EXPECT_EQ(
+ action->getFRU(),
+ "/xyz/openbmc_project/inventory/system/chassis/disk_backplane");
+ EXPECT_EQ(action->getKeyword(), "CCIN");
+ EXPECT_EQ(action->getValue(), (std::vector<uint8_t>{0x11, 0x22, 0x33}));
}
// Test where fails: Element is not an object
@@ -1181,7 +1199,28 @@
}
catch (const std::invalid_argument& e)
{
- EXPECT_STREQ(e.what(), "Required property missing: value");
+ EXPECT_STREQ(e.what(), "Invalid property: Must contain "
+ "either value or byte_values");
+ }
+
+ // Test where fails: both value and byte_value specified
+ try
+ {
+ const json element = R"(
+ {
+ "fru": "system/chassis/disk_backplane",
+ "keyword": "CCIN",
+ "value": "2D35",
+ "byte_values": [ "0x01", "0x02" ]
+ }
+ )"_json;
+ parseCompareVPD(element);
+ ADD_FAILURE() << "Should not have reached this line.";
+ }
+ catch (const std::invalid_argument& e)
+ {
+ EXPECT_STREQ(e.what(), "Invalid property: Must contain "
+ "either value or byte_values");
}
// Test where fails: fru value is invalid
@@ -1237,6 +1276,24 @@
{
EXPECT_STREQ(e.what(), "Element is not a string");
}
+
+ // Test where fails: byte_values is wrong format
+ try
+ {
+ const json element = R"(
+ {
+ "fru": "system/chassis/disk_backplane",
+ "keyword": "CCIN",
+ "byte_values": [1, 2, 3]
+ }
+ )"_json;
+ parseCompareVPD(element);
+ ADD_FAILURE() << "Should not have reached this line.";
+ }
+ catch (const std::invalid_argument& e)
+ {
+ EXPECT_STREQ(e.what(), "Element is not a string");
+ }
}
TEST(ConfigFileParserTests, ParseConfiguration)
diff --git a/phosphor-regulators/test/mock_vpd.hpp b/phosphor-regulators/test/mock_vpd.hpp
index 9b0d494..7c0779f 100644
--- a/phosphor-regulators/test/mock_vpd.hpp
+++ b/phosphor-regulators/test/mock_vpd.hpp
@@ -40,7 +40,7 @@
MOCK_METHOD(void, clearCache, (), (override));
- MOCK_METHOD(std::string, getValue,
+ MOCK_METHOD(std::vector<uint8_t>, getValue,
(const std::string& inventoryPath, const std::string& keyword),
(override));
};
diff --git a/phosphor-regulators/test/validate-regulators-config_tests.cpp b/phosphor-regulators/test/validate-regulators-config_tests.cpp
index e6b84ec..92aff99 100644
--- a/phosphor-regulators/test/validate-regulators-config_tests.cpp
+++ b/phosphor-regulators/test/validate-regulators-config_tests.cpp
@@ -652,6 +652,14 @@
json configFile = compareVpdFile;
EXPECT_JSON_VALID(configFile);
}
+ // Valid, using byte_values.
+ {
+ json configFile = compareVpdFile;
+ configFile["rules"][0]["actions"][1]["compare_vpd"].erase("value");
+ configFile["rules"][0]["actions"][1]["compare_vpd"]["byte_values"] = {
+ "0x01", "0x02"};
+ EXPECT_JSON_VALID(configFile);
+ }
// Invalid: no FRU property.
{
@@ -694,14 +702,14 @@
}
// Invalid: property keyword is not "CCIN", "Manufacturer", "Model",
- // "PartNumber"
+ // "PartNumber", "HW"
{
json configFile = compareVpdFile;
configFile["rules"][0]["actions"][1]["compare_vpd"]["keyword"] =
"Number";
EXPECT_JSON_INVALID(configFile, "Validation failed.",
"'Number' is not one of ['CCIN', "
- "'Manufacturer', 'Model', 'PartNumber']");
+ "'Manufacturer', 'Model', 'PartNumber', 'HW']");
}
// Invalid: property value wrong type.
@@ -711,6 +719,39 @@
EXPECT_JSON_INVALID(configFile, "Validation failed.",
"1 is not of type 'string'");
}
+
+ // Invalid: property byte_values has wrong type
+ {
+ json configFile = compareVpdFile;
+ configFile["rules"][0]["actions"][1]["compare_vpd"].erase("value");
+ configFile["rules"][0]["actions"][1]["compare_vpd"]["byte_values"] =
+ "0x50";
+ EXPECT_JSON_INVALID(configFile, "Validation failed.",
+ "'0x50' is not of type 'array'");
+ }
+
+ // Invalid: property byte_values is empty
+ {
+ json configFile = compareVpdFile;
+ configFile["rules"][0]["actions"][1]["compare_vpd"].erase("value");
+ configFile["rules"][0]["actions"][1]["compare_vpd"]["byte_values"] =
+ json::array();
+ EXPECT_JSON_INVALID(configFile, "Validation failed.",
+ "[] is too short");
+ }
+
+ // Invalid: properties byte_values and value both exist
+ {
+ json configFile = compareVpdFile;
+ configFile["rules"][0]["actions"][1]["compare_vpd"]["byte_values"] = {
+ "0x01", "0x02"};
+ EXPECT_JSON_INVALID(
+ configFile, "Validation failed.",
+ "{'byte_values': ['0x01', '0x02'], 'fru': "
+ "'system/chassis/motherboard/regulator2', 'keyword': 'CCIN', "
+ "'value': '2D35'} is valid under each of {'required': "
+ "['byte_values']}, {'required': ['value']}");
+ }
}
TEST(ValidateRegulatorsConfigTest, ConfigFile)
{