Skip to content

Commit

Permalink
Stop using ResolverProxy for resolving node lookups (project-chip#29264)
Browse files Browse the repository at this point in the history 
* Stop using ResolverProxy for performing Operational node discovery

* Switch avahi to be capable to keep track of contexts and handle stopping node resolution

* Restyle

* Remove obsolete todo

* Make sure stop resolve actuall clears multiple entries if they exist

* increase log level verbositity for openiot to see progress in pytest

* A bit more rich logging in the openiot utils - show successes and failures

* report failure on commissioning (likely timeout, but show it since it is not none/bool

* Restyle

* Switch Dns.cpp to use address resolver for dns resolution

* Restyle

* remove operational delegate functionality from ResolverDelegateProxy and more cleanups on members/methods

* Restyle

* Schedule next result to display all results of a discovery

* Fix namespacing

* Code review update

---------

Co-authored-by: Andrei Litvin <andreilitvin@google.com>
  • Loading branch information
@andy31415 @andreilitvin
andy31415 and andreilitvin authored Sep 15, 2023
1 parent 64df62c commit a9a4259
Show file tree
Hide file tree
Showing 11 changed files with 247 additions and 230 deletions.
2 changes: 1 addition & 1 deletion scripts/examples/openiotsdk_example.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ function run_test() {
fi

set +e
pytest --json-report --json-report-summary --json-report-file="$EXAMPLE_TEST_PATH"/test_report_"$EXAMPLE".json --binaryPath="$EXAMPLE_EXE_PATH" --fvp="$FVP_BIN" --fvpConfig="$FVP_CONFIG_FILE" "${TEST_OPTIONS[@]}" "$EXAMPLE_TEST_PATH"/test_app.py
pytest --log-level=INFO --json-report --json-report-summary --json-report-file="$EXAMPLE_TEST_PATH"/test_report_"$EXAMPLE".json --binaryPath="$EXAMPLE_EXE_PATH" --fvp="$FVP_BIN" --fvpConfig="$FVP_CONFIG_FILE" "${TEST_OPTIONS[@]}" "$EXAMPLE_TEST_PATH"/test_app.py
set -e

if [[ ! -f $EXAMPLE_TEST_PATH/test_report_$EXAMPLE.json ]]; then
Expand Down
176 changes: 77 additions & 99 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,70 +55,6 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span<
proxy->Release();
}

static void HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & addresses, CHIP_ERROR error)
{
ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(context);
if (CHIP_NO_ERROR != error)
{
proxy->OnOperationalNodeResolutionFailed(PeerId(), error);
proxy->Release();
return;
}

VerifyOrDie(proxy != nullptr);

if (result == nullptr)
{
proxy->OnOperationalNodeResolutionFailed(PeerId(), CHIP_ERROR_UNKNOWN_RESOURCE_ID);
proxy->Release();
return;
}

VerifyOrDie(proxy != nullptr);

PeerId peerId;
error = ExtractIdFromInstanceName(result->mName, &peerId);
if (CHIP_NO_ERROR != error)
{
proxy->OnOperationalNodeResolutionFailed(PeerId(), error);
proxy->Release();
return;
}

VerifyOrDie(proxy != nullptr);

ResolvedNodeData nodeData;
Platform::CopyString(nodeData.resolutionData.hostName, result->mHostName);
nodeData.resolutionData.interfaceId = result->mInterface;
nodeData.resolutionData.port = result->mPort;
nodeData.operationalData.peerId = peerId;

size_t addressesFound = 0;
for (auto & ip : addresses)
{
if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress))
{
// Out of space.
ChipLogProgress(Discovery, "Can't add more IPs to ResolvedNodeData");
break;
}
nodeData.resolutionData.ipAddress[addressesFound] = ip;
++addressesFound;
}
nodeData.resolutionData.numIPs = addressesFound;

for (size_t i = 0; i < result->mTextEntrySize; ++i)
{
ByteSpan key(reinterpret_cast<const uint8_t *>(result->mTextEntries[i].mKey), strlen(result->mTextEntries[i].mKey));
ByteSpan val(result->mTextEntries[i].mData, result->mTextEntries[i].mDataSize);
FillNodeDataFromTxt(key, val, nodeData.resolutionData);
}

nodeData.LogNodeIdResolved();
proxy->OnOperationalNodeResolved(nodeData);
proxy->Release();
}

