Skip to content

Commit

Permalink
Use safer System::Clock types in dnssd (#11062)
Browse files Browse the repository at this point in the history 
* Use safer System::Clock types in dnssd

#### Problem

Code uses plain integers to represent time values and relies on
users to get the unit scale correct.

Part of #10062 _Some operations on System::Clock types are not safe_

#### Change overview

Convert `src/lib/dnssd` to use the safer `Clock` types.

#### Testing

CI; no change to functionality intended.

Conversion includes `TestActiveResolveAttempts.cpp`.

* restyle
  • Loading branch information
@kpschoedel @pull
kpschoedel authored and pull[bot] committed Aug 30, 2022
1 parent 59e0cd1 commit 4102915
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 96 deletions.
6 changes: 3 additions & 3 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,14 +513,14 @@ CHIP_ERROR MinMdnsResolver::ScheduleResolveRetries()
ReturnErrorCodeIf(mSystemLayer == nullptr, CHIP_ERROR_INCORRECT_STATE);
mSystemLayer->CancelTimer(&ResolveRetryCallback, this);

Optional<uint32_t> delayMs = mActiveResolves.GetMsUntilNextExpectedResponse();
Optional<System::Clock::Timeout> delay = mActiveResolves.GetTimeUntilNextExpectedResponse();

if (!delayMs.HasValue())
if (!delay.HasValue())
{
return CHIP_NO_ERROR;
}

return mSystemLayer->StartTimer(System::Clock::Milliseconds32(delayMs.Value()), &ResolveRetryCallback, this);
return mSystemLayer->StartTimer(delay.Value(), &ResolveRetryCallback, this);
}

