From 56045a7590928888b5632675479f7b136a1182a1 Mon Sep 17 00:00:00 2001 From: Shamser Ahmed Date: Tue, 1 Oct 2024 13:12:05 +0100 Subject: [PATCH] Changes following review Signed-off-by: Shamser Ahmed --- dali/base/sysinfologger.cpp | 88 +++++++++++++++++++-------------- dali/base/sysinfologger.hpp | 24 ++++----- testing/unittests/dalitests.cpp | 27 +++++----- 3 files changed, 76 insertions(+), 63 deletions(-) diff --git a/dali/base/sysinfologger.cpp b/dali/base/sysinfologger.cpp index 1f7bae5c197..242d303733f 100644 --- a/dali/base/sysinfologger.cpp +++ b/dali/base/sysinfologger.cpp @@ -34,7 +34,7 @@ #define ATTR_HIDDEN "@hidden" #define ATTR_SOURCE "@source" -static void extractDate(unsigned __int64 ts, unsigned & year, unsigned & month, unsigned & day) +static void extractDate(timestamp_type ts, unsigned & year, unsigned & month, unsigned & day) { CDateTime timeStamp; timeStamp.setTimeStamp(ts); @@ -77,7 +77,7 @@ class CSysInfoLoggerMsg : implements ISysInfoLoggerMsg { msgPtree.setown(createPTree(MSG_NODE)); } - CSysInfoLoggerMsg(unsigned id, const LogMsgCategory & cat, LogMsgCode code, const char * source, const char * msg, unsigned __int64 ts, bool hidden) + CSysInfoLoggerMsg(unsigned id, const LogMsgCategory & cat, LogMsgCode code, const char * source, const char * msg, timestamp_type ts, bool hidden) { msgPtree.setown(createPTree(MSG_NODE)); msgPtree->setPropInt64(ATTR_ID, id); @@ -99,7 +99,7 @@ class CSysInfoLoggerMsg : implements ISysInfoLoggerMsg { return msgPtree->getPropBool(ATTR_HIDDEN, false); } - virtual unsigned __int64 queryTimeStamp() const override + virtual timestamp_type queryTimeStamp() const override { return msgPtree->getPropInt64(ATTR_TIMESTAMP); } @@ -161,7 +161,7 @@ class CSysInfoLoggerMsgFilter : public CSimpleInterfaceOf { Linked filter; + // N.b. IRemoteConnection exists for the duration of the iterator so if this iterator exists for too long, it could cause + // performance issues for other clients: consider caching some messages and releasing connection (and reopening as necessary). Owned conn; bool updateable = false; Owned msgIter; @@ -426,7 +427,7 @@ class CSysInfoLoggerMsgIterator : public CSimpleInterfaceOfisValid(); msgIter->next()) { - unsigned __int64 ts = msgIter->query().getPropInt64(ATTR_TIMESTAMP, 0); + timestamp_type ts = msgIter->query().getPropInt64(ATTR_TIMESTAMP, 0); if (filter->isInDateRange(ts)) return true; } @@ -476,9 +477,9 @@ class CSysInfoLoggerMsgIterator : public CSimpleInterfaceOf filter = new CSysInfoLoggerMsgFilter(visibleOnly, hiddenOnly, year, month, day); + Owned filter = new CSysInfoLoggerMsgFilter(visibleOnly, hiddenOnly, year, month, day, source); return new CSysInfoLoggerMsgIterator(filter, false); } @@ -488,7 +489,7 @@ ISysInfoLoggerMsgIterator * createSysInfoLoggerMsgIterator(IConstSysInfoLoggerMs } // returns messageId -unsigned __int64 logSysInfoError(const LogMsgCategory & cat, LogMsgCode code, const char *source, const char * msg, unsigned __int64 ts) +unsigned __int64 logSysInfoError(const LogMsgCategory & cat, LogMsgCode code, const char *source, const char * msg, timestamp_type ts) { if (ts==0) ts = getTimeStampNowValue(); @@ -533,9 +534,9 @@ unsigned updateMessage(IConstSysInfoLoggerMsgFilter * msgFilter, std::function updateOp) +unsigned updateMessage(unsigned __int64 msgId, const char *source, std::function updateOp) { - Owned msgFilter = createSysInfoLoggerMsgFilter(msgId); + Owned msgFilter = createSysInfoLoggerMsgFilter(msgId, source); return updateMessage(msgFilter, updateOp); } @@ -544,9 +545,9 @@ unsigned hideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter) return updateMessage(msgFilter, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(true);}); } -bool hideLogSysInfoMsg(unsigned __int64 msgId) +bool hideLogSysInfoMsg(unsigned __int64 msgId, const char *source) { - return updateMessage(msgId, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(true);})==1; + return updateMessage(msgId, source, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(true);})==1; } unsigned unhideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter) @@ -554,9 +555,9 @@ unsigned unhideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter) return updateMessage(msgFilter, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(false);}); } -bool unhideLogSysInfoMsg(unsigned __int64 msgId) +bool unhideLogSysInfoMsg(unsigned __int64 msgId, const char *source) { - return updateMessage(msgId, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(false);})==1; + return updateMessage(msgId, source, [](CSysInfoLoggerMsg & sysInfoMsg){sysInfoMsg.setHidden(false);})==1; } unsigned deleteLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter) @@ -577,20 +578,19 @@ unsigned deleteLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter) unsigned count = 0; for (auto & xpath: deleteXpathList) { - if (root->removeProp(xpath.c_str())); + if (root->removeProp(xpath.c_str())) ++count; } return count; } -bool deleteLogSysInfoMsg(unsigned __int64 msgId) +bool deleteLogSysInfoMsg(unsigned __int64 msgId, const char *source) { - Owned msgFilter = createSysInfoLoggerMsgFilter(); - msgFilter->setMatchMsgId(msgId); + Owned msgFilter = createSysInfoLoggerMsgFilter(msgId, source); return deleteLogSysInfoMsg(msgFilter); } -unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigned year, unsigned month, unsigned day) +unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigned year, unsigned month, unsigned day, const char *source) { if (!year && month) throw makeStringExceptionV(-1, "deleteOlderThanLogSysInfoMsg: year must be provided if month is specified (year=%u, month=%u, day=%u)", year, month, day); @@ -601,14 +601,16 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne if (day>31) throw makeStringExceptionV(-1, "deleteOlderThanLogSysInfoMsg: invalid day(year=%u, month=%u, day=%u)", year, month, day); // With visibleOnly/hiddenOnly option, use createSysInfoLoggerMsgFilter() - if (visibleOnly || hiddenOnly) + if (visibleOnly || hiddenOnly || day) { unsigned count = 0; - Owned msgFilter = createSysInfoLoggerMsgFilter(); + Owned msgFilter = createSysInfoLoggerMsgFilter(source); if (hiddenOnly) msgFilter->setHiddenOnly(); if (visibleOnly) msgFilter->setVisibleOnly(); + if (source) + msgFilter->setMatchSource(source); msgFilter->setOlderThanDate(year, month, day); return deleteLogSysInfoMsg(msgFilter); } @@ -620,7 +622,9 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne std::vector deleteXpathList; IPropertyTree * root = conn->queryRoot(); + // future: optimize by getting only minimum set of subtrees to delete and get sorted elements(so search can stop earlier) Owned monthIter = root->getElements("*"); + Owned innerException; //only first exception record/reported ForEach(*monthIter) { IPropertyTree & monthPT = monthIter->query(); @@ -636,7 +640,11 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne msgMonth = readDigits(p, 2, false); } if (msgYear == 0 || msgMonth == 0) - throw makeStringExceptionV(-1, "child of " SYS_INFO_ROOT " is invalid: %s", monthPT.queryName()); + { + if (!innerException) + innerException.setown(makeStringExceptionV(-1, "child of " SYS_INFO_ROOT " is invalid: %s", monthPT.queryName())); + continue; + } if (msgYear > year) continue; if (msgYear < year) @@ -659,7 +667,11 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne if (*d++ == 'd') msgDay = readDigits(d, 2); if (msgDay == 0) - throw makeStringExceptionV(-1, "child of " SYS_INFO_ROOT "/%s is invalid: %s", monthPT.queryName(), dayPT.queryName()); + { + if (!innerException) + innerException.setown(makeStringExceptionV(-1, "child of " SYS_INFO_ROOT "/%s is invalid: %s", monthPT.queryName(), dayPT.queryName())); + continue; + } if (day && (msgDay >= day)) continue; @@ -670,7 +682,6 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne } } } - unsigned count = 0; for (auto & xpath: deleteXpathList) { @@ -678,5 +689,8 @@ unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigne ++count; } + if (innerException) // allow items to be deleted even if there is an exception + throw innerException.getClear(); + return count; } diff --git a/dali/base/sysinfologger.hpp b/dali/base/sysinfologger.hpp index 488923bdfed..fe2ad33f985 100644 --- a/dali/base/sysinfologger.hpp +++ b/dali/base/sysinfologger.hpp @@ -30,7 +30,7 @@ interface ISysInfoLoggerMsg { virtual bool queryIsHidden() const = 0; - virtual unsigned __int64 queryTimeStamp() const = 0; + virtual timestamp_type queryTimeStamp() const = 0; virtual const char * querySource() const = 0; virtual LogMsgCode queryLogMsgCode() const = 0; virtual LogMsgAudience queryAudience() const = 0; @@ -42,10 +42,10 @@ interface ISysInfoLoggerMsg interface IConstSysInfoLoggerMsgFilter : public IInterface { virtual bool hasDateRange() const = 0; - virtual bool isInDateRange(unsigned __int64 ts) const = 0; + virtual bool isInDateRange(timestamp_type ts) const = 0; virtual bool queryHiddenOnly() const = 0; virtual bool queryVisibleOnly() const = 0; - virtual unsigned __int64 queryMatchTimeStamp() const = 0; + virtual timestamp_type queryMatchTimeStamp() const = 0; virtual unsigned queryStartYear() const = 0; virtual unsigned queryStartMonth() const = 0; virtual unsigned queryStartDay() const = 0; @@ -63,7 +63,7 @@ interface ISysInfoLoggerMsgFilter : extends IConstSysInfoLoggerMsgFilter { virtual void setHiddenOnly() = 0; virtual void setVisibleOnly() = 0; - virtual void setMatchTimeStamp(unsigned __int64 ts) = 0; + virtual void setMatchTimeStamp(timestamp_type ts) = 0; virtual void setMatchSource(const char * source) = 0; virtual void setMatchCode(LogMsgCode code) = 0; virtual void setMatchAudience(LogMsgAudience audience) = 0; @@ -75,18 +75,18 @@ interface ISysInfoLoggerMsgFilter : extends IConstSysInfoLoggerMsgFilter typedef IIteratorOf ISysInfoLoggerMsgIterator; -SYSINFO_API ISysInfoLoggerMsgFilter * createSysInfoLoggerMsgFilter(); -SYSINFO_API ISysInfoLoggerMsgFilter * createSysInfoLoggerMsgFilter(unsigned __int64 msgId); -SYSINFO_API ISysInfoLoggerMsgIterator * createSysInfoLoggerMsgIterator(bool _visibleOnly, bool _hiddenOnly, unsigned _year, unsigned _month, unsigned _day); +SYSINFO_API ISysInfoLoggerMsgFilter * createSysInfoLoggerMsgFilter(const char *source=nullptr); +SYSINFO_API ISysInfoLoggerMsgFilter * createSysInfoLoggerMsgFilter(unsigned __int64 msgId, const char *source=nullptr); +SYSINFO_API ISysInfoLoggerMsgIterator * createSysInfoLoggerMsgIterator(bool _visibleOnly, bool _hiddenOnly, unsigned _year, unsigned _month, unsigned _day, const char *source=nullptr); SYSINFO_API ISysInfoLoggerMsgIterator * createSysInfoLoggerMsgIterator(ISysInfoLoggerMsgFilter * msgFilter); SYSINFO_API unsigned __int64 logSysInfoError(const LogMsgCategory & cat, LogMsgCode code, const char *source, const char * msg, unsigned __int64 ts); SYSINFO_API unsigned hideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter); -SYSINFO_API bool hideLogSysInfoMsg(unsigned __int64 msgId); -SYSINFO_API unsigned unhideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter); -SYSINFO_API bool unhideLogSysInfoMsg(unsigned __int64 msgId); +SYSINFO_API bool hideLogSysInfoMsg(unsigned __int64 msgId, const char *source=nullptr); +SYSINFO_API unsigned unhideLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter, const char *source=nullptr); +SYSINFO_API bool unhideLogSysInfoMsg(unsigned __int64 msgId, const char *source=nullptr); SYSINFO_API unsigned deleteLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilter); -SYSINFO_API bool deleteLogSysInfoMsg(unsigned __int64 msgId); -SYSINFO_API unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigned year, unsigned month, unsigned day); +SYSINFO_API bool deleteLogSysInfoMsg(unsigned __int64 msgId, const char *source=nullptr); +SYSINFO_API unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigned year, unsigned month, unsigned day, const char *source=nullptr); #endif diff --git a/testing/unittests/dalitests.cpp b/testing/unittests/dalitests.cpp index 870b5998df3..cbba197fbf4 100644 --- a/testing/unittests/dalitests.cpp +++ b/testing/unittests/dalitests.cpp @@ -3035,7 +3035,7 @@ class CFileNameNormalizeUnitTest : public CppUnit::TestFixture, CDfsLogicalFileN CPPUNIT_TEST_SUITE_REGISTRATION( CFileNameNormalizeUnitTest ); CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( CFileNameNormalizeUnitTest, "CFileNameNormalizeUnitTest" ); -#define SOURCE_COMPONENT "sysinfologger-unittest" +#define SOURCE_COMPONENT_UNITTEST "sysinfologger-unittest" class DaliSysInfoLoggerTester : public CppUnit::TestFixture { @@ -3136,12 +3136,12 @@ class DaliSysInfoLoggerTester : public CppUnit::TestFixture try { std::set matchedMessages; // used to make sure every message written has been read back - Owned iter = createSysInfoLoggerMsgIterator(visibleOnly, hiddenOnly, year, month, day); + Owned iter = createSysInfoLoggerMsgIterator(visibleOnly, hiddenOnly, year, month, day, SOURCE_COMPONENT_UNITTEST); ForEach(*iter) { const ISysInfoLoggerMsg & sysInfoMsg = iter->query(); - if (strcmp(sysInfoMsg.querySource(), SOURCE_COMPONENT)!=0) + if (strcmp(sysInfoMsg.querySource(), SOURCE_COMPONENT_UNITTEST)!=0) continue; // not a message written by this unittest so ignore // Check written message matches read message @@ -3195,11 +3195,11 @@ class DaliSysInfoLoggerTester : public CppUnit::TestFixture dateTime.setString(testCase.dateTimeStamp); unsigned __int64 ts = dateTime.getTimeStamp(); - unsigned __int64 msgId = logSysInfoError(testCase.cat, testCase.code, SOURCE_COMPONENT, testCase.msg, ts); + unsigned __int64 msgId = logSysInfoError(testCase.cat, testCase.code, SOURCE_COMPONENT_UNITTEST, testCase.msg, ts); writtenMessages.push_back({msgId, ts, testCaseIndex++}); if (testCase.hidden) { - Owned msgFilter = createSysInfoLoggerMsgFilter(msgId); + Owned msgFilter = createSysInfoLoggerMsgFilter(msgId, SOURCE_COMPONENT_UNITTEST); ASSERT(hideLogSysInfoMsg(msgFilter)==1); } } @@ -3218,7 +3218,7 @@ class DaliSysInfoLoggerTester : public CppUnit::TestFixture void testSysInfoLogger() { // cleanup - remove messages that may have been left over from previous run - deleteOlderThanLogSysInfoMsg(false, false, 2001, 03, 00); + deleteOlderThanLogSysInfoMsg(false, false, 2001, 03, 00, SOURCE_COMPONENT_UNITTEST); // Start of tests testWrite(); ASSERT(testRead(false, false)==9); @@ -3226,8 +3226,8 @@ class DaliSysInfoLoggerTester : public CppUnit::TestFixture ASSERT(testRead(false, false, 2000, 02, 04)==5); ASSERT(testRead(false, true)==5); //all visible messages ASSERT(testRead(true, false)==4); //all hidden messages - ASSERT(deleteOlderThanLogSysInfoMsg(false, true, 2000, 02, 03)==2); - ASSERT(deleteOlderThanLogSysInfoMsg(true, false, 2000, 02, 04)==5); + ASSERT(deleteOlderThanLogSysInfoMsg(false, true, 2000, 02, 03, SOURCE_COMPONENT_UNITTEST)==2); + ASSERT(deleteOlderThanLogSysInfoMsg(true, false, 2000, 02, 04, SOURCE_COMPONENT_UNITTEST)==5); // testCase[7] and [8] are the only 2 remaining // Delete single message test: delete testCase[7] @@ -3236,26 +3236,25 @@ class DaliSysInfoLoggerTester : public CppUnit::TestFixture if (matched==writtenMessages.end()) throw makeStringExceptionV(-1, "Can't find test case %u in written messages", testCaseId); - Owned msgFilter = createSysInfoLoggerMsgFilter(matched->msgId); + Owned msgFilter = createSysInfoLoggerMsgFilter(matched->msgId, SOURCE_COMPONENT_UNITTEST); ASSERT(deleteLogSysInfoMsg(msgFilter)==1); // Verify only 1 message remaining ASSERT(testRead(false, false)==1); // Delete 2000/02/04 and 2000/02/03 (one message but there are 2 parents remaining) - ASSERT(deleteOlderThanLogSysInfoMsg(false, false, 2000, 02, 05)==2); + ASSERT(deleteOlderThanLogSysInfoMsg(false, false, 2000, 02, 05, SOURCE_COMPONENT_UNITTEST)==1); // There shouldn't be any records remaining ASSERT(testRead(false, false)==0); testWrite(); // delete all messages with MsgCode 42303 -> 3 messages - msgFilter.setown(createSysInfoLoggerMsgFilter()); + msgFilter.setown(createSysInfoLoggerMsgFilter(SOURCE_COMPONENT_UNITTEST)); msgFilter->setMatchCode(42304); ASSERT(deleteLogSysInfoMsg(msgFilter)==3); - // delete all messages matching source=SOURCE_COMPONENT - msgFilter.setown(createSysInfoLoggerMsgFilter()); - msgFilter->setMatchSource(SOURCE_COMPONENT); + // delete all messages matching source=SOURCE_COMPONENT_UNITTEST + msgFilter.setown(createSysInfoLoggerMsgFilter(SOURCE_COMPONENT_UNITTEST)); ASSERT(deleteLogSysInfoMsg(msgFilter)==6); } };