Add length limit for user-defined values
Previously, the only limit for most user-defined values like id or name
was the one enforced by dbus, which was fairly big. In order to save
space used by persistent data, new meson options were added, with length
limit for those fields.
This directly impacts following dbus interfaces:
- TriggerManager.AddTrigger:
- Id
- Name
- Reports
- Thresholds (only name of discrete threshold)
- Trigger.Name
- Trigger.Reports
- Trigger.Thresholds (only name of discrete threshold)
- ReportManager.AddReport(FutureVersion):
- Id
- Name
- MetricParameters (metricId)
- Report.Name
- Report.ReadingParameters (metricId)
For Id fields we support 'prefixes', which also are limited, but those
limit are separate. So if limit for prefix is set to 5 and limit for
id/name is set to 5, following Ids are fine:
- 12345/12345
- 12345
and following are not:
- 123456/1234
- 1/123456
Testing done:
- UTs are passing.
- new limits are reflected when calling mentioned dbus interfaces.
Signed-off-by: Szymon Dompke <szymon.dompke@intel.com>
Change-Id: I29291a1cc56a344d92fb65491c9186fdb90a8529
diff --git a/meson.build b/meson.build
index e66248e..9033b37 100644
--- a/meson.build
+++ b/meson.build
@@ -64,6 +64,8 @@
get_option('max-dbus-path-length').to_string(),
'-DTELEMETRY_MAX_APPEND_LIMIT=' +
get_option('max-append-limit').to_string(),
+ '-DTELEMETRY_MAX_ID_NAME_LENGTH=' + get_option('max-id-name-length').to_string(),
+ '-DTELEMETRY_MAX_PREFIX_LENGTH=' + get_option('max-prefix-length').to_string(),
language: 'cpp'
)
@@ -91,7 +93,7 @@
'src/types/report_types.cpp',
'src/utils/conversion_trigger.cpp',
'src/utils/dbus_path_utils.cpp',
- 'src/utils/generate_id.cpp',
+ 'src/utils/make_id_name.cpp',
'src/utils/messanger_service.cpp',
],
dependencies: [
diff --git a/meson_options.txt b/meson_options.txt
index 2f26922..e033f4f 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -7,7 +7,11 @@
description: 'Minimal value of interval in milliseconds')
option('max-triggers', type: 'integer', min: 0, value: 10,
description: 'Max number of Triggers')
-option('max-dbus-path-length', type: 'integer', min: 1, value: 4095,
+option('max-dbus-path-length', type: 'integer', min: 256, value: 4095,
description: 'Max length of dbus object path')
option('max-append-limit', type: 'integer', min: 0, value: 256,
description: 'Max AppendLimit value')
+option('max-id-name-length', type: 'integer', min: 32, value: 256,
+ description: 'Max length of any "id" or "name" type field.')
+option('max-prefix-length', type: 'integer', min: 32, value: 256,
+ description: 'Max length of dbus prefix for any object.')
diff --git a/src/interfaces/report_factory.hpp b/src/interfaces/report_factory.hpp
index d8e076a..9448598 100644
--- a/src/interfaces/report_factory.hpp
+++ b/src/interfaces/report_factory.hpp
@@ -30,10 +30,10 @@
virtual std::vector<LabeledMetricParameters>
convertMetricParams(const ReadingParameters& metricParams) const = 0;
- virtual void
- updateMetrics(std::vector<std::shared_ptr<interfaces::Metric>>& metrics,
- bool enabled,
- const ReadingParameters& metricParams) const = 0;
+ virtual void updateMetrics(
+ std::vector<std::shared_ptr<interfaces::Metric>>& metrics, bool enabled,
+ const std::vector<LabeledMetricParameters>& labeledMetricParams)
+ const = 0;
virtual std::unique_ptr<interfaces::Report>
make(const std::string& id, const std::string& name,
diff --git a/src/report.cpp b/src/report.cpp
index cf2b389..9ee60a4 100644
--- a/src/report.cpp
+++ b/src/report.cpp
@@ -331,8 +331,12 @@
"ReadingParametersFutureVersion", readingParameters,
sdbusplus::vtable::property_::emits_change,
[this, &reportFactory](auto newVal, auto& oldVal) {
- reportFactory.updateMetrics(
- metrics, state.get<ReportFlags::enabled>(), newVal);
+ auto labeledMetricParams =
+ reportFactory.convertMetricParams(newVal);
+ ReportManager::verifyMetricParameters(labeledMetricParams);
+ reportFactory.updateMetrics(metrics,
+ state.get<ReportFlags::enabled>(),
+ labeledMetricParams);
readingParameters = toReadingParameters(
utils::transform(metrics, [](const auto& metric) {
return metric->dumpConfiguration();
diff --git a/src/report_factory.cpp b/src/report_factory.cpp
index 1c530f3..cb096cf 100644
--- a/src/report_factory.cpp
+++ b/src/report_factory.cpp
@@ -49,9 +49,8 @@
void ReportFactory::updateMetrics(
std::vector<std::shared_ptr<interfaces::Metric>>& metrics, bool enabled,
- const ReadingParameters& metricParams) const
+ const std::vector<LabeledMetricParameters>& labeledMetricParams) const
{
- auto labeledMetricParams = convertMetricParams(metricParams);
std::vector<std::shared_ptr<interfaces::Metric>> oldMetrics = metrics;
std::vector<std::shared_ptr<interfaces::Metric>> newMetrics;
diff --git a/src/report_factory.hpp b/src/report_factory.hpp
index e8729b1..f49be6d 100644
--- a/src/report_factory.hpp
+++ b/src/report_factory.hpp
@@ -24,10 +24,10 @@
std::vector<LabeledMetricParameters> convertMetricParams(
const ReadingParameters& metricParams) const override;
- void
- updateMetrics(std::vector<std::shared_ptr<interfaces::Metric>>& metrics,
- bool enabled,
- const ReadingParameters& metricParams) const override;
+ void updateMetrics(
+ std::vector<std::shared_ptr<interfaces::Metric>>& metrics, bool enabled,
+ const std::vector<LabeledMetricParameters>& labeledMetricParams)
+ const override;
std::unique_ptr<interfaces::Report>
make(const std::string& reportId, const std::string& name,
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index 738dd6d..1677bb3 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -3,7 +3,8 @@
#include "report.hpp"
#include "types/report_types.hpp"
#include "utils/conversion.hpp"
-#include "utils/generate_id.hpp"
+#include "utils/dbus_path_utils.hpp"
+#include "utils/make_id_name.hpp"
#include "utils/transform.hpp"
#include <phosphor-logging/log.hpp>
@@ -46,10 +47,6 @@
"MaxReports", size_t{}, sdbusplus::vtable::property_::const_,
[](const auto&) { return maxReports; });
dbusIface.register_property_r(
- "MaxReportIdLength", size_t{},
- sdbusplus::vtable::property_::const_,
- [](const auto&) { return maxReportIdLength; });
- dbusIface.register_property_r(
"MinInterval", uint64_t{}, sdbusplus::vtable::property_::const_,
[](const auto&) -> uint64_t { return minInterval.count(); });
dbusIface.register_property_r(
@@ -133,12 +130,31 @@
reports.end());
}
+void ReportManager::verifyMetricParameters(
+ const std::vector<LabeledMetricParameters>& readingParams)
+{
+ namespace ts = utils::tstring;
+
+ for (auto readingParam : readingParams)
+ {
+ if (readingParam.at_label<ts::Id>().length() >
+ utils::constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "MetricId too long");
+ }
+ }
+}
+
void ReportManager::verifyAddReport(
const std::string& reportId, const std::string& reportName,
const ReportingType reportingType, Milliseconds interval,
const ReportUpdates reportUpdates, const uint64_t appendLimit,
const std::vector<LabeledMetricParameters>& readingParams)
{
+ namespace ts = utils::tstring;
+
if (reports.size() >= maxReports)
{
throw sdbusplus::exception::SdBusError(
@@ -178,10 +194,10 @@
"Too many reading parameters");
}
+ verifyMetricParameters(readingParams);
+
try
{
- namespace ts = utils::tstring;
-
for (const LabeledMetricParameters& item : readingParams)
{
utils::toOperationType(
@@ -221,8 +237,8 @@
const auto existingReportIds = utils::transform(
reports, [](const auto& report) { return report->getId(); });
- auto [id, name] = utils::generateId(reportId, reportName, reportNameDefault,
- existingReportIds, maxReportIdLength);
+ auto [id, name] = utils::makeIdName(reportId, reportName, reportNameDefault,
+ existingReportIds);
verifyAddReport(id, name, reportingType, interval, reportUpdates,
appendLimit, labeledMetricParams);
diff --git a/src/report_manager.hpp b/src/report_manager.hpp
index 88dc3e0..4882419 100644
--- a/src/report_manager.hpp
+++ b/src/report_manager.hpp
@@ -33,6 +33,9 @@
void removeReport(const interfaces::Report* report) override;
+ static void verifyMetricParameters(
+ const std::vector<LabeledMetricParameters>& readingParams);
+
private:
std::unique_ptr<interfaces::ReportFactory> reportFactory;
std::unique_ptr<interfaces::JsonStorage> reportStorage;
@@ -64,14 +67,14 @@
public:
static constexpr size_t maxReports{TELEMETRY_MAX_REPORTS};
static constexpr size_t maxNumberMetrics{TELEMETRY_MAX_READING_PARAMS};
- static constexpr size_t maxReportIdLength{
- TELEMETRY_MAX_DBUS_PATH_LENGTH -
- utils::constants::reportDirStr.length()};
static constexpr Milliseconds minInterval{TELEMETRY_MIN_INTERVAL};
static constexpr size_t maxAppendLimit{TELEMETRY_MAX_APPEND_LIMIT};
static constexpr const char* reportManagerIfaceName =
"xyz.openbmc_project.Telemetry.ReportManager";
static constexpr const char* reportManagerPath =
"/xyz/openbmc_project/Telemetry/Reports";
- static constexpr const char* reportNameDefault = "Report";
+ static constexpr std::string_view reportNameDefault = "Report";
+
+ static_assert(!reportNameDefault.empty(),
+ "Default report name cannot be empty.");
};
diff --git a/src/trigger.cpp b/src/trigger.cpp
index c0935c3..c11ea42 100644
--- a/src/trigger.cpp
+++ b/src/trigger.cpp
@@ -75,6 +75,7 @@
[this, &triggerFactory](auto newVal, auto& oldVal) {
auto newThresholdParams = std::visit(
utils::ToLabeledThresholdParamConversion(), newVal);
+ TriggerManager::verifyThresholdParams(newThresholdParams);
triggerFactory.updateThresholds(
thresholds, *id, triggerActions, reportIds, sensors,
newThresholdParams);
@@ -134,6 +135,12 @@
dbusIface.register_property_rw(
"Name", name, sdbusplus::vtable::property_::emits_change,
[this](auto newVal, auto& oldVal) {
+ if (newVal.length() > utils::constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Name is too long");
+ }
name = oldVal = newVal;
return 1;
},
diff --git a/src/trigger_manager.cpp b/src/trigger_manager.cpp
index df8dea9..e337bca 100644
--- a/src/trigger_manager.cpp
+++ b/src/trigger_manager.cpp
@@ -4,8 +4,9 @@
#include "types/trigger_types.hpp"
#include "utils/conversion_trigger.hpp"
#include "utils/dbus_path_utils.hpp"
-#include "utils/generate_id.hpp"
+#include "utils/make_id_name.hpp"
#include "utils/transform.hpp"
+#include "utils/tstring.hpp"
#include <phosphor-logging/log.hpp>
@@ -72,9 +73,32 @@
}
}
+void TriggerManager::verifyThresholdParams(
+ const LabeledTriggerThresholdParams& thresholdParams)
+{
+ namespace ts = utils::tstring;
+
+ if (auto discreteParams =
+ std::get_if<std::vector<discrete::LabeledThresholdParam>>(
+ &thresholdParams);
+ discreteParams != nullptr)
+ {
+ for (auto discreteParam : *discreteParams)
+ {
+ if (discreteParam.at_label<ts::UserId>().length() >
+ utils::constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "UserId too long");
+ }
+ }
+ }
+}
+
void TriggerManager::verifyAddTrigger(
- const std::string& triggerId, const std::string& triggerName,
- const std::vector<std::string>& newReportIds) const
+ const std::vector<std::string>& reportIds,
+ const LabeledTriggerThresholdParams& thresholdParams) const
{
if (triggers.size() >= maxTriggers)
{
@@ -83,18 +107,8 @@
"Reached maximal trigger count");
}
- utils::verifyIdCharacters(triggerId);
-
- for (const auto& trigger : triggers)
- {
- if (trigger->getId() == triggerId)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::file_exists), "Duplicate trigger");
- }
- }
-
- verifyReportIds(newReportIds);
+ verifyReportIds(reportIds);
+ verifyThresholdParams(thresholdParams);
}
interfaces::Trigger& TriggerManager::addTrigger(
@@ -107,11 +121,10 @@
const auto existingTriggerIds = utils::transform(
triggers, [](const auto& trigger) { return trigger->getId(); });
- auto [id, name] =
- utils::generateId(triggerIdIn, triggerNameIn, triggerNameDefault,
- existingTriggerIds, maxTriggerIdLength);
+ auto [id, name] = utils::makeIdName(triggerIdIn, triggerNameIn,
+ triggerNameDefault, existingTriggerIds);
- verifyAddTrigger(id, name, reportIds);
+ verifyAddTrigger(reportIds, labeledThresholdParams);
triggers.emplace_back(triggerFactory->make(
id, name, triggerActions, reportIds, *this, *triggerStorage,
diff --git a/src/trigger_manager.hpp b/src/trigger_manager.hpp
index 4db6941..65ee2af 100644
--- a/src/trigger_manager.hpp
+++ b/src/trigger_manager.hpp
@@ -27,6 +27,8 @@
void removeTrigger(const interfaces::Trigger* trigger) override;
static void verifyReportIds(const std::vector<std::string>& newReportIds);
+ static void verifyThresholdParams(
+ const LabeledTriggerThresholdParams& thresholdParams);
private:
std::unique_ptr<interfaces::TriggerFactory> triggerFactory;
@@ -34,9 +36,9 @@
std::unique_ptr<sdbusplus::asio::dbus_interface> managerIface;
std::vector<std::unique_ptr<interfaces::Trigger>> triggers;
- void verifyAddTrigger(const std::string& triggerId,
- const std::string& triggerName,
- const std::vector<std::string>& newReportIds) const;
+ void verifyAddTrigger(
+ const std::vector<std::string>& reportIds,
+ const LabeledTriggerThresholdParams& thresholdParams) const;
interfaces::Trigger&
addTrigger(const std::string& triggerId, const std::string& triggerName,
@@ -48,12 +50,12 @@
public:
static constexpr size_t maxTriggers{TELEMETRY_MAX_TRIGGERS};
- static constexpr size_t maxTriggerIdLength{
- TELEMETRY_MAX_DBUS_PATH_LENGTH -
- utils::constants::triggerDirStr.length()};
static constexpr const char* triggerManagerIfaceName =
"xyz.openbmc_project.Telemetry.TriggerManager";
static constexpr const char* triggerManagerPath =
"/xyz/openbmc_project/Telemetry/Triggers";
- static constexpr const char* triggerNameDefault = "Trigger";
+ static constexpr std::string_view triggerNameDefault = "Trigger";
+
+ static_assert(!triggerNameDefault.empty(),
+ "Default trigger name cannot be empty.");
};
diff --git a/src/utils/dbus_path_utils.cpp b/src/utils/dbus_path_utils.cpp
index 9d37cfc..5c1fddb 100644
--- a/src/utils/dbus_path_utils.cpp
+++ b/src/utils/dbus_path_utils.cpp
@@ -35,16 +35,48 @@
if (path.str.starts_with(constants::reportDirStr))
{
auto id = path.str.substr(constants::reportDirStr.length());
- if (std::cmp_greater(std::count(id.begin(), id.end(), '/'),
- constants::maxPrefixesInId))
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Too many prefixes in id");
- }
+ verifyIdPrefixes(id);
return id;
}
throw sdbusplus::exception::SdBusError(
static_cast<int>(std::errc::invalid_argument), "Invalid path prefix");
}
+
+void verifyIdPrefixes(std::string_view id)
+{
+ size_t pos_start = 0;
+ size_t pos_end = 0;
+ size_t prefix_cnt = 0;
+ while ((pos_end = id.find('/', pos_start)) != std::string::npos)
+ {
+ if (pos_start == pos_end)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Invalid prefixes in id");
+ }
+
+ if (++prefix_cnt > constants::maxPrefixesInId)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Too many prefixes");
+ }
+
+ if (pos_end - pos_start > constants::maxPrefixLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Prefix too long");
+ }
+
+ pos_start = pos_end + 1;
+ }
+
+ if (id.length() - pos_start > constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument), "Id too long");
+ }
+}
} // namespace utils
diff --git a/src/utils/dbus_path_utils.hpp b/src/utils/dbus_path_utils.hpp
index a1b5709..3798d98 100644
--- a/src/utils/dbus_path_utils.hpp
+++ b/src/utils/dbus_path_utils.hpp
@@ -19,6 +19,21 @@
constexpr std::string_view allowedCharactersInPath =
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_/";
constexpr size_t maxPrefixesInId = 1;
+constexpr size_t maxPrefixLength{TELEMETRY_MAX_PREFIX_LENGTH};
+constexpr size_t maxIdNameLength{TELEMETRY_MAX_ID_NAME_LENGTH};
+constexpr size_t maxDbusPathLength{TELEMETRY_MAX_DBUS_PATH_LENGTH};
+
+constexpr size_t maxTriggeFullIdLength{maxDbusPathLength -
+ triggerDirStr.length()};
+constexpr size_t maxReportFullIdLength{maxDbusPathLength -
+ reportDirStr.length()};
+
+static_assert(maxPrefixesInId * (maxPrefixLength + 1) + maxIdNameLength <=
+ maxTriggeFullIdLength,
+ "Misconfigured prefix/id/name lengths.");
+static_assert(maxPrefixesInId * (maxPrefixLength + 1) + maxIdNameLength <=
+ maxReportFullIdLength,
+ "Misconfigured prefix/id/name lengths.");
const sdbusplus::message::object_path triggerDirPath =
sdbusplus::message::object_path(std::string(triggerDirStr));
@@ -33,9 +48,22 @@
!path.ends_with('/');
}
+inline void verifyIdCharacters(std::string_view id)
+{
+ if (id.find_first_not_of(utils::constants::allowedCharactersInPath) !=
+ std::string::npos)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Invalid character in id");
+ }
+}
+
sdbusplus::message::object_path pathAppend(sdbusplus::message::object_path path,
const std::string& appended);
std::string reportPathToId(const sdbusplus::message::object_path& path);
+void verifyIdPrefixes(std::string_view id);
+
} // namespace utils
diff --git a/src/utils/generate_id.cpp b/src/utils/generate_id.cpp
deleted file mode 100644
index 42c9155..0000000
--- a/src/utils/generate_id.cpp
+++ /dev/null
@@ -1,135 +0,0 @@
-#include "utils/generate_id.hpp"
-
-#include "utils/dbus_path_utils.hpp"
-
-#include <sdbusplus/exception.hpp>
-
-#include <algorithm>
-#include <system_error>
-
-namespace utils
-{
-namespace details
-{
-
-size_t countDigits(size_t value)
-{
- size_t result = 1;
- while (value >= 10)
- {
- ++result;
- value /= 10;
- }
- return result;
-}
-
-std::string generateId(std::string_view id, std::string_view name,
- const std::vector<std::string>& conflictIds,
- size_t maxLength)
-{
- verifyIdCharacters(id);
-
- if (id.starts_with('/'))
- {
- id = id.substr(1);
- }
-
- if ((id.length() > maxLength) ||
- (id.ends_with('/') && id.length() >= maxLength))
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument), "Id too long");
- }
-
- if (!id.empty() && !id.ends_with('/'))
- {
- if (std::find(conflictIds.begin(), conflictIds.end(), id) !=
- conflictIds.end())
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::file_exists), "Duplicated id");
- }
- return std::string(id);
- }
-
- std::string strippedId(name);
- strippedId.erase(
- std::remove_if(
- strippedId.begin(), strippedId.end(),
- [](char c) {
- return c == '/' ||
- utils::constants::allowedCharactersInPath.find(c) ==
- std::string_view::npos;
- }),
- strippedId.end());
- strippedId = std::string(id) + strippedId;
-
- size_t idx = 0;
- std::string tmpId = strippedId.substr(0, maxLength);
-
- while (std::find(conflictIds.begin(), conflictIds.end(), tmpId) !=
- conflictIds.end() ||
- tmpId.empty())
- {
- size_t digitsInIdx = countDigits(idx);
-
- if (digitsInIdx > maxLength)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::file_exists),
- "Unique indices are depleted");
- }
-
- tmpId =
- strippedId.substr(0, maxLength - digitsInIdx) + std::to_string(idx);
- ++idx;
- }
-
- return tmpId;
-}
-
-} // namespace details
-
-void verifyIdCharacters(std::string_view id)
-{
- if (id.find_first_not_of(utils::constants::allowedCharactersInPath) !=
- std::string::npos)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid character in id");
- }
-
- if (auto pos = id.find_first_of("/");
- pos != std::string::npos && pos != id.find_last_of("/"))
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Too many '/' in id");
- }
-}
-
-std::pair<std::string, std::string> generateId(
- std::string_view id, std::string_view name, std::string_view defaultName,
- const std::vector<std::string>& conflictIds, const size_t maxLength)
-{
- if (name.empty() && !id.ends_with('/'))
- {
- name = id;
-
- if (auto pos = name.find_last_of("/"); pos != std::string::npos)
- {
- name = name.substr(pos + 1);
- }
- }
-
- if (name.empty())
- {
- name = defaultName;
- }
-
- return std::make_pair(details::generateId(id, name, conflictIds, maxLength),
- std::string{name});
-}
-
-} // namespace utils
diff --git a/src/utils/generate_id.hpp b/src/utils/generate_id.hpp
deleted file mode 100644
index 94952dc..0000000
--- a/src/utils/generate_id.hpp
+++ /dev/null
@@ -1,15 +0,0 @@
-#pragma once
-
-#include <string>
-#include <string_view>
-#include <vector>
-
-namespace utils
-{
-
-void verifyIdCharacters(std::string_view id);
-std::pair<std::string, std::string> generateId(
- std::string_view id, std::string_view name, std::string_view defaultName,
- const std::vector<std::string>& conflictIds, const size_t maxLength);
-
-} // namespace utils
diff --git a/src/utils/make_id_name.cpp b/src/utils/make_id_name.cpp
new file mode 100644
index 0000000..0df6c17
--- /dev/null
+++ b/src/utils/make_id_name.cpp
@@ -0,0 +1,122 @@
+#include "utils/make_id_name.hpp"
+
+#include "utils/dbus_path_utils.hpp"
+
+#include <sdbusplus/exception.hpp>
+
+#include <algorithm>
+#include <system_error>
+
+namespace utils
+{
+namespace details
+{
+
+size_t countDigits(size_t value)
+{
+ size_t result = 1;
+ while (value >= 10)
+ {
+ ++result;
+ value /= 10;
+ }
+ return result;
+}
+
+std::string generateId(std::string_view idIn, std::string_view nameIn,
+ std::string_view defaultName,
+ const std::vector<std::string>& conflictIds)
+{
+ verifyIdCharacters(idIn);
+ verifyIdPrefixes(idIn);
+
+ if (!idIn.empty() && !idIn.ends_with('/'))
+ {
+ if (std::find(conflictIds.begin(), conflictIds.end(), idIn) !=
+ conflictIds.end())
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::file_exists), "Duplicated id");
+ }
+ return std::string(idIn);
+ }
+
+ const std::string prefixes(idIn);
+
+ std::string strippedId(nameIn);
+ if (strippedId.find_first_of(utils::constants::allowedCharactersInPath) ==
+ std::string::npos)
+ {
+ strippedId = defaultName;
+ }
+ strippedId.erase(
+ std::remove_if(
+ strippedId.begin(), strippedId.end(),
+ [](char c) {
+ return c == '/' ||
+ utils::constants::allowedCharactersInPath.find(c) ==
+ std::string_view::npos;
+ }),
+ strippedId.end());
+
+ size_t idx = 0;
+ std::string tmpId =
+ prefixes + strippedId.substr(0, constants::maxIdNameLength);
+
+ while (std::find(conflictIds.begin(), conflictIds.end(), tmpId) !=
+ conflictIds.end())
+ {
+ size_t digitsInIdx = countDigits(idx);
+
+ if (digitsInIdx > constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::file_exists),
+ "Unique indices are depleted");
+ }
+
+ tmpId = prefixes +
+ strippedId.substr(0, constants::maxIdNameLength - digitsInIdx) +
+ std::to_string(idx);
+ ++idx;
+ }
+
+ return tmpId;
+}
+
+} // namespace details
+
+std::pair<std::string, std::string>
+ makeIdName(std::string_view id, std::string_view name,
+ std::string_view defaultName,
+ const std::vector<std::string>& conflictIds)
+{
+ if (name.length() > constants::maxIdNameLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument), "Name too long");
+ }
+
+ if (name.empty() && !id.ends_with('/'))
+ {
+ name = id;
+
+ if (auto pos = name.find_last_of("/"); pos != std::string::npos)
+ {
+ name = name.substr(pos + 1);
+ }
+
+ name = name.substr(0, constants::maxIdNameLength);
+ }
+
+ if (name.empty())
+ {
+ name = defaultName;
+ }
+
+ return std::make_pair(
+ details::generateId(id, name, defaultName, conflictIds),
+ std::string{name});
+}
+
+} // namespace utils
diff --git a/src/utils/make_id_name.hpp b/src/utils/make_id_name.hpp
new file mode 100644
index 0000000..2c4ce75
--- /dev/null
+++ b/src/utils/make_id_name.hpp
@@ -0,0 +1,15 @@
+#pragma once
+
+#include <string>
+#include <string_view>
+#include <vector>
+
+namespace utils
+{
+
+std::pair<std::string, std::string>
+ makeIdName(std::string_view id, std::string_view name,
+ std::string_view defaultName,
+ const std::vector<std::string>& conflictIds);
+
+} // namespace utils
diff --git a/tests/meson.build b/tests/meson.build
index 25d5085..ce86dd3 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -31,7 +31,7 @@
'../src/types/report_types.cpp',
'../src/utils/conversion_trigger.cpp',
'../src/utils/dbus_path_utils.cpp',
- '../src/utils/generate_id.cpp',
+ '../src/utils/make_id_name.cpp',
'../src/utils/messanger_service.cpp',
'src/dbus_environment.cpp',
'src/main.cpp',
@@ -40,7 +40,7 @@
'src/test_detached_timer.cpp',
'src/test_discrete_threshold.cpp',
'src/test_ensure.cpp',
- 'src/test_generate_id.cpp',
+ 'src/test_make_id_name.cpp',
'src/test_metric.cpp',
'src/test_numeric_threshold.cpp',
'src/test_on_change_threshold.cpp',
@@ -56,6 +56,7 @@
'src/test_trigger_manager.cpp',
'src/test_unique_call.cpp',
'src/utils/generate_unique_mock_id.cpp',
+ 'src/utils/string_utils.cpp',
],
dependencies: [
boost,
diff --git a/tests/src/mocks/report_factory_mock.hpp b/tests/src/mocks/report_factory_mock.hpp
index b14bbb7..a043c09 100644
--- a/tests/src/mocks/report_factory_mock.hpp
+++ b/tests/src/mocks/report_factory_mock.hpp
@@ -56,7 +56,7 @@
MOCK_METHOD(void, updateMetrics,
(std::vector<std::shared_ptr<interfaces::Metric>> & metrics,
- bool enabled, const ReadingParameters&),
+ bool enabled, const std::vector<LabeledMetricParameters>&),
(const, override));
MOCK_METHOD(std::unique_ptr<interfaces::Report>, make,
diff --git a/tests/src/params/report_params.hpp b/tests/src/params/report_params.hpp
index 3d32182..458e11b 100644
--- a/tests/src/params/report_params.hpp
+++ b/tests/src/params/report_params.hpp
@@ -9,9 +9,9 @@
class ReportParams final
{
public:
- ReportParams& reportId(std::string val)
+ ReportParams& reportId(std::string_view val)
{
- reportIdProperty = std::move(val);
+ reportIdProperty = val;
return *this;
}
@@ -20,9 +20,9 @@
return reportIdProperty;
}
- ReportParams& reportName(std::string val)
+ ReportParams& reportName(std::string_view val)
{
- reportNameProperty = std::move(val);
+ reportNameProperty = val;
return *this;
}
diff --git a/tests/src/params/trigger_params.hpp b/tests/src/params/trigger_params.hpp
index 65f423e..9f633b8 100644
--- a/tests/src/params/trigger_params.hpp
+++ b/tests/src/params/trigger_params.hpp
@@ -24,9 +24,9 @@
});
}
- TriggerParams& id(std::string val)
+ TriggerParams& id(std::string_view val)
{
- idProperty = std::move(val);
+ idProperty = val;
return *this;
}
@@ -35,9 +35,9 @@
return idProperty;
}
- TriggerParams& name(std::string val)
+ TriggerParams& name(std::string_view val)
{
- nameProperty = std::move(val);
+ nameProperty = val;
return *this;
}
diff --git a/tests/src/test_generate_id.cpp b/tests/src/test_generate_id.cpp
deleted file mode 100644
index dc534dd..0000000
--- a/tests/src/test_generate_id.cpp
+++ /dev/null
@@ -1,172 +0,0 @@
-#include "helpers.hpp"
-#include "utils/generate_id.hpp"
-
-#include <sdbusplus/exception.hpp>
-
-#include <gmock/gmock.h>
-
-using namespace testing;
-using namespace std::literals::string_literals;
-
-class ScenarioBase : public Test
-{
- public:
- std::string_view defaultName = "defName";
- size_t maxSize = 32;
- std::vector<std::string> conflicts;
-};
-
-class ScenarioNameProvided : public ScenarioBase
-{
- public:
- auto generateId(std::string_view id, std::string_view name) const
- {
- return utils::generateId(id, name, defaultName, conflicts, maxSize);
- }
-};
-
-class TestGenerateIdNameProvided : public ScenarioNameProvided
-{};
-
-TEST_F(TestGenerateIdNameProvided, usesIdWhenProvided)
-{
- const std::string name = "name";
-
- EXPECT_THAT(this->generateId("id0", name), Eq(std::pair{"id0"s, name}));
- EXPECT_THAT(this->generateId("/id1", name), Eq(std::pair{"id1"s, name}));
- EXPECT_THAT(this->generateId("prefix/id2", name),
- Eq(std::pair{"prefix/id2"s, name}));
-}
-
-TEST_F(TestGenerateIdNameProvided, usedDefaultWhenNothingProvided)
-{
- this->defaultName = "def";
-
- EXPECT_THAT(this->generateId("", ""), Eq(std::pair{"def"s, "def"s}));
- EXPECT_THAT(this->generateId("/", ""), Eq(std::pair{"def"s, "def"s}));
- EXPECT_THAT(this->generateId("abc/", ""),
- Eq(std::pair{"abc/def"s, "def"s}));
-}
-
-class ScenarioNameNotProvided : public ScenarioBase
-{
- public:
- auto generateId(std::string_view id, std::string_view name) const
- {
- return utils::generateId(id, "", name, conflicts, maxSize);
- }
-};
-
-class TestGenerateIdNameNotProvided : public ScenarioNameNotProvided
-{};
-
-TEST_F(TestGenerateIdNameNotProvided, usesIdAsNameWhenProvided)
-{
- EXPECT_THAT(this->generateId("id0", defaultName),
- Eq(std::pair{"id0"s, "id0"s}));
- EXPECT_THAT(this->generateId("/id1", defaultName),
- Eq(std::pair{"id1"s, "id1"s}));
- EXPECT_THAT(this->generateId("prefix/id2", defaultName),
- Eq(std::pair{"prefix/id2"s, "id2"s}));
-}
-
-template <class Scenario>
-class TestGenerateId : public Scenario
-{};
-
-using TestScenarios =
- ::testing::Types<ScenarioNameProvided, ScenarioNameNotProvided>;
-TYPED_TEST_SUITE(TestGenerateId, TestScenarios);
-
-TEST_F(TestGenerateIdNameNotProvided, leadingSlashDoesntCountToLengthLimit)
-{
- this->maxSize = 2;
-
- EXPECT_THAT(this->generateId("12", "default").first, Eq("12"));
- EXPECT_THAT(this->generateId("/23", "default").first, Eq("23"));
- EXPECT_THROW(this->generateId("123", "trigger"),
- sdbusplus::exception::SdBusError);
-}
-
-TYPED_TEST(TestGenerateId, throwsWhenProvidedIdIsTooLong)
-{
- this->maxSize = 4;
-
- EXPECT_THROW(this->generateId("12345", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("12/45", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("123/", "trigger"),
- sdbusplus::exception::SdBusError);
-}
-
-TYPED_TEST(TestGenerateId, throwsWhenThereAreNoFreeIds)
-{
- this->maxSize = 1;
- this->conflicts = {"n", "0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
-
- EXPECT_THROW(this->generateId("", "n"), sdbusplus::exception::SdBusError);
-}
-
-TYPED_TEST(TestGenerateId, throwsWhenIdContainsMoreThanOneSlash)
-{
- EXPECT_THROW(this->generateId("/12/", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("12//", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("12//123", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("12/12/123", "name"),
- sdbusplus::exception::SdBusError);
-}
-
-TYPED_TEST(TestGenerateId, usesNamePartOfNameWhenIdNotProvidedAndNameIsTooLong)
-{
- this->maxSize = 4;
-
- this->conflicts = {};
- EXPECT_THAT(this->generateId("", "trigger"),
- Eq(std::pair{"trig"s, "trigger"s}));
- EXPECT_THAT(this->generateId("a/", "trigger"),
- Eq(std::pair{"a/tr"s, "trigger"s}));
-
- this->conflicts = {"trig"};
- EXPECT_THAT(this->generateId("", "trigger"),
- Eq(std::pair{"tri0"s, "trigger"s}));
-
- this->conflicts = {"trig", "tri0"};
- EXPECT_THAT(this->generateId("", "trigger"),
- Eq(std::pair{"tri1"s, "trigger"s}));
-}
-
-TYPED_TEST(TestGenerateId, throwsWhenProvidedIdIsTaken)
-{
- this->conflicts = {"id", "prefix/id"};
-
- EXPECT_THROW(this->generateId("id", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("/id", "name"),
- sdbusplus::exception::SdBusError);
- EXPECT_THROW(this->generateId("prefix/id", "name"),
- sdbusplus::exception::SdBusError);
-}
-
-TYPED_TEST(TestGenerateId, usesNameWhenIdNotProvided)
-{
- EXPECT_THAT(this->generateId("", "name"), Eq(std::pair{"name"s, "name"s}));
- EXPECT_THAT(this->generateId("/", "name"), Eq(std::pair{"name"s, "name"s}));
- EXPECT_THAT(this->generateId("abc/", "name"),
- Eq(std::pair{"abc/name"s, "name"s}));
- EXPECT_THAT(this->generateId("123/", "name"),
- Eq(std::pair{"123/name"s, "name"s}));
-}
-
-TYPED_TEST(TestGenerateId, usesNameWithoutInvalidCharactersWhenIdNotProvided)
-{
- EXPECT_THAT(this->generateId("", "n#a$/m@e"),
- Eq(std::pair{"name"s, "n#a$/m@e"s}));
- EXPECT_THAT(this->generateId("", "n!^aŹ/me"),
- Eq(std::pair{"name"s, "n!^aŹ/me"s}));
- EXPECT_THAT(this->generateId("p/", "n!^aŹ/m*(e"),
- Eq(std::pair{"p/name"s, "n!^aŹ/m*(e"s}));
-}
diff --git a/tests/src/test_make_id_name.cpp b/tests/src/test_make_id_name.cpp
new file mode 100644
index 0000000..e88a8ac
--- /dev/null
+++ b/tests/src/test_make_id_name.cpp
@@ -0,0 +1,189 @@
+#include "helpers.hpp"
+#include "utils/dbus_path_utils.hpp"
+#include "utils/make_id_name.hpp"
+#include "utils/string_utils.hpp"
+
+#include <sdbusplus/exception.hpp>
+
+#include <gmock/gmock.h>
+
+using namespace testing;
+using namespace std::literals::string_literals;
+using namespace utils::string_utils;
+
+class ScenarioBase : public Test
+{
+ public:
+ std::string_view defaultName = "defName";
+ std::vector<std::string> conflicts;
+};
+
+class ScenarioNameProvided : public ScenarioBase
+{
+ public:
+ auto makeIdName(std::string_view id, std::string_view name) const
+ {
+ return utils::makeIdName(id, name, defaultName, conflicts);
+ }
+};
+
+TEST_F(ScenarioNameProvided, throwsWhenProvidedNameIsTooLong)
+{
+ EXPECT_THROW(this->makeIdName("", getTooLongName()),
+ sdbusplus::exception::SdBusError);
+}
+
+class TestMakeIdNameNameProvided : public ScenarioNameProvided
+{};
+
+TEST_F(TestMakeIdNameNameProvided, usesIdWhenProvided)
+{
+ const std::string name = "name";
+
+ EXPECT_THAT(this->makeIdName("id0", name), Eq(std::pair{"id0"s, name}));
+ EXPECT_THAT(this->makeIdName("prefix/id2", name),
+ Eq(std::pair{"prefix/id2"s, name}));
+}
+
+TEST_F(TestMakeIdNameNameProvided, usedDefaultWhenNothingProvided)
+{
+ this->defaultName = "def";
+
+ EXPECT_THAT(this->makeIdName("", ""), Eq(std::pair{"def"s, "def"s}));
+ EXPECT_THAT(this->makeIdName("abc/", ""),
+ Eq(std::pair{"abc/def"s, "def"s}));
+}
+
+TEST_F(TestMakeIdNameNameProvided, usedDefaultWhenNameContainsNoIdChars)
+{
+ this->defaultName = "def";
+ const std::string name = " !";
+
+ EXPECT_THAT(this->makeIdName("", name), Eq(std::pair{"def"s, name}));
+ EXPECT_THAT(this->makeIdName("prefix/", name),
+ Eq(std::pair{"prefix/def"s, name}));
+}
+
+class ScenarioNameNotProvided : public ScenarioBase
+{
+ public:
+ auto makeIdName(std::string_view id, std::string_view name) const
+ {
+ return utils::makeIdName(id, "", name, conflicts);
+ }
+};
+
+class TestMakeIdNameNameNotProvided : public ScenarioNameNotProvided
+{};
+
+TEST_F(TestMakeIdNameNameNotProvided, usesIdAsNameWhenProvided)
+{
+ EXPECT_THAT(this->makeIdName("id0", defaultName),
+ Eq(std::pair{"id0"s, "id0"s}));
+ EXPECT_THAT(this->makeIdName("prefix/id2", defaultName),
+ Eq(std::pair{"prefix/id2"s, "id2"s}));
+}
+
+template <class Scenario>
+class TestMakeIdName : public Scenario
+{};
+
+using TestScenarios =
+ ::testing::Types<ScenarioNameProvided, ScenarioNameNotProvided>;
+TYPED_TEST_SUITE(TestMakeIdName, TestScenarios);
+
+TYPED_TEST(TestMakeIdName, throwsWhenProvidedIdContainsIncorrectCharacters)
+{
+ EXPECT_THROW(this->makeIdName("Id%@%!@%!%()%fooo/Id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("Id/Id%@%!@%!%()%fooo", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("/123", "trigger"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("/123/", "trigger"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestMakeIdName, throwsWhenProvidedIdContainsTooLongSegment)
+{
+ std::string longPrefix = getTooLongPrefix();
+ std::string longSuffix = getTooLongId();
+ EXPECT_THROW(this->makeIdName(longPrefix + "/", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName(longPrefix + "/Id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName(longPrefix + "/" + longSuffix, "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("Prefix/" + longSuffix, "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName(longSuffix, "name"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestMakeIdName, throwsWhenProvidedIdOrPrefixTooLong)
+{
+ EXPECT_THROW(this->makeIdName(getTooLongId(), "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName(getTooLongPrefix() + "/Id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("Prefix/" + getTooLongId(), "trigger"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestMakeIdName, throwsWhenIdContainsMoreThanOneSlash)
+{
+ EXPECT_THROW(this->makeIdName("/12/", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("12//", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("12//123", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("12/12/123", "name"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestMakeIdName, usesNameWhenThereAreConflicts)
+{
+ this->conflicts = {"trigger"};
+ EXPECT_THAT(this->makeIdName("", "trigger"),
+ Eq(std::pair{"trigger0"s, "trigger"s}));
+
+ this->conflicts = {"trigger", "trigger0"};
+ EXPECT_THAT(this->makeIdName("", "trigger"),
+ Eq(std::pair{"trigger1"s, "trigger"s}));
+
+ this->conflicts = {getMaxId()};
+ std::string expectedId = getMaxId();
+ expectedId[expectedId.length() - 1] = '0';
+ EXPECT_THAT(this->makeIdName("", getMaxId()),
+ Eq(std::pair{expectedId, getMaxId()}));
+}
+
+TYPED_TEST(TestMakeIdName, throwsWhenProvidedIdIsTaken)
+{
+ this->conflicts = {"id", "prefix/id"};
+
+ EXPECT_THROW(this->makeIdName("id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->makeIdName("prefix/id", "name"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestMakeIdName, usesNameWhenIdNotProvided)
+{
+ EXPECT_THAT(this->makeIdName("", "name"), Eq(std::pair{"name"s, "name"s}));
+ EXPECT_THAT(this->makeIdName("abc/", "name"),
+ Eq(std::pair{"abc/name"s, "name"s}));
+ EXPECT_THAT(this->makeIdName("123/", "name"),
+ Eq(std::pair{"123/name"s, "name"s}));
+}
+
+TYPED_TEST(TestMakeIdName, usesNameWithoutInvalidCharactersWhenIdNotProvided)
+{
+ EXPECT_THAT(this->makeIdName("", "n#a$/m@e"),
+ Eq(std::pair{"name"s, "n#a$/m@e"s}));
+ EXPECT_THAT(this->makeIdName("", "n!^aŹ/me"),
+ Eq(std::pair{"name"s, "n!^aŹ/me"s}));
+ EXPECT_THAT(this->makeIdName("p/", "n!^aŹ/m*(e"),
+ Eq(std::pair{"p/name"s, "n!^aŹ/m*(e"s}));
+}
diff --git a/tests/src/test_report.cpp b/tests/src/test_report.cpp
index ac2419d..14239a9 100644
--- a/tests/src/test_report.cpp
+++ b/tests/src/test_report.cpp
@@ -16,6 +16,7 @@
#include "utils/conv_container.hpp"
#include "utils/dbus_path_utils.hpp"
#include "utils/messanger.hpp"
+#include "utils/string_utils.hpp"
#include "utils/transform.hpp"
#include "utils/tstring.hpp"
@@ -277,6 +278,29 @@
Eq(newParams));
}
+TEST_F(TestReport, setReadingParametersWithTooLongMetricId)
+{
+ const ReadingParameters currentValue =
+ toReadingParameters(defaultParams().metricParameters());
+
+ ReadingParameters newParams = toReadingParameters(
+ std::vector<LabeledMetricParameters>{{LabeledMetricParameters{
+ {LabeledSensorInfo{"Service",
+ "/xyz/openbmc_project/sensors/power/psu",
+ "NewMetadata123"}},
+ OperationType::avg,
+ utils::string_utils::getTooLongId(),
+ CollectionTimeScope::startup,
+ CollectionDuration(250ms)}}});
+
+ changeProperty<ReadingParameters>(
+ sut->getPath(), "ReadingParametersFutureVersion",
+ {.valueBefore = Eq(currentValue),
+ .newValue = newParams,
+ .ec = Eq(boost::system::errc::invalid_argument),
+ .valueAfter = Eq(currentValue)});
+}
+
TEST_F(TestReport, setReportingTypeWithValidNewType)
{
changeProperty<std::string>(
diff --git a/tests/src/test_report_manager.cpp b/tests/src/test_report_manager.cpp
index 5d0d4a5..6ce0f1f 100644
--- a/tests/src/test_report_manager.cpp
+++ b/tests/src/test_report_manager.cpp
@@ -8,7 +8,10 @@
#include "report.hpp"
#include "report_manager.hpp"
#include "utils/conversion.hpp"
+#include "utils/dbus_path_utils.hpp"
+#include "utils/string_utils.hpp"
#include "utils/transform.hpp"
+#include "utils/tstring.hpp"
using namespace testing;
using namespace std::string_literals;
@@ -162,7 +165,7 @@
TEST_F(TestReportManager, addReportWithMaxLengthId)
{
- std::string reportId(ReportManager::maxReportIdLength, 'z');
+ std::string reportId = utils::string_utils::getMaxId();
reportParams.reportId(reportId);
reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock));
@@ -172,13 +175,142 @@
EXPECT_THAT(path, Eq("/"s + reportId));
}
-TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongName)
+TEST_F(TestReportManager, addReportWithMaxLengthPrefix)
+{
+ std::string reportId = utils::string_utils::getMaxPrefix() + "/MyId";
+ reportParams.reportId(reportId);
+ reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + reportId));
+}
+
+TEST_F(TestReportManager, addReportWithMaxLengthName)
+{
+ reportParams.reportName(utils::string_utils::getMaxName());
+ reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + reportParams.reportId()));
+}
+
+TEST_F(TestReportManager, addReportWithMaxLengthMetricId)
+{
+ namespace ts = utils::tstring;
+ std::vector<LabeledMetricParameters> newMetricParams{
+ {LabeledMetricParameters{
+ {LabeledSensorInfo{"Service",
+ "/xyz/openbmc_project/sensors/power/p1",
+ "metadata1"}},
+ OperationType::avg,
+ utils::string_utils::getMaxId(),
+ CollectionTimeScope::point,
+ CollectionDuration(Milliseconds(0u))}}};
+
+ reportParams.metricParameters(newMetricParams);
+ reportFactoryMock.expectMake(reportParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + reportParams.reportId()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongFullId)
{
reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
.Times(0);
reportParams.reportId(
- std::string(ReportManager::maxReportIdLength + 1, 'z'));
+ std::string(utils::constants::maxReportFullIdLength + 1, 'z'));
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongId)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ reportParams.reportId(utils::string_utils::getTooLongId());
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongPrefix)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ reportParams.reportId(utils::string_utils::getTooLongPrefix() + "/MyId");
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooManyPrefixes)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ std::string reportId;
+ for (size_t i = 0; i < utils::constants::maxPrefixesInId + 1; i++)
+ {
+ reportId += "prefix/";
+ }
+ reportId += "MyId";
+
+ reportParams.reportId(reportId);
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongName)
+{
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ reportParams.reportName(utils::string_utils::getTooLongName());
+
+ auto [ec, path] = addReport(reportParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestReportManager, DISABLED_failToAddReportWithTooLongMetricId)
+{
+ namespace ts = utils::tstring;
+
+ std::vector<LabeledMetricParameters> newMetricParams{
+ {LabeledMetricParameters{
+ {LabeledSensorInfo{"Service",
+ "/xyz/openbmc_project/sensors/power/p1",
+ "metadata1"}},
+ OperationType::avg,
+ utils::string_utils::getTooLongId(),
+ CollectionTimeScope::point,
+ CollectionDuration(Milliseconds(0u))}}};
+
+ reportFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ reportParams.metricParameters(newMetricParams);
auto [ec, path] = addReport(reportParams);
diff --git a/tests/src/test_trigger.cpp b/tests/src/test_trigger.cpp
index 5c41d97..9db2632 100644
--- a/tests/src/test_trigger.cpp
+++ b/tests/src/test_trigger.cpp
@@ -12,7 +12,9 @@
#include "trigger.hpp"
#include "trigger_manager.hpp"
#include "utils/conversion_trigger.hpp"
+#include "utils/dbus_path_utils.hpp"
#include "utils/messanger.hpp"
+#include "utils/string_utils.hpp"
#include "utils/transform.hpp"
#include "utils/tstring.hpp"
@@ -113,6 +115,26 @@
property, newValue);
}
+ template <class T>
+ struct ChangePropertyParams
+ {
+ Matcher<T> valueBefore = _;
+ T newValue;
+ Matcher<boost::system::error_code> ec =
+ Eq(boost::system::errc::success);
+ Matcher<T> valueAfter = Eq(newValue);
+ };
+
+ template <class T>
+ static void changeProperty(const std::string& path,
+ const std::string& property,
+ ChangePropertyParams<T> p)
+ {
+ ASSERT_THAT(getProperty<T>(path, property), p.valueBefore);
+ ASSERT_THAT(setProperty<T>(path, property, p.newValue), p.ec);
+ EXPECT_THAT(getProperty<T>(path, property), p.valueAfter);
+ }
+
boost::system::error_code deleteTrigger(const std::string& path)
{
std::promise<boost::system::error_code> methodPromise;
@@ -233,6 +255,34 @@
Eq(boost::system::errc::invalid_argument));
}
+TEST_F(
+ TestTrigger,
+ DISABLED_settingPropertyReportNamesThrowsExceptionWhenReportWithTooLongPrefix)
+{
+ std::vector<object_path> newPropertyVal{
+ object_path("/xyz/openbmc_project/Telemetry/Reports/" +
+ utils::string_utils::getTooLongPrefix() + "/MyReport")};
+
+ EXPECT_CALL(triggerPresenceChanged, Call(_)).Times(0);
+
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
+ Eq(boost::system::errc::invalid_argument));
+}
+
+TEST_F(
+ TestTrigger,
+ DISABLED_settingPropertyReportNamesThrowsExceptionWhenReportWithTooLongId)
+{
+ std::vector<object_path> newPropertyVal{
+ object_path("/xyz/openbmc_project/Telemetry/Reports/Prefix/" +
+ utils::string_utils::getTooLongId())};
+
+ EXPECT_CALL(triggerPresenceChanged, Call(_)).Times(0);
+
+ EXPECT_THAT(setProperty(sut->getPath(), "Reports", newPropertyVal),
+ Eq(boost::system::errc::invalid_argument));
+}
+
TEST_F(TestTrigger,
DISABLED_settingPropertyReportNamesThrowsExceptionWhenReportWithBadPath)
{
@@ -270,6 +320,36 @@
Eq(boost::system::errc::success));
}
+TEST_F(TestTrigger, setThresholdParamsWithTooLongDiscreteName)
+{
+ const TriggerThresholdParams currentValue =
+ std::visit(utils::FromLabeledThresholdParamConversion(),
+ triggerParams.thresholdParams());
+
+ TriggerThresholdParams newThresholds =
+ std::vector<discrete::ThresholdParam>({std::make_tuple(
+ utils::string_utils::getTooLongName(), "OK", 10, "12.3")});
+
+ changeProperty<TriggerThresholdParams>(
+ sut->getPath(), "Thresholds",
+ {.valueBefore = Eq(currentValue),
+ .newValue = newThresholds,
+ .ec = Eq(boost::system::errc::invalid_argument),
+ .valueAfter = Eq(currentValue)});
+}
+
+TEST_F(TestTrigger, setNameTooLong)
+{
+ std::string currentValue = TriggerParams().name();
+
+ changeProperty<std::string>(
+ sut->getPath(), "Name",
+ {.valueBefore = Eq(currentValue),
+ .newValue = utils::string_utils::getTooLongName(),
+ .ec = Eq(boost::system::errc::invalid_argument),
+ .valueAfter = Eq(currentValue)});
+}
+
TEST_F(TestTrigger, checkIfNumericCoversionsAreGood)
{
const auto& labeledParamsBase =
diff --git a/tests/src/test_trigger_manager.cpp b/tests/src/test_trigger_manager.cpp
index 58d0df1..48c76ea 100644
--- a/tests/src/test_trigger_manager.cpp
+++ b/tests/src/test_trigger_manager.cpp
@@ -7,14 +7,18 @@
#include "trigger.hpp"
#include "trigger_manager.hpp"
#include "utils/conversion_trigger.hpp"
+#include "utils/dbus_path_utils.hpp"
+#include "utils/string_utils.hpp"
#include "utils/transform.hpp"
using namespace testing;
using sdbusplus::message::object_path;
+using namespace std::literals::string_literals;
class TestTriggerManager : public Test
{
public:
+ TriggerParams triggerParams;
std::pair<boost::system::error_code, std::string>
addTrigger(const TriggerParams& params)
{
@@ -207,33 +211,157 @@
TEST_F(TestTriggerManager, addTriggerWithoutIdAndWithLongNameTwice)
{
- addTrigger(TriggerParams().id("").name(
- std::string(2 * TriggerManager::maxTriggerIdLength, 'z')));
+ std::string longName = utils::string_utils::getMaxName();
+ addTrigger(TriggerParams().id("").name(longName));
- auto [ec, path] = addTrigger(TriggerParams().id("").name(
- std::string(2 * TriggerManager::maxTriggerIdLength, 'z')));
+ auto [ec, path] = addTrigger(TriggerParams().id("").name(longName));
EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
EXPECT_THAT(path, Not(Eq("")));
}
TEST_F(TestTriggerManager, addTriggerWithMaxLengthId)
{
- auto triggerId = std::string(TriggerManager::maxTriggerIdLength, 'z');
- auto triggerParams = TriggerParams().id(triggerId);
-
- triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock))
- .WillOnce(Return(ByMove(std::move(triggerMockPtr))));
+ std::string reportId = utils::string_utils::getMaxId();
+ triggerParams.id(reportId);
+ triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock));
auto [ec, path] = addTrigger(triggerParams);
EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
- EXPECT_THAT(path, Eq(triggerMock.getPath()));
+ EXPECT_THAT(path, Eq("/"s + reportId));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithMaxLengthPrefix)
+{
+ std::string reportId = utils::string_utils::getMaxPrefix() + "/MyId";
+ triggerParams.id(reportId);
+ triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + reportId));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithMaxLengthName)
+{
+ triggerParams.name(utils::string_utils::getMaxName());
+ triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + triggerParams.id()));
+}
+
+TEST_F(TestTriggerManager, addTriggerWithMaxLengthDiscreteThresholdName)
+{
+ namespace ts = utils::tstring;
+
+ triggerParams =
+ TriggerParams()
+ .id("DiscreteTrigger")
+ .name("My Discrete Trigger")
+ .thresholdParams(std::vector<discrete::LabeledThresholdParam>{
+ discrete::LabeledThresholdParam{
+ utils::string_utils::getMaxName(),
+ discrete::Severity::warning, Milliseconds(10).count(),
+ "15.2"}});
+
+ triggerFactoryMock.expectMake(triggerParams, Ref(*sut), Ref(storageMock));
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::success));
+ EXPECT_THAT(path, Eq("/"s + triggerParams.id()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongFullId)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ triggerParams.id(
+ std::string(utils::constants::maxReportFullIdLength + 1, 'z'));
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
}
TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongId)
{
- auto triggerId = std::string(TriggerManager::maxTriggerIdLength + 1, 'z');
- auto triggerParams = TriggerParams().id(triggerId);
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ triggerParams.id(utils::string_utils::getTooLongId());
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongPrefix)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ triggerParams.id(utils::string_utils::getTooLongPrefix() + "/MyId");
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooManyPrefixes)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ std::string reportId;
+ for (size_t i = 0; i < utils::constants::maxPrefixesInId + 1; i++)
+ {
+ reportId += "prefix/";
+ }
+ reportId += "MyId";
+
+ triggerParams.id(reportId);
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongName)
+{
+ triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
+ .Times(0);
+
+ triggerParams.name(utils::string_utils::getTooLongName());
+
+ auto [ec, path] = addTrigger(triggerParams);
+
+ EXPECT_THAT(ec.value(), Eq(boost::system::errc::invalid_argument));
+ EXPECT_THAT(path, Eq(std::string()));
+}
+
+TEST_F(TestTriggerManager, DISABLED_failToAddTriggerWithTooLongMetricId)
+{
+ namespace ts = utils::tstring;
+
+ triggerParams =
+ TriggerParams()
+ .id("DiscreteTrigger")
+ .name("My Discrete Trigger")
+ .thresholdParams(std::vector<discrete::LabeledThresholdParam>{
+ discrete::LabeledThresholdParam{
+ utils::string_utils::getTooLongName(),
+ discrete::Severity::warning, Milliseconds(10).count(),
+ "15.2"}});
triggerFactoryMock.expectMake(std::nullopt, Ref(*sut), Ref(storageMock))
.Times(0);
diff --git a/tests/src/utils/string_utils.cpp b/tests/src/utils/string_utils.cpp
new file mode 100644
index 0000000..a39afca
--- /dev/null
+++ b/tests/src/utils/string_utils.cpp
@@ -0,0 +1,74 @@
+#include "utils/string_utils.hpp"
+
+#include "utils/dbus_path_utils.hpp"
+
+#include <cmath>
+
+namespace details
+{
+constexpr std::string_view allowedCharactersInId =
+ "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_";
+
+std::string repeat(size_t n)
+{
+ std::string result;
+ for (size_t i = 0; i < n; i++)
+ {
+ result += allowedCharactersInId;
+ }
+ return result;
+}
+
+std::string getString(size_t length)
+{
+ return details::repeat(
+ std::ceil(static_cast<double>(length) /
+ static_cast<double>(allowedCharactersInId.length())))
+ .substr(0, length);
+}
+
+std::string getStringWithSpaces(size_t length)
+{
+ std::string result = getString(length);
+ size_t idx = 1;
+ while (idx < length)
+ {
+ result[idx] = ' ';
+ idx += 5;
+ }
+ return result;
+}
+} // namespace details
+
+namespace utils::string_utils
+{
+std::string getMaxPrefix()
+{
+ return details::getString(constants::maxPrefixLength);
+}
+
+std::string getMaxId()
+{
+ return details::getString(constants::maxIdNameLength);
+}
+
+std::string getMaxName()
+{
+ return details::getStringWithSpaces(constants::maxIdNameLength);
+}
+
+std::string getTooLongPrefix()
+{
+ return details::getString(constants::maxPrefixLength + 1);
+}
+
+std::string getTooLongId()
+{
+ return details::getString(constants::maxIdNameLength + 1);
+}
+
+std::string getTooLongName()
+{
+ return details::getStringWithSpaces(constants::maxIdNameLength + 1);
+}
+} // namespace utils::string_utils
\ No newline at end of file
diff --git a/tests/src/utils/string_utils.hpp b/tests/src/utils/string_utils.hpp
new file mode 100644
index 0000000..90f802c
--- /dev/null
+++ b/tests/src/utils/string_utils.hpp
@@ -0,0 +1,13 @@
+#pragma once
+
+#include <string>
+
+namespace utils::string_utils
+{
+std::string getMaxPrefix();
+std::string getMaxId();
+std::string getMaxName();
+std::string getTooLongPrefix();
+std::string getTooLongId();
+std::string getTooLongName();
+} // namespace utils::string_utils
\ No newline at end of file