Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter received mdns records to chip only. #7527

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

cecille
Copy link
Contributor

@cecille cecille commented Jun 10, 2021

Problem

Discovery was returning all sorts of non-chip devices and this was causing out of memory errors and spam.
Fixes #7327

Change overview

Filters to _chip or _chipc devices as appropriate.

Testing

Tested on M5 with chip-device-ctrl discover -all (also tested by mturon@, who was previously seeing other devices)

@cecille cecille added the TE3 label Jun 10, 2021
@franck-apple franck-apple removed the TE3 label Jun 10, 2021
src/lib/mdns/Resolver_ImplMinimalMdns.cpp Show resolved Hide resolved
@@ -238,11 +223,26 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
}
else if (mDiscoveryType == DiscoveryType::kOperational)
{
OnOperationalSrvRecord(data.GetName(), srv);
// Ensure this is our record.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reword comment? I am unsure if this helps provide more context or information beyond reading the code.

@@ -238,11 +223,26 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
}
else if (mDiscoveryType == DiscoveryType::kOperational)
{
OnOperationalSrvRecord(data.GetName(), srv);
// Ensure this is our record.
if (HasQNamePart(data.GetName(), kOperationalServiceName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous code was checking that the suffix was fully _chip._tcp.local.

updated code seems to check only that "_chip" occurs somewhere in the qname. Is this ok and sufficient? Maybe a comment of "note: this does not fully validate path" may be useful if we at some point decide we really need to fully validate the path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix this too.

@andy31415 andy31415 merged commit b0dbfc7 into project-chip:master Jun 11, 2021
@andy31415
Copy link
Contributor

Merged: comments were nitpicks

@cecille cecille deleted the filter_mdns_records branch July 23, 2021 19:34
}
else
{
mValid = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is marking the whole response as invalid even though the record we're looking for might be later in the packet, causing #9099. In fact this is almost always happening in some test scenarios with thread devices.

@@ -238,11 +223,26 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
}
else if (mDiscoveryType == DiscoveryType::kOperational)
{
OnOperationalSrvRecord(data.GetName(), srv);
// Ensure this is our record.
if (HasQNamePart(data.GetName(), kOperationalServiceName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix this too.

nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python discover command returns Insufficient space
8 participants