Simplify journal paging
sd-journal has its own paging mechanisms for generating and seeking to
unique ids. Ironically they look fairly similar to what we've
implemented here, but they have more content, presumably because they
can use internal implementation details to do paging.
This commit switches all sd-journal paging to use cursors. Functionally
this changes the odata.id from being a concatenated string into being a
base64 encoded identifier that is much longer.
The end result is vastly simplified code.
Tested:
check journal script [1] succeeds
[1] https://github.com/openbmc/openbmc-tools/blob/master/check_journal/check_journal
Script runs to completion and shows all tests passed.
Change-Id: Ia49fbfc272bef6dfbe1ea45a8d993dc785041659
Signed-off-by: Ed Tanous <etanous@nvidia.com>
diff --git a/meson.build b/meson.build
index 18aa27b..b66d55a 100644
--- a/meson.build
+++ b/meson.build
@@ -449,7 +449,6 @@
'test/redfish-core/lib/chassis_test.cpp',
'test/redfish-core/lib/log_services_dump_test.cpp',
'test/redfish-core/lib/manager_diagnostic_data_test.cpp',
- 'test/redfish-core/lib/manager_logservices_journal_test.cpp',
'test/redfish-core/lib/metadata_test.cpp',
'test/redfish-core/lib/power_subsystem_test.cpp',
'test/redfish-core/lib/service_root_test.cpp',
diff --git a/redfish-core/lib/manager_logservices_journal.hpp b/redfish-core/lib/manager_logservices_journal.hpp
index 17fb95b..8bc1549 100644
--- a/redfish-core/lib/manager_logservices_journal.hpp
+++ b/redfish-core/lib/manager_logservices_journal.hpp
@@ -19,92 +19,6 @@
namespace redfish
{
-// Entry is formed like "BootID_timestamp" or "BootID_timestamp_index"
-inline bool
- getTimestampFromID(const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
- std::string_view entryIDStrView, sd_id128_t& bootID,
- uint64_t& timestamp, uint64_t& index)
-{
- // Convert the unique ID back to a bootID + timestamp to find the entry
- auto underscore1Pos = entryIDStrView.find('_');
- if (underscore1Pos == std::string_view::npos)
- {
- // EntryID has no bootID or timestamp
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
-
- // EntryID has bootID + timestamp
-
- // Convert entryIDViewString to BootID
- // NOTE: bootID string which needs to be null-terminated for
- // sd_id128_from_string()
- std::string bootIDStr(entryIDStrView.substr(0, underscore1Pos));
- if (sd_id128_from_string(bootIDStr.c_str(), &bootID) < 0)
- {
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
-
- // Get the timestamp from entryID
- entryIDStrView.remove_prefix(underscore1Pos + 1);
-
- auto [timestampEnd, tstampEc] = std::from_chars(
- entryIDStrView.begin(), entryIDStrView.end(), timestamp);
- if (tstampEc != std::errc())
- {
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
- entryIDStrView = std::string_view(
- timestampEnd,
- static_cast<size_t>(std::distance(timestampEnd, entryIDStrView.end())));
- if (entryIDStrView.empty())
- {
- index = 0U;
- return true;
- }
- // Timestamp might include optional index, if two events happened at the
- // same "time".
- if (entryIDStrView[0] != '_')
- {
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
- entryIDStrView.remove_prefix(1);
- auto [ptr, indexEc] =
- std::from_chars(entryIDStrView.begin(), entryIDStrView.end(), index);
- if (indexEc != std::errc() || ptr != entryIDStrView.end())
- {
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
- if (index <= 1)
- {
- // Indexes go directly from no postfix (handled above) to _2
- // so if we ever see _0 or _1, it's incorrect
- messages::resourceNotFound(asyncResp->res, "LogEntry", entryIDStrView);
- return false;
- }
-
- // URI indexes are one based, journald is zero based
- index -= 1;
- return true;
-}
-
-inline std::string getUniqueEntryID(uint64_t index, uint64_t curTs,
- sd_id128_t& curBootID)
-{
- // make entryID as <bootID>_<timestamp>[_<index>]
- std::array<char, SD_ID128_STRING_MAX> bootIDStr{};
- sd_id128_to_string(curBootID, bootIDStr.data());
- std::string postfix;
- if (index > 0)
- {
- postfix = std::format("_{}", index + 1);
- }
- return std::format("{}_{}{}", bootIDStr.data(), curTs, postfix);
-}
inline int getJournalMetadata(sd_journal* journal, std::string_view field,
std::string_view& contents)
@@ -157,13 +71,21 @@
}
inline bool fillBMCJournalLogEntryJson(
- const std::string& bmcJournalLogEntryID, sd_journal* journal,
- nlohmann::json::object_t& bmcJournalLogEntryJson)
+ sd_journal* journal, nlohmann::json::object_t& bmcJournalLogEntryJson)
{
+ char* cursor = nullptr;
+ int ret = sd_journal_get_cursor(journal, &cursor);
+ if (ret < 0)
+ {
+ return false;
+ }
+ std::unique_ptr<char*> cursorptr = std::make_unique<char*>(cursor);
+ std::string bmcJournalLogEntryID(cursor);
+
// Get the Log Entry contents
std::string message;
std::string_view syslogID;
- int ret = getJournalMetadata(journal, "SYSLOG_IDENTIFIER", syslogID);
+ ret = getJournalMetadata(journal, "SYSLOG_IDENTIFIER", syslogID);
if (ret < 0)
{
BMCWEB_LOG_DEBUG("Failed to read SYSLOG_IDENTIFIER field: {}",
@@ -200,11 +122,15 @@
// Fill in the log entry with the gathered data
bmcJournalLogEntryJson["@odata.type"] = "#LogEntry.v1_9_0.LogEntry";
+
+ std::string entryIdBase64 =
+ crow::utility::base64encode(bmcJournalLogEntryID);
+
bmcJournalLogEntryJson["@odata.id"] = boost::urls::format(
"/redfish/v1/Managers/{}/LogServices/Journal/Entries/{}",
- BMCWEB_REDFISH_MANAGER_URI_NAME, bmcJournalLogEntryID);
+ BMCWEB_REDFISH_MANAGER_URI_NAME, entryIdBase64);
bmcJournalLogEntryJson["Name"] = "BMC Journal Entry";
- bmcJournalLogEntryJson["Id"] = bmcJournalLogEntryID;
+ bmcJournalLogEntryJson["Id"] = entryIdBase64;
bmcJournalLogEntryJson["Message"] = std::move(message);
bmcJournalLogEntryJson["EntryType"] = log_entry::LogEntryType::Oem;
log_entry::EventSeverity severityEnum = log_entry::EventSeverity::OK;
@@ -262,9 +188,6 @@
struct JournalReadState
{
std::unique_ptr<sd_journal, decltype(&sd_journal_close)> journal;
- uint64_t index = 0;
- sd_id128_t prevBootID{};
- uint64_t prevTs = 0;
};
inline void readJournalEntries(
@@ -305,41 +228,8 @@
return;
}
- // Get the entry timestamp
- sd_id128_t curBootID{};
- uint64_t curTs = 0;
- int ret = sd_journal_get_monotonic_usec(readState.journal.get(), &curTs,
- &curBootID);
- if (ret < 0)
- {
- BMCWEB_LOG_ERROR("Failed to read entry timestamp: {}",
- strerror(-ret));
- messages::internalError(asyncResp->res);
- return;
- }
-
- // If the timestamp isn't unique on the same boot, increment the index
- bool sameBootIDs = sd_id128_equal(curBootID, readState.prevBootID) != 0;
- if (sameBootIDs && (curTs == readState.prevTs))
- {
- readState.index++;
- }
- else
- {
- // Otherwise, reset it
- readState.index = 0;
- }
-
- // Save the bootID
- readState.prevBootID = curBootID;
-
- // Save the timestamp
- readState.prevTs = curTs;
-
- std::string idStr = getUniqueEntryID(readState.index, curTs, curBootID);
-
nlohmann::json::object_t bmcJournalLogEntry;
- if (!fillBMCJournalLogEntryJson(idStr, readState.journal.get(),
+ if (!fillBMCJournalLogEntryJson(readState.journal.get(),
bmcJournalLogEntry))
{
messages::internalError(asyncResp->res);
@@ -347,7 +237,7 @@
}
logEntryArray->emplace_back(std::move(bmcJournalLogEntry));
- ret = sd_journal_next(readState.journal.get());
+ int ret = sd_journal_next(readState.journal.get());
if (ret < 0)
{
messages::internalError(asyncResp->res);
@@ -479,8 +369,6 @@
BMCWEB_REDFISH_MANAGER_URI_NAME, std::to_string(skip + top));
}
uint64_t index = 0;
- sd_id128_t curBootID{};
- uint64_t curTs = 0;
if (skip > 0)
{
if (sd_journal_next_skip(journal.get(), skip) < 0)
@@ -488,88 +376,9 @@
messages::internalError(asyncResp->res);
return;
}
-
- // Get the entry timestamp
- ret = sd_journal_get_monotonic_usec(journal.get(), &curTs, &curBootID);
- if (ret < 0)
- {
- BMCWEB_LOG_ERROR("Failed to read entry timestamp: {}",
- strerror(-ret));
- messages::internalError(asyncResp->res);
- return;
- }
-
- uint64_t endChunkSeqNum = 0;
-#if LIBSYSTEMD_VERSION >= 254
- {
- if (sd_journal_get_seqnum(journal.get(), &endChunkSeqNum, nullptr) <
- 0)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- }
-#endif
-
- // Seek to the first entry with the same timestamp and boot
- ret = sd_journal_seek_monotonic_usec(journal.get(), curBootID, curTs);
- if (ret < 0)
- {
- BMCWEB_LOG_ERROR("Failed to seek: {}", strerror(-ret));
- messages::internalError(asyncResp->res);
- return;
- }
- if (sd_journal_next(journal.get()) < 0)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- uint64_t startChunkSeqNum = 0;
-#if LIBSYSTEMD_VERSION >= 254
- {
- if (sd_journal_get_seqnum(journal.get(), &startChunkSeqNum,
- nullptr) < 0)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- }
-#endif
-
- // Get the difference between the start and end. Most of the time this
- // will be 0
- BMCWEB_LOG_DEBUG("start={} end={}", startChunkSeqNum, endChunkSeqNum);
- index = endChunkSeqNum - startChunkSeqNum;
- if (index > endChunkSeqNum)
- {
- // Detect underflows. Should never happen.
- messages::internalError(asyncResp->res);
- return;
- }
- if (index > 0)
- {
- BMCWEB_LOG_DEBUG("index = {}", index);
- if (sd_journal_next_skip(journal.get(), index) < 0)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- }
- }
- // If this is the first entry of this series, reset the timestamps so the
- // Index doesn't increment
- if (index == 0)
- {
- curBootID = {};
- curTs = 0;
- }
- else
- {
- index -= 1;
}
BMCWEB_LOG_DEBUG("Index was {}", index);
- readJournalEntries(top, asyncResp,
- {std::move(journal), index, curBootID, curTs});
+ readJournalEntries(top, asyncResp, {std::move(journal)});
}
inline void handleManagersJournalEntriesLogEntryGet(
@@ -588,15 +397,6 @@
return;
}
- // Convert the unique ID back to a timestamp to find the entry
- sd_id128_t bootID{};
- uint64_t ts = 0;
- uint64_t index = 0;
- if (!getTimestampFromID(asyncResp, entryID, bootID, ts, index))
- {
- return;
- }
-
sd_journal* journalTmp = nullptr;
int ret = sd_journal_open(&journalTmp, SD_JOURNAL_LOCAL_ONLY);
if (ret < 0)
@@ -608,25 +408,42 @@
std::unique_ptr<sd_journal, decltype(&sd_journal_close)> journal(
journalTmp, sd_journal_close);
journalTmp = nullptr;
- // Go to the timestamp in the log and move to the entry at the
- // index tracking the unique ID
- ret = sd_journal_seek_monotonic_usec(journal.get(), bootID, ts);
+
+ std::string cursor;
+ if (!crow::utility::base64Decode(entryID, cursor))
+ {
+ messages::resourceNotFound(asyncResp->res, "LogEntry", entryID);
+ return;
+ }
+
+ // Go to the cursor in the log
+ ret = sd_journal_seek_cursor(journal.get(), cursor.c_str());
if (ret < 0)
{
- BMCWEB_LOG_ERROR("failed to seek to an entry in journal{}",
- strerror(-ret));
+ messages::resourceNotFound(asyncResp->res, "LogEntry", entryID);
+ return;
+ }
+
+ if (sd_journal_next(journal.get()) < 0)
+ {
messages::internalError(asyncResp->res);
return;
}
- if (sd_journal_next_skip(journal.get(), index + 1) < 0)
+ ret = sd_journal_test_cursor(journal.get(), cursor.c_str());
+ if (ret == 0)
+ {
+ messages::resourceNotFound(asyncResp->res, "LogEntry", entryID);
+ return;
+ }
+ if (ret < 0)
{
messages::internalError(asyncResp->res);
return;
}
nlohmann::json::object_t bmcJournalLogEntry;
- if (!fillBMCJournalLogEntryJson(entryID, journal.get(), bmcJournalLogEntry))
+ if (!fillBMCJournalLogEntryJson(journal.get(), bmcJournalLogEntry))
{
messages::internalError(asyncResp->res);
return;
diff --git a/test/redfish-core/lib/manager_logservices_journal_test.cpp b/test/redfish-core/lib/manager_logservices_journal_test.cpp
deleted file mode 100644
index 86c201c..0000000
--- a/test/redfish-core/lib/manager_logservices_journal_test.cpp
+++ /dev/null
@@ -1,71 +0,0 @@
-#include "async_resp.hpp"
-#include "manager_logservices_journal.hpp"
-
-#include <systemd/sd-id128.h>
-
-#include <cstdint>
-#include <format>
-#include <memory>
-#include <string>
-
-#include <gtest/gtest.h>
-
-namespace redfish
-{
-namespace
-{
-
-TEST(LogServicesBMCJouralTest, LogServicesBMCJouralGetReturnsError)
-{
- auto shareAsyncResp = std::make_shared<bmcweb::AsyncResp>();
- sd_id128_t bootIDOut{};
- uint64_t timestampOut = 0;
- uint64_t indexOut = 0;
- uint64_t timestampIn = 1740970301UL;
- std::string badBootIDStr = "78770392794344a29f81507f3ce5e";
- std::string goodBootIDStr = "78770392794344a29f81507f3ce5e78c";
- sd_id128_t goodBootID{};
-
- // invalid test cases
- EXPECT_FALSE(getTimestampFromID(shareAsyncResp, "", bootIDOut, timestampOut,
- indexOut));
- EXPECT_FALSE(getTimestampFromID(shareAsyncResp, badBootIDStr, bootIDOut,
- timestampOut, indexOut));
- EXPECT_FALSE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}", badBootIDStr, timestampIn),
- bootIDOut, timestampOut, indexOut));
- EXPECT_FALSE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}", badBootIDStr, timestampIn),
- bootIDOut, timestampOut, indexOut));
-
- // obtain a goodBootID
- EXPECT_GE(sd_id128_from_string(goodBootIDStr.c_str(), &goodBootID), 0);
-
- EXPECT_FALSE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}", goodBootIDStr, "InvalidNum"),
- bootIDOut, timestampOut, indexOut));
-
- // Success cases
- EXPECT_TRUE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}", goodBootIDStr, timestampIn),
- bootIDOut, timestampOut, indexOut));
- EXPECT_NE(sd_id128_equal(goodBootID, bootIDOut), 0);
- EXPECT_EQ(timestampIn, timestampOut);
- EXPECT_EQ(indexOut, 0);
-
- // Index of _1 is invalid. First index is omitted
- EXPECT_FALSE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}_1", goodBootIDStr, timestampIn),
- bootIDOut, timestampOut, indexOut));
-
- // Index of _2 is valid, and should return a zero index (1)
- EXPECT_TRUE(getTimestampFromID(
- shareAsyncResp, std::format("{}_{}_2", goodBootIDStr, timestampIn),
- bootIDOut, timestampOut, indexOut));
- EXPECT_NE(sd_id128_equal(goodBootID, bootIDOut), 0);
- EXPECT_EQ(timestampIn, timestampOut);
- EXPECT_EQ(indexOut, 1);
-}
-
-} // namespace
-} // namespace redfish