Redesigned error handling
Current error handling send only error code, which is not enough to
display detailed information. New error handling in additional to
error code send property name. This allows to send meaningful messages
back to used about errors.
Tested:
- Old redfish code properly handles errors (reads only error_code)
- Redfish version which read property name from error displays more
detailed error information
Change-Id: I54caa20881ac3f3e38cb295a3aa95ddab491303f
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
diff --git a/src/errors.cpp b/src/errors.cpp
new file mode 100644
index 0000000..fe88fd0
--- /dev/null
+++ b/src/errors.cpp
@@ -0,0 +1,44 @@
+#include "errors.hpp"
+
+#include <system_error>
+
+namespace errors
+{
+
+using namespace std::literals::string_literals;
+
+InvalidArgument::InvalidArgument(std::string_view propertyNameArg) :
+ propertyName(propertyNameArg),
+ errWhatDetailed("Invalid argument was given for property: "s +
+ description())
+{}
+
+InvalidArgument::InvalidArgument(std::string_view propertyNameArg,
+ std::string_view info) :
+ propertyName(propertyNameArg),
+ errWhatDetailed(
+ ("Invalid argument was given for property: "s + description() + ". "s)
+ .append(info))
+{}
+
+const char* InvalidArgument::name() const noexcept
+{
+ return "org.freedesktop.DBus.Error.InvalidArgs";
+}
+
+const char* InvalidArgument::description() const noexcept
+{
+ return propertyName.c_str();
+}
+
+const char* InvalidArgument::what() const noexcept
+{
+ return errWhatDetailed.c_str();
+}
+
+int InvalidArgument::get_errno() const noexcept
+{
+ return static_cast<int>(std::errc::invalid_argument);
+}
+
+} // namespace errors
diff --git a/src/errors.hpp b/src/errors.hpp
new file mode 100644
index 0000000..e542787
--- /dev/null
+++ b/src/errors.hpp
@@ -0,0 +1,28 @@
+#pragma once
+
+#include <sdbusplus/exception.hpp>
+
+#include <string>
+#include <string_view>
+
+namespace errors
+{
+
+class InvalidArgument final : public sdbusplus::exception::internal_exception
+{
+ public:
+ explicit InvalidArgument(std::string_view propertyName);
+ InvalidArgument(std::string_view propertyName, std::string_view info);
+
+ const char* name() const noexcept override;
+ const char* description() const noexcept override;
+ const char* what() const noexcept override;
+ int get_errno() const noexcept override;
+
+ std::string propertyName;
+
+ private:
+ std::string errWhatDetailed;
+};
+
+} // namespace errors
diff --git a/src/metrics/collection_data.cpp b/src/metrics/collection_data.cpp
index 9512252..7ab5354 100644
--- a/src/metrics/collection_data.cpp
+++ b/src/metrics/collection_data.cpp
@@ -40,9 +40,8 @@
{
if (duration.t.count() == 0)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid CollectionDuration");
+ throw errors::InvalidArgument(
+ "ReadingParameters.CollectionDuration");
}
}
diff --git a/src/report.cpp b/src/report.cpp
index 70cd8f7..6fc6aaf 100644
--- a/src/report.cpp
+++ b/src/report.cpp
@@ -1,5 +1,6 @@
#include "report.hpp"
+#include "errors.hpp"
#include "messages/collect_trigger_id.hpp"
#include "messages/trigger_presence_changed_ind.hpp"
#include "messages/update_report_ind.hpp"
@@ -144,7 +145,7 @@
if (reportIface)
{
- reportIface->signal_property("errors");
+ reportIface->signal_property("ErrorMessages");
}
}
@@ -160,7 +161,7 @@
if (reportIface)
{
- reportIface->signal_property("Errors");
+ reportIface->signal_property("ErrorMessages");
}
}
@@ -266,9 +267,7 @@
if (newValT < ReportManager::minInterval &&
newValT != Milliseconds{0})
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid interval");
+ throw errors::InvalidArgument("Interval");
}
if (newValT != interval)
diff --git a/src/report_factory.cpp b/src/report_factory.cpp
index cb096cf..8b150e1 100644
--- a/src/report_factory.cpp
+++ b/src/report_factory.cpp
@@ -118,6 +118,7 @@
{
return {};
}
+
auto tree = utils::getSubTreeSensors(yield, bus);
return getMetricParamsFromSensorTree(metricParams, tree);
}
@@ -129,6 +130,7 @@
{
return {};
}
+
auto tree = utils::getSubTreeSensors(bus);
return getMetricParamsFromSensorTree(metricParams, tree);
}
@@ -138,52 +140,67 @@
const ReadingParameters& metricParams,
const std::vector<utils::SensorTree>& tree) const
{
- return utils::transform(metricParams, [&tree](const auto& item) {
- auto [sensorPaths, operationType, id, collectionTimeScope,
- collectionDuration] = item;
+ try
+ {
+ return utils::transform(metricParams, [&tree](const auto& item) {
+ auto [sensorPaths, operationType, id, collectionTimeScope,
+ collectionDuration] = item;
- std::vector<LabeledSensorInfo> sensorParameters;
+ std::vector<LabeledSensorInfo> sensorParameters;
- for (const auto& [sensorPath, metadata] : sensorPaths)
- {
- auto it = std::find_if(
- tree.begin(), tree.end(),
- [path = sensorPath](const auto& v) { return v.first == path; });
-
- if (it != tree.end() && it->second.size() == 1)
+ for (const auto& [sensorPath, metadata] : sensorPaths)
{
- const auto& [service, ifaces] = it->second.front();
- sensorParameters.emplace_back(service, sensorPath, metadata);
+ auto it = std::find_if(tree.begin(), tree.end(),
+ [path = sensorPath](const auto& v) {
+ return v.first == path;
+ });
+
+ if (it != tree.end() && it->second.size() == 1)
+ {
+ const auto& [service, ifaces] = it->second.front();
+ sensorParameters.emplace_back(service, sensorPath,
+ metadata);
+ }
}
+
+ if (sensorParameters.size() != sensorPaths.size())
+ {
+ throw errors::InvalidArgument("ReadingParameters",
+ "Service not found.");
+ }
+
+ if (operationType.empty())
+ {
+ operationType = utils::enumToString(OperationType::avg);
+ }
+ else if (operationType == "SINGLE")
+ {
+ operationType = utils::enumToString(OperationType::avg);
+ collectionTimeScope =
+ utils::enumToString(CollectionTimeScope::point);
+ }
+
+ if (collectionTimeScope.empty())
+ {
+ collectionTimeScope =
+ utils::enumToString(CollectionTimeScope::point);
+ }
+
+ return LabeledMetricParameters(
+ std::move(sensorParameters),
+ utils::toOperationType(operationType), id,
+ utils::toCollectionTimeScope(collectionTimeScope),
+ CollectionDuration(Milliseconds(collectionDuration)));
+ });
+ }
+ catch (const errors::InvalidArgument& e)
+ {
+ if (e.propertyName == "ReadingParameters")
+ {
+ throw;
}
- if (sensorParameters.size() != sensorPaths.size())
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Could not find service for provided sensors");
- }
-
- if (operationType.empty())
- {
- operationType = utils::enumToString(OperationType::avg);
- }
- else if (operationType == "SINGLE")
- {
- operationType = utils::enumToString(OperationType::avg);
- collectionTimeScope =
- utils::enumToString(CollectionTimeScope::point);
- }
-
- if (collectionTimeScope.empty())
- {
- collectionTimeScope =
- utils::enumToString(CollectionTimeScope::point);
- }
-
- return LabeledMetricParameters(
- std::move(sensorParameters), utils::toOperationType(operationType),
- id, utils::toCollectionTimeScope(collectionTimeScope),
- CollectionDuration(Milliseconds(collectionDuration)));
- });
+ using namespace std::literals::string_literals;
+ throw errors::InvalidArgument("ReadingParameters."s + e.propertyName);
+ }
}
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index 5812311..932ffe0 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -112,14 +112,28 @@
std::optional<uint64_t> appendLimit;
std::optional<std::string> reportUpdates;
std::optional<ReadingParameters> metricParams;
+ std::optional<ReadingParameters> readingParameters;
std::optional<bool> enabled;
- sdbusplus::unpackProperties(
- properties, "Id", reportId, "Name", reportName,
- "ReportingType", reportingType, "ReportActions",
- reportActions, "Interval", interval, "AppendLimit",
- appendLimit, "ReportUpdates", reportUpdates,
- "MetricParams", metricParams, "Enabled", enabled);
+ try
+ {
+ sdbusplus::unpackProperties(
+ properties, "Id", reportId, "Name", reportName,
+ "ReportingType", reportingType, "ReportActions",
+ reportActions, "Interval", interval, "AppendLimit",
+ appendLimit, "ReportUpdates", reportUpdates,
+ "MetricParams", metricParams, "Enabled", enabled,
+ "ReadingParameters", readingParameters);
+ }
+ catch (const sdbusplus::exception::UnpackPropertyError& e)
+ {
+ throw errors::InvalidArgument(e.propertyName);
+ }
+
+ if (readingParameters == std::nullopt)
+ {
+ readingParameters = metricParams;
+ }
return addReport(
yield, reportId.value_or(""),
@@ -135,11 +149,11 @@
reportAction);
}),
Milliseconds(interval.value_or(0)),
- appendLimit.value_or(0),
+ appendLimit.value_or(maxAppendLimit),
utils::toReportUpdates(
reportUpdates.value_or(utils::enumToString(
ReportUpdates::overwrite))),
- metricParams.value_or(ReadingParameters{}),
+ readingParameters.value_or(ReadingParameters{}),
enabled.value_or(true))
.getPath();
});
@@ -164,9 +178,7 @@
if (readingParam.at_label<ts::Id>().length() >
utils::constants::maxIdNameLength)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "MetricId too long");
+ throw errors::InvalidArgument("ReadingParameters.Id", "Too long.");
}
}
}
@@ -189,17 +201,14 @@
if (appendLimit > maxAppendLimit &&
appendLimit != std::numeric_limits<uint64_t>::max())
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Append limit out of range");
+ throw errors::InvalidArgument("AppendLimit", "Out of range.");
}
if ((reportingType == ReportingType::periodic && interval < minInterval) ||
(reportingType != ReportingType::periodic &&
interval != Milliseconds{0}))
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument), "Invalid interval");
+ throw errors::InvalidArgument("Interval");
}
size_t metricCount = 0;
@@ -213,25 +222,15 @@
if (readingParams.size() > maxNumberMetrics ||
metricCount > maxNumberMetrics)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::argument_list_too_long),
- "Too many reading parameters");
+ throw errors::InvalidArgument("MetricParams", "Too many.");
}
verifyMetricParameters(readingParams);
- try
+ for (const LabeledMetricParameters& item : readingParams)
{
- for (const LabeledMetricParameters& item : readingParams)
- {
- utils::toOperationType(
- utils::toUnderlying(item.at_label<ts::OperationType>()));
- }
- }
- catch (const std::exception& e)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument), e.what());
+ utils::toOperationType(
+ utils::toUnderlying(item.at_label<ts::OperationType>()));
}
}
diff --git a/src/trigger.cpp b/src/trigger.cpp
index c11ea42..ff08638 100644
--- a/src/trigger.cpp
+++ b/src/trigger.cpp
@@ -137,9 +137,8 @@
[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");
+ throw errors::InvalidArgument("Name",
+ "Name is too long.");
}
name = oldVal = newVal;
return 1;
diff --git a/src/trigger_manager.cpp b/src/trigger_manager.cpp
index e337bca..fc6bbcd 100644
--- a/src/trigger_manager.cpp
+++ b/src/trigger_manager.cpp
@@ -88,9 +88,8 @@
if (discreteParam.at_label<ts::UserId>().length() >
utils::constants::maxIdNameLength)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "UserId too long");
+ throw errors::InvalidArgument("ThresholdParams.Id",
+ "UserId too long.");
}
}
}
diff --git a/src/types/collection_time_scope.hpp b/src/types/collection_time_scope.hpp
index 90c4fd3..25e0744 100644
--- a/src/types/collection_time_scope.hpp
+++ b/src/types/collection_time_scope.hpp
@@ -17,6 +17,12 @@
namespace utils
{
+template <>
+struct EnumTraits<CollectionTimeScope>
+{
+ static constexpr auto propertyName = ConstexprString{"CollectionTimeScope"};
+};
+
constexpr std::array<std::pair<std::string_view, CollectionTimeScope>, 3>
convDataCollectionTimeScope = {
{std::make_pair<std::string_view, CollectionTimeScope>(
diff --git a/src/types/error_type.hpp b/src/types/error_type.hpp
index 0f1d61e..00e9bb1 100644
--- a/src/types/error_type.hpp
+++ b/src/types/error_type.hpp
@@ -17,6 +17,12 @@
namespace utils
{
+template <>
+struct EnumTraits<ErrorType>
+{
+ static constexpr auto propertyName = ConstexprString{"ErrorType"};
+};
+
constexpr auto convDataErrorType =
std::array{std::make_pair<std::string_view, ErrorType>(
"PropertyConflict", ErrorType::propertyConflict)};
diff --git a/src/types/operation_type.hpp b/src/types/operation_type.hpp
index 6d19620..cca8b23 100644
--- a/src/types/operation_type.hpp
+++ b/src/types/operation_type.hpp
@@ -17,6 +17,12 @@
namespace utils
{
+template <>
+struct EnumTraits<OperationType>
+{
+ static constexpr auto propertyName = ConstexprString{"OperationType"};
+};
+
constexpr std::array<std::pair<std::string_view, OperationType>, 4>
convDataOperationType = {{std::make_pair<std::string_view, OperationType>(
"Maximum", OperationType::max),
diff --git a/src/types/report_action.hpp b/src/types/report_action.hpp
index 44348f2..3a114e3 100644
--- a/src/types/report_action.hpp
+++ b/src/types/report_action.hpp
@@ -16,6 +16,12 @@
namespace utils
{
+template <>
+struct EnumTraits<ReportAction>
+{
+ static constexpr auto propertyName = ConstexprString{"ReportAction"};
+};
+
constexpr std::array<std::pair<std::string_view, ReportAction>, 2>
convDataReportAction = {
{std::make_pair<std::string_view, ReportAction>(
@@ -40,4 +46,4 @@
return std::string(enumToString(convDataReportAction, value));
}
-} // namespace utils
\ No newline at end of file
+} // namespace utils
diff --git a/src/types/report_updates.hpp b/src/types/report_updates.hpp
index 2640bf0..8313a94 100644
--- a/src/types/report_updates.hpp
+++ b/src/types/report_updates.hpp
@@ -22,12 +22,7 @@
template <>
struct EnumTraits<ReportUpdates>
{
- [[noreturn]] static void throwConversionError()
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid ReportUpdates");
- }
+ static constexpr auto propertyName = ConstexprString{"ReportUpdates"};
};
constexpr auto convDataReportUpdates =
diff --git a/src/types/reporting_type.hpp b/src/types/reporting_type.hpp
index ae0b60a..2c9f500 100644
--- a/src/types/reporting_type.hpp
+++ b/src/types/reporting_type.hpp
@@ -22,12 +22,7 @@
template <>
struct EnumTraits<ReportingType>
{
- [[noreturn]] static void throwConversionError()
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid reportingType");
- }
+ static constexpr auto propertyName = ConstexprString{"ReportingType"};
};
constexpr std::array<std::pair<std::string_view, ReportingType>, 3>
diff --git a/src/types/trigger_types.hpp b/src/types/trigger_types.hpp
index 9a053c8..7ba0768 100644
--- a/src/types/trigger_types.hpp
+++ b/src/types/trigger_types.hpp
@@ -177,3 +177,32 @@
return std::get<std::string>(val);
}
}
+
+namespace utils
+{
+
+template <>
+struct EnumTraits<TriggerAction>
+{
+ static constexpr auto propertyName = ConstexprString{"TriggerAction"};
+};
+
+template <>
+struct EnumTraits<discrete::Severity>
+{
+ static constexpr auto propertyName = ConstexprString{"discrete::Severity"};
+};
+
+template <>
+struct EnumTraits<numeric::Type>
+{
+ static constexpr auto propertyName = ConstexprString{"numeric::Type"};
+};
+
+template <>
+struct EnumTraits<numeric::Direction>
+{
+ static constexpr auto propertyName = ConstexprString{"numeric::Direction"};
+};
+
+} // namespace utils
diff --git a/src/utils/conversion.hpp b/src/utils/conversion.hpp
index fd3dbcb..e92249a 100644
--- a/src/utils/conversion.hpp
+++ b/src/utils/conversion.hpp
@@ -1,5 +1,7 @@
#pragma once
+#include "errors.hpp"
+
#include <sdbusplus/exception.hpp>
#include <algorithm>
@@ -10,24 +12,40 @@
namespace utils
{
-template <class T>
-struct EnumTraits
+template <size_t N>
+struct ConstexprString
{
- [[noreturn]] static void throwConversionError()
+ constexpr ConstexprString(const char (&data)[N]) : s()
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid enum value");
+ for (size_t i = 0; i < N; ++i)
+ {
+ s[i] = data[i];
+ }
}
+
+ constexpr operator std::string_view() const
+ {
+ return {s.data(), s.size()};
+ }
+
+ std::array<char, N> s;
};
+[[noreturn]] inline void throwConversionError(std::string_view propertyName)
+{
+ throw errors::InvalidArgument(propertyName, "Cannot convert.");
+}
+
+template <class T>
+struct EnumTraits;
+
template <class T, T first, T last>
inline T toEnum(std::underlying_type_t<T> x)
{
if (x < static_cast<std::underlying_type_t<T>>(first) ||
x > static_cast<std::underlying_type_t<T>>(last))
{
- EnumTraits<T>::throwConversionError();
+ throwConversionError(EnumTraits<T>::propertyName);
}
return static_cast<T>(x);
}
@@ -77,7 +95,7 @@
[&value](const auto& item) { return item.first == value; });
if (it == std::end(data))
{
- EnumTraits<T>::throwConversionError();
+ throwConversionError(EnumTraits<T>::propertyName);
}
return it->second;
}
@@ -92,7 +110,7 @@
[value](const auto& item) { return item.second == value; });
if (it == std::end(data))
{
- EnumTraits<T>::throwConversionError();
+ throwConversionError(EnumTraits<T>::propertyName);
}
return it->first;
}
diff --git a/src/utils/dbus_path_utils.cpp b/src/utils/dbus_path_utils.cpp
index 5c1fddb..2eabc86 100644
--- a/src/utils/dbus_path_utils.cpp
+++ b/src/utils/dbus_path_utils.cpp
@@ -51,23 +51,17 @@
{
if (pos_start == pos_end)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid prefixes in id");
+ throw errors::InvalidArgument("Id", "Invalid prefixes in id.");
}
if (++prefix_cnt > constants::maxPrefixesInId)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Too many prefixes");
+ throw errors::InvalidArgument("Id", "Too many prefixes.");
}
if (pos_end - pos_start > constants::maxPrefixLength)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Prefix too long");
+ throw errors::InvalidArgument("Id", "Prefix too long.");
}
pos_start = pos_end + 1;
@@ -75,8 +69,7 @@
if (id.length() - pos_start > constants::maxIdNameLength)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument), "Id too long");
+ throw errors::InvalidArgument("Id", "Too long.");
}
}
} // namespace utils
diff --git a/src/utils/dbus_path_utils.hpp b/src/utils/dbus_path_utils.hpp
index 3798d98..c1bda62 100644
--- a/src/utils/dbus_path_utils.hpp
+++ b/src/utils/dbus_path_utils.hpp
@@ -1,5 +1,7 @@
#pragma once
+#include "errors.hpp"
+
#include <sdbusplus/message.hpp>
#include <algorithm>
@@ -53,9 +55,7 @@
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");
+ throw errors::InvalidArgument("Id", "Invalid character.");
}
}
diff --git a/src/utils/make_id_name.cpp b/src/utils/make_id_name.cpp
index 0df6c17..1390dad 100644
--- a/src/utils/make_id_name.cpp
+++ b/src/utils/make_id_name.cpp
@@ -93,8 +93,7 @@
{
if (name.length() > constants::maxIdNameLength)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument), "Name too long");
+ throw errors::InvalidArgument("Name", "Too long.");
}
if (name.empty() && !id.ends_with('/'))