From 226f239c5d85bfc0eb02cd771ddca65a34ca8f73 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 8 May 2024 11:32:42 -0700 Subject: [PATCH] [owning-list] add `RemoveAndFreeAllMatching()` to `OwningList` class (#10210) This commit introduces a new method, `RemoveAndFreeAllMatching()`, to the `OwningList` class. This method removes and frees all entries in the list that match a given indicator. This is used to simplify the native mDNS implementation. The unit test `test_linked_list` is also updated to validate the newly added helper method. --- src/core/common/owning_list.hpp | 23 ++++++++++++++ src/core/net/mdns.cpp | 55 ++++++++++++--------------------- tests/unit/test_linked_list.cpp | 37 ++++++++++++++++++++++ 3 files changed, 79 insertions(+), 36 deletions(-) diff --git a/src/core/common/owning_list.hpp b/src/core/common/owning_list.hpp index 583bc685996..3d1ea5544a7 100644 --- a/src/core/common/owning_list.hpp +++ b/src/core/common/owning_list.hpp @@ -149,6 +149,29 @@ template class OwningList : public LinkedList { LinkedList::RemoveAllMatching(aIndicator, aRemovedList); } + + /** + * Removes and frees all entries in the list matching a given entry indicator. + * + * The template type `Indicator` specifies the type of @p aIndicator object which is used to match against entries + * in the list. To check that an entry matches the given indicator, the `Matches()` method is invoked on each + * `Type` entry in the list. The `Matches()` method should be provided by `Type` class accordingly: + * + * bool Type::Matches(const Indicator &aIndicator) const + * + * @param[in] aIndicator An entry indicator to match against entries in the list. + * + * @retval TRUE At least one matching entry was removed. + * @retval FALSE No matching entry was found. + * + */ + template bool RemoveAndFreeAllMatching(const Indicator &aIndicator) + { + OwningList removedList; + + RemoveAllMatching(aIndicator, removedList); + return !removedList.IsEmpty(); + } }; } // namespace ot diff --git a/src/core/net/mdns.cpp b/src/core/net/mdns.cpp index ddf699539c6..996fa1c3525 100644 --- a/src/core/net/mdns.cpp +++ b/src/core/net/mdns.cpp @@ -313,11 +313,8 @@ void Core::HandleEntryTimer(void) void Core::RemoveEmptyEntries(void) { - OwningList removedHosts; - OwningList removedServices; - - mHostEntries.RemoveAllMatching(Entry::kRemoving, removedHosts); - mServiceEntries.RemoveAllMatching(Entry::kRemoving, removedServices); + mHostEntries.RemoveAndFreeAllMatching(Entry::kRemoving); + mServiceEntries.RemoveAndFreeAllMatching(Entry::kRemoving); } void Core::HandleEntryTask(void) @@ -1874,9 +1871,7 @@ void Core::ServiceEntry::ClearService(void) void Core::ServiceEntry::ScheduleToRemoveIfEmpty(void) { - OwningList removedSubTypes; - - mSubTypes.RemoveAllMatching(EmptyChecker(), removedSubTypes); + mSubTypes.RemoveAndFreeAllMatching(EmptyChecker()); if (IsEmpty()) { @@ -2149,8 +2144,6 @@ void Core::ServiceEntry::PrepareResponseRecords(TxMessage &aResponse, TimeMilli void Core::ServiceEntry::UpdateRecordsState(const TxMessage &aResponse) { - OwningList removedSubTypes; - Entry::UpdateRecordsState(aResponse); mPtrRecord.UpdateStateAfterAnswer(aResponse); @@ -2162,7 +2155,7 @@ void Core::ServiceEntry::UpdateRecordsState(const TxMessage &aResponse) subType.mPtrRecord.UpdateStateAfterAnswer(aResponse); } - mSubTypes.RemoveAllMatching(EmptyChecker(), removedSubTypes); + mSubTypes.RemoveAndFreeAllMatching(EmptyChecker()); if (IsEmpty()) { @@ -4142,11 +4135,10 @@ void Core::TxMessageHistory::CalculateHash(const Message &aMessage, Hash &aHash) void Core::TxMessageHistory::HandleTimer(void) { - TimeMilli now = TimerMilli::GetNow(); - TimeMilli nextTime = now.GetDistantFuture(); - OwningList expiredEntries; + TimeMilli now = TimerMilli::GetNow(); + TimeMilli nextTime = now.GetDistantFuture(); - mHashEntries.RemoveAllMatching(ExpireChecker(now), expiredEntries); + mHashEntries.RemoveAndFreeAllMatching(ExpireChecker(now)); for (const HashEntry &entry : mHashEntries) { @@ -4268,21 +4260,16 @@ void Core::AddPassiveIp6AddrCache(const char *aHostName) void Core::HandleCacheTimer(void) { - CacheTimerContext context(GetInstance()); - ExpireChecker expireChecker(context.GetNow()); - OwningList expiredBrowseList; - OwningList expiredSrvList; - OwningList expiredTxtList; - OwningList expiredIp6AddrList; - OwningList expiredIp4AddrList; + CacheTimerContext context(GetInstance()); + ExpireChecker expireChecker(context.GetNow()); // First remove all expired entries. - mBrowseCacheList.RemoveAllMatching(expireChecker, expiredBrowseList); - mSrvCacheList.RemoveAllMatching(expireChecker, expiredSrvList); - mTxtCacheList.RemoveAllMatching(expireChecker, expiredTxtList); - mIp6AddrCacheList.RemoveAllMatching(expireChecker, expiredIp6AddrList); - mIp4AddrCacheList.RemoveAllMatching(expireChecker, expiredIp4AddrList); + mBrowseCacheList.RemoveAndFreeAllMatching(expireChecker); + mSrvCacheList.RemoveAndFreeAllMatching(expireChecker); + mTxtCacheList.RemoveAndFreeAllMatching(expireChecker); + mIp6AddrCacheList.RemoveAndFreeAllMatching(expireChecker); + mIp4AddrCacheList.RemoveAndFreeAllMatching(expireChecker); // Process cache types in a specific order to optimize name // compression when constructing query messages. @@ -4720,9 +4707,7 @@ void Core::CacheEntry::Remove(const ResultCallback &aCallback) void Core::CacheEntry::ClearEmptyCallbacks(void) { - CallbackList emptyCallbacks; - - mCallbacks.RemoveAllMatching(EmptyChecker(), emptyCallbacks); + mCallbacks.RemoveAndFreeAllMatching(EmptyChecker()); if (mCallbacks.IsEmpty()) { @@ -5765,13 +5750,13 @@ void Core::AddrCache::DetermineRecordFireTime(void) void Core::AddrCache::ProcessExpiredRecords(TimeMilli aNow) { - OwningList expiredEntries; Heap::Array addrArray; AddressResult result; + bool didRemoveAny; - mCommittedEntries.RemoveAllMatching(ExpireChecker(aNow), expiredEntries); + didRemoveAny = mCommittedEntries.RemoveAndFreeAllMatching(ExpireChecker(aNow)); - VerifyOrExit(!expiredEntries.IsEmpty()); + VerifyOrExit(didRemoveAny); ConstructResult(result, addrArray); InvokeCallbacks(result); @@ -5963,9 +5948,7 @@ void Core::AddrCache::CommitNewResponseEntries(void) } else { - OwningList removedEntries; - - mCommittedEntries.RemoveAllMatching(EmptyChecker(), removedEntries); + mCommittedEntries.RemoveAndFreeAllMatching(EmptyChecker()); } while (!mNewEntries.IsEmpty()) diff --git a/tests/unit/test_linked_list.cpp b/tests/unit/test_linked_list.cpp index 8c581d76f15..819f3790f12 100644 --- a/tests/unit/test_linked_list.cpp +++ b/tests/unit/test_linked_list.cpp @@ -313,6 +313,7 @@ void TestOwningList(void) OwningList list; OwningList removedList; OwnedPtr ptr; + bool didRemove; printf("TestOwningList\n"); @@ -433,6 +434,42 @@ void TestOwningList(void) VerifyOrQuit(list.IsEmpty()); VerifyLinkedListContent(&removedList, &c, &d, &f, nullptr); VerifyOrQuit(!c.WasFreed()); + + // Test `RemoveAndFreeAllMatching()` + + a.ResetTestFlags(); + b.ResetTestFlags(); + c.ResetTestFlags(); + d.ResetTestFlags(); + e.ResetTestFlags(); + f.ResetTestFlags(); + list.Push(a); + list.Push(b); + list.Push(c); + list.Push(d); + list.Push(e); + list.Push(f); + VerifyLinkedListContent(&list, &f, &e, &d, &c, &b, &a, nullptr); + + didRemove = list.RemoveAndFreeAllMatching(kAlphaType); + VerifyLinkedListContent(&list, &f, &d, &c, nullptr); + VerifyOrQuit(didRemove); + VerifyOrQuit(a.WasFreed()); + VerifyOrQuit(b.WasFreed()); + VerifyOrQuit(e.WasFreed()); + VerifyOrQuit(!c.WasFreed()); + + didRemove = list.RemoveAndFreeAllMatching(kAlphaType); + VerifyOrQuit(!didRemove); + VerifyLinkedListContent(&list, &f, &d, &c, nullptr); + VerifyOrQuit(!c.WasFreed()); + + didRemove = list.RemoveAndFreeAllMatching(kBetaType); + VerifyOrQuit(list.IsEmpty()); + VerifyOrQuit(didRemove); + VerifyOrQuit(c.WasFreed()); + VerifyOrQuit(d.WasFreed()); + VerifyOrQuit(f.WasFreed()); } } // namespace ot