static void HandleNodeBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error)
{
ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(context);
Expand Down Expand Up @@ -336,6 +272,68 @@ CHIP_ERROR AddTxtRecord(TxtFieldKey key, TextEntry * entries, size_t & entriesCo

} // namespace

void DiscoveryImplPlatform::HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & addresses,
CHIP_ERROR error)
{
DiscoveryImplPlatform * impl = static_cast<DiscoveryImplPlatform *>(context);

if (impl->mOperationalDelegate == nullptr)
{
ChipLogError(Discovery, "No delegate to handle node resolution data.");
return;
}

if (CHIP_NO_ERROR != error)
{
impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), error);
return;
}

if (result == nullptr)
{
impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), CHIP_ERROR_UNKNOWN_RESOURCE_ID);
return;
}

PeerId peerId;
error = ExtractIdFromInstanceName(result->mName, &peerId);
if (CHIP_NO_ERROR != error)
{
impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), error);
return;
}

ResolvedNodeData nodeData;
Platform::CopyString(nodeData.resolutionData.hostName, result->mHostName);
nodeData.resolutionData.interfaceId = result->mInterface;
nodeData.resolutionData.port = result->mPort;
nodeData.operationalData.peerId = peerId;

size_t addressesFound = 0;
for (auto & ip : addresses)
{
if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress))
{
// Out of space.
ChipLogProgress(Discovery, "Can't add more IPs to ResolvedNodeData");
break;
}
nodeData.resolutionData.ipAddress[addressesFound] = ip;
++addressesFound;
}
nodeData.resolutionData.numIPs = addressesFound;

for (size_t i = 0; i < result->mTextEntrySize; ++i)
{
ByteSpan key(reinterpret_cast<const uint8_t *>(result->mTextEntries[i].mKey), strlen(result->mTextEntries[i].mKey));
ByteSpan val(result->mTextEntries[i].mData, result->mTextEntries[i].mDataSize);
FillNodeDataFromTxt(key, val, nodeData.resolutionData);
}

nodeData.LogNodeIdResolved();
impl->mOperationalDelegate->OnOperationalNodeResolved(nodeData);
}

void DnssdService::ToDiscoveredNodeData(const Span<Inet::IPAddress> & addresses, DiscoveredNodeData & nodeData)
{
auto & resolutionData = nodeData.resolutionData;
Expand Down Expand Up @@ -633,15 +631,27 @@ bool DiscoveryImplPlatform::IsInitialized()

CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId)
{
ReturnErrorOnFailure(InitImpl());
return mResolverProxy.ResolveNodeId(peerId);
// Resolve requests can only be issued once DNSSD is initialized and there is
// no caching currently
VerifyOrReturnError(mState == State::kInitialized, CHIP_ERROR_INCORRECT_STATE);

ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...",
ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()));

DnssdService service;

ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId));
Platform::CopyString(service.mType, kOperationalServiceName);
service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp;
service.mAddressType = Inet::IPAddressType::kAny;

return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, this);
}

void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
char name[Common::kInstanceNameMaxLength + 1];
ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId));

ChipDnssdResolveNoLongerNeeded(name);
}

Expand Down Expand Up @@ -684,38 +694,6 @@ Resolver & chip::Dnssd::Resolver::Instance()
return DiscoveryImplPlatform::GetInstance();
}

CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
{
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);

ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...",
ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()));

DnssdService service;

ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId));
Platform::CopyString(service.mType, kOperationalServiceName);
service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp;
service.mAddressType = Inet::IPAddressType::kAny;

mDelegate->Retain();

CHIP_ERROR err = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate);
if (err != CHIP_NO_ERROR)
{
mDelegate->Release();
}
return err;
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
char name[Common::kInstanceNameMaxLength + 1];
ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId));

ChipDnssdResolveNoLongerNeeded(name);
}

ResolverProxy::~ResolverProxy()
{
Shutdown();
Expand Down
6 changes: 5 additions & 1 deletion src/lib/dnssd/Discovery_ImplPlatform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
CHIP_ERROR UpdateCommissionableInstanceName() override;

// Members that implement Resolver interface.
void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mResolverProxy.SetOperationalDelegate(delegate); }
void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; }
void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override
{
mResolverProxy.SetCommissioningDelegate(delegate);
Expand Down Expand Up @@ -91,9 +91,13 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
size_t subTypeSize, uint16_t port, Inet::InterfaceId interfaceId, const chip::ByteSpan & mac,
DnssdServiceProtocol procotol, PeerId peerId);

static void HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & addresses,
CHIP_ERROR error);

State mState = State::kUninitialized;
uint8_t mCommissionableInstanceName[sizeof(uint64_t)];
ResolverProxy mResolverProxy;
OperationalResolveDelegate * mOperationalDelegate = nullptr;