void MinMdnsResolver::ResolveRetryCallback(System::Layer *, void * self)
Expand Down
41 changes: 21 additions & 20 deletions src/lib/dnssd/minimal_mdns/ActiveResolveAttempts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ using namespace chip;
namespace mdns {
namespace Minimal {

constexpr chip::System::Clock::Timeout ActiveResolveAttempts::kMaxRetryDelay;

void ActiveResolveAttempts::Reset()

{
Expand Down Expand Up @@ -56,9 +58,9 @@ void ActiveResolveAttempts::MarkPending(const PeerId & peerId)
// Strategy when picking the peer id to use:
// 1 if a matching peer id is already found, use that one
// 2 if an 'unused' entry is found, use that
// 3 otherwise expire the one with the largest nextRetryDelaySec
// or if equal nextRetryDelaySec, pick the one with the oldest
// queryDueTimeMs
// 3 otherwise expire the one with the largest nextRetryDelay
// or if equal nextRetryDelay, pick the one with the oldest
// queryDueTime

RetryEntry * entryToUse = &mRetryQueue[0];

Expand Down Expand Up @@ -94,12 +96,11 @@ void ActiveResolveAttempts::MarkPending(const PeerId & peerId)
// - on same delay, use queryDueTime to determine the oldest request
// (the one with the smallest due time was issued the longest time
// ago)
if (entry->nextRetryDelaySec > entryToUse->nextRetryDelaySec)
if (entry->nextRetryDelay > entryToUse->nextRetryDelay)
{
entryToUse = entry;
}
else if ((entry->nextRetryDelaySec == entryToUse->nextRetryDelaySec) &&
(entry->queryDueTimeMs < entryToUse->queryDueTimeMs))
else if ((entry->nextRetryDelay == entryToUse->nextRetryDelay) && (entry->queryDueTime < entryToUse->queryDueTime))
{
entryToUse = entry;
}
Expand All @@ -117,16 +118,16 @@ void ActiveResolveAttempts::MarkPending(const PeerId & peerId)
ChipLogError(Discovery, "Re-using pending resolve entry before reply was received.");
}

entryToUse->peerId = peerId;
entryToUse->queryDueTimeMs = mClock->GetMonotonicMilliseconds();
entryToUse->nextRetryDelaySec = 1;
entryToUse->peerId = peerId;
entryToUse->queryDueTime = mClock->GetMonotonicTimestamp();
entryToUse->nextRetryDelay = System::Clock::Seconds16(1);
}

Optional<uint32_t> ActiveResolveAttempts::GetMsUntilNextExpectedResponse() const
Optional<System::Clock::Timeout> ActiveResolveAttempts::GetTimeUntilNextExpectedResponse() const
{
Optional<uint32_t> minDelay = Optional<uint32_t>::Missing();
Optional<System::Clock::Timeout> minDelay = Optional<System::Clock::Timeout>::Missing();

chip::System::Clock::MonotonicMilliseconds nowMs = mClock->GetMonotonicMilliseconds();
chip::System::Clock::Timestamp now = mClock->GetMonotonicTimestamp();

for (auto & entry : mRetryQueue)
{
Expand All @@ -135,13 +136,13 @@ Optional<uint32_t> ActiveResolveAttempts::GetMsUntilNextExpectedResponse() const
continue;
}

if (nowMs >= entry.queryDueTimeMs)
if (now >= entry.queryDueTime)
{
// found an entry that needs processing right now
return Optional<uint32_t>::Value(0);
return Optional<System::Clock::Timeout>::Value(0);
}

uint32_t entryDelay = static_cast<int>(entry.queryDueTimeMs - nowMs);
System::Clock::Timeout entryDelay = entry.queryDueTime - now;
if (!minDelay.HasValue() || (minDelay.Value() > entryDelay))
{
minDelay.SetValue(entryDelay);
Expand All @@ -153,7 +154,7 @@ Optional<uint32_t> ActiveResolveAttempts::GetMsUntilNextExpectedResponse() const

Optional<PeerId> ActiveResolveAttempts::NextScheduledPeer()
{
chip::System::Clock::MonotonicMilliseconds nowMs = mClock->GetMonotonicMilliseconds();
chip::System::Clock::Timestamp now = mClock->GetMonotonicTimestamp();

for (auto & entry : mRetryQueue)
{
Expand All @@ -162,20 +163,20 @@ Optional<PeerId> ActiveResolveAttempts::NextScheduledPeer()
continue; // not a pending item
}

if (entry.queryDueTimeMs > nowMs)
if (entry.queryDueTime > now)
{
continue; // not yet due
}

if (entry.nextRetryDelaySec > kMaxRetryDelaySec)
if (entry.nextRetryDelay > kMaxRetryDelay)
{
ChipLogError(Discovery, "Timeout waiting for mDNS resolution.");
entry.peerId.SetNodeId(kUndefinedNodeId);
continue;
}

entry.queryDueTimeMs = nowMs + entry.nextRetryDelaySec * 1000L;
entry.nextRetryDelaySec *= 2;
entry.queryDueTime = now + entry.nextRetryDelay;
entry.nextRetryDelay *= 2;

return Optional<PeerId>::Value(entry.peerId);
}
Expand Down
12 changes: 6 additions & 6 deletions src/lib/dnssd/minimal_mdns/ActiveResolveAttempts.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ namespace Minimal {
class ActiveResolveAttempts
{
public:
static constexpr size_t kRetryQueueSize = 4;
static constexpr uint32_t kMaxRetryDelaySec = 16;
static constexpr size_t kRetryQueueSize = 4;
static constexpr chip::System::Clock::Timeout kMaxRetryDelay = chip::System::Clock::Seconds16(16);

ActiveResolveAttempts(chip::System::Clock::ClockBase * clock) : mClock(clock) { Reset(); }

Expand All @@ -55,10 +55,10 @@ class ActiveResolveAttempts
/// by NextScheduledPeer (potentially with others as well)
void MarkPending(const chip::PeerId & peerId);

// Get minimum milliseconds until the next pending reply is required.
// Get minimum time until the next pending reply is required.
//
// Returns missing if no actively tracked elements exist.
chip::Optional<uint32_t> GetMsUntilNextExpectedResponse() const;
chip::Optional<chip::System::Clock::Timeout> GetTimeUntilNextExpectedResponse() const;

// Get the peer Id that needs scheduling for a query
//
Expand All @@ -79,7 +79,7 @@ class ActiveResolveAttempts
chip::PeerId peerId;

// When a reply is expected for this item
chip::System::Clock::MonotonicMilliseconds queryDueTimeMs;
chip::System::Clock::Timestamp queryDueTime;

// Next expected delay for sending if reply is not reached by
// 'queryDueTimeMs'
Expand All @@ -89,7 +89,7 @@ class ActiveResolveAttempts
// one second
// - the intervals between successive queries MUST increase by at
// least a factor of two
uint32_t nextRetryDelaySec = 1;
chip::System::Clock::Timeout nextRetryDelay = chip::System::Clock::Seconds16(1);
};

chip::System::Clock::ClockBase * mClock;
Expand Down
7 changes: 3 additions & 4 deletions src/lib/dnssd/minimal_mdns/ResponseSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ CHIP_ERROR ResponseSender::Respond(uint32_t messageId, const QueryData & query,

// send all 'Answer' replies
{
const uint64_t kTimeNowMs = chip::System::SystemClock().GetMonotonicMilliseconds();
const chip::System::Clock::Timestamp kTimeNow = chip::System::SystemClock().GetMonotonicTimestamp();

QueryReplyFilter queryReplyFilter(query);
QueryResponderRecordFilter responseFilter;
Expand All @@ -102,8 +102,7 @@ CHIP_ERROR ResponseSender::Respond(uint32_t messageId, const QueryData & query,
//
// TODO: the 'last sent' value does NOT track the interface we used to send, so this may cause
// broadcasts on one interface to throttle broadcasts on another interface.
constexpr uint64_t kOneSecondMs = 1000;
responseFilter.SetIncludeOnlyMulticastBeforeMS(kTimeNowMs - kOneSecondMs);
responseFilter.SetIncludeOnlyMulticastBeforeMS(kTimeNow - chip::System::Clock::Seconds32(1));
}
for (size_t i = 0; i < kMaxQueryResponders; ++i)
{
Expand All @@ -120,7 +119,7 @@ CHIP_ERROR ResponseSender::Respond(uint32_t messageId, const QueryData & query,

if (!mSendState.SendUnicast())
{
it->lastMulticastTime = kTimeNowMs;
it->lastMulticastTime = kTimeNow;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/dnssd/minimal_mdns/responders/QueryResponder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ void QueryResponderBase::ClearBroadcastThrottle()
{
for (size_t i = 0; i < mResponderInfoSize; i++)
{
mResponderInfos[i].lastMulticastTime = 0;
mResponderInfos[i].lastMulticastTime = chip::System::Clock::Zero;
}
}

Expand Down
18 changes: 9 additions & 9 deletions src/lib/dnssd/minimal_mdns/responders/QueryResponder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@ namespace Minimal {
/// Represents available data (replies) for mDNS queries.
struct QueryResponderRecord
{
Responder * responder = nullptr; // what response/data is available
bool reportService = false; // report as a service when listing dnssd services
uint64_t lastMulticastTime = 0; // last time this record was multicast
Responder * responder = nullptr; // what response/data is available
bool reportService = false; // report as a service when listing dnssd services
chip::System::Clock::Timestamp lastMulticastTime{ 0 }; // last time this record was multicast
};

namespace Internal {
Expand Down Expand Up @@ -122,9 +122,9 @@ class QueryResponderRecordFilter

/// Filter out anything that was multicast past ms.
/// If ms is 0, no filtering is done
QueryResponderRecordFilter & SetIncludeOnlyMulticastBeforeMS(uint64_t ms)
QueryResponderRecordFilter & SetIncludeOnlyMulticastBeforeMS(chip::System::Clock::Timestamp time)
{
mIncludeOnlyMulticastBeforeMS = ms;
mIncludeOnlyMulticastBefore = time;
return *this;
}

Expand All @@ -140,7 +140,7 @@ class QueryResponderRecordFilter
return false;
}

if ((mIncludeOnlyMulticastBeforeMS > 0) && (record->lastMulticastTime >= mIncludeOnlyMulticastBeforeMS))
if ((mIncludeOnlyMulticastBefore > chip::System::Clock::Zero) && (record->lastMulticastTime >= mIncludeOnlyMulticastBefore))
{
return false;
}
Expand All @@ -154,9 +154,9 @@ class QueryResponderRecordFilter
}

private:
bool mIncludeAdditionalRepliesOnly = false;
ReplyFilter * mReplyFilter = nullptr;
uint64_t mIncludeOnlyMulticastBeforeMS = 0;
bool mIncludeAdditionalRepliesOnly = false;
ReplyFilter * mReplyFilter = nullptr;
chip::System::Clock::Timestamp mIncludeOnlyMulticastBefore{ 0 };
};

/// Iterates over an array of QueryResponderRecord items, providing only 'valid' ones, where
Expand Down
Loading

0 comments on commit 4102915

Please sign in to comment.