static DiscoveryImplPlatform sManager;
};
Expand Down
63 changes: 10 additions & 53 deletions src/lib/dnssd/ResolverProxy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,40 +23,12 @@
namespace chip {
namespace Dnssd {

class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>,
public OperationalResolveDelegate,
public CommissioningResolveDelegate
class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>, public CommissioningResolveDelegate

{
public:
void SetOperationalDelegate(OperationalResolveDelegate * delegate) { mOperationalDelegate = delegate; }
void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) { mCommissioningDelegate = delegate; }

// OperationalResolveDelegate
void OnOperationalNodeResolved(const ResolvedNodeData & nodeData) override
{
if (mOperationalDelegate != nullptr)
{
mOperationalDelegate->OnOperationalNodeResolved(nodeData);
}
else
{
ChipLogError(Discovery, "Missing operational delegate. Data discarded.");
}
}

void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override
{
if (mOperationalDelegate != nullptr)
{
mOperationalDelegate->OnOperationalNodeResolutionFailed(peerId, error);
}
else
{
ChipLogError(Discovery, "Missing operational delegate. Failure info discarded.");
}
}

// CommissioningResolveDelegate
void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override
{
Expand All @@ -71,7 +43,6 @@ class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>,
}

private:
OperationalResolveDelegate * mOperationalDelegate = nullptr;
CommissioningResolveDelegate * mCommissioningDelegate = nullptr;
};

Expand All @@ -90,13 +61,6 @@ class ResolverProxy : public Resolver

if (mDelegate != nullptr)
{
if (mPreInitOperationalDelegate != nullptr)
{
ChipLogProgress(Discovery, "Setting operational delegate post init");
mDelegate->SetOperationalDelegate(mPreInitOperationalDelegate);
mPreInitOperationalDelegate = nullptr;
}

if (mPreInitCommissioningDelegate != nullptr)
{
ChipLogProgress(Discovery, "Setting commissioning delegate post init");
Expand All @@ -112,18 +76,10 @@ class ResolverProxy : public Resolver

void SetOperationalDelegate(OperationalResolveDelegate * delegate) override
{
if (mDelegate != nullptr)
{
mDelegate->SetOperationalDelegate(delegate);
}
else
{
if (delegate != nullptr)
{
ChipLogProgress(Discovery, "Delaying proxy of operational discovery: missing delegate");
}
mPreInitOperationalDelegate = delegate;
}
/// Unfortunately cannot remove this method since it is in a Resolver interface.
ChipLogError(Discovery, "!!! Operational proxy does NOT support operational discovery");
ChipLogError(Discovery, "!!! Please use AddressResolver or DNSSD Resolver directly");
chipDie(); // force detection of invalid usages.
}

void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override
Expand All @@ -145,22 +101,23 @@ class ResolverProxy : public Resolver
void Shutdown() override
{
VerifyOrReturn(mDelegate != nullptr);
mDelegate->SetOperationalDelegate(nullptr);
mDelegate->SetCommissioningDelegate(nullptr);
mDelegate->Release();
mDelegate = nullptr;
}

CHIP_ERROR ResolveNodeId(const PeerId & peerId) override;
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override;
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
CHIP_ERROR StopDiscovery() override;
CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override;

// TODO: ResolverProxy should not be used anymore to implement operational node resolution
// This method still here because Resolver interface requires it
CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override {}

private:
ResolverDelegateProxy * mDelegate = nullptr;
OperationalResolveDelegate * mPreInitOperationalDelegate = nullptr;
CommissioningResolveDelegate * mPreInitCommissioningDelegate = nullptr;

// While discovery (commissionable or commissioner) is ongoing,
Expand Down
19 changes: 0 additions & 19 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,25 +748,6 @@ ResolverProxy::~ResolverProxy()
Shutdown();
}

// Minimal implementation does not support associating a context to a request (while platforms implementations do). So keep
// updating the delegate that ends up being used by the server by calling 'SetOperationalDelegate'.
// This effectively allow minimal to have multiple controllers issuing requests as long the requests are serialized, but
// it won't work well if requests are issued in parallel.
CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
{
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);

ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...",
ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()));
chip::Dnssd::Resolver::Instance().SetOperationalDelegate(mDelegate);
return chip::Dnssd::Resolver::Instance().ResolveNodeId(peerId);
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId)
{
return chip::Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId);
}

CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
{
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
Expand Down
7 changes: 0 additions & 7 deletions src/lib/dnssd/Resolver_ImplNone.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@ ResolverProxy::~ResolverProxy()
Shutdown();
}

CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}

void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) {}

CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
Expand Down
Loading

0 comments on commit a9a4259

Please sign in to comment.