[Esp32] mdns filter to handle MDNS traffic for esp32 #27961
Conversation
pullapprove
bot
requested review from
amitnj,
andy31415,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple and
kcoppock
pullapprove
bot
requested review from
xylophone21,
younghak-hwang and
yufengwangca
| namespace mdns { | ||
| namespace Minimal { |
There was a problem hiding this comment.
This is an odd namespace to put this in....
| // Non-mDNS is not filtered | ||
| VerifyOrReturnValue(pktInfo.DestPort == kMdnsPort, FilterOutcome::kAllowPacket); | ||
|
|
||
| if (pktInfo.DestAddress.IsIPv4() && pktInfo.DestPort == kMdnsPort) |
There was a problem hiding this comment.
Why is this bothering about checking pktInfo.DestPort again?
There was a problem hiding this comment.
Added change which will drop all IPv4 Mdns packets and allow IPv6 Mdns packets
| { | ||
| if (!mStopLogging) | ||
| { | ||
| ChipLogError(Discovery, "Started dropping mDNS from storm."); |
There was a problem hiding this comment.
This case has nothing to do with "storm", This is going to log as soon as we get an ipv4 packet, then stop logging usefully....
|
|
||
| protected: | ||
| std::atomic_size_t mNumQueuedMdnsPackets = { 0 }; | ||
| size_t mMaxQueuedPackets = 0; |
There was a problem hiding this comment.
| size_t mMaxQueuedPackets = 0; | |
| const size_t mMaxQueuedPackets = 0; |
so it's clear why a non-atomic thing is ok here.
| { | ||
| if (mStopLogging) | ||
| { | ||
| mStopLogging = false; |
There was a problem hiding this comment.
If we are really worried about the before/after functions being called at the same time on different threads, this logic around mStopLogging is broken. By the time we get here we might have already gone back to a state where mNumQueuedMdnsPackets >= mMaxQueuedPackets, no?
There was a problem hiding this comment.
Added a change which would not break
examples/all-clusters-minimal-app/esp32/main/DeviceWithDisplay.cpp
Outdated
Show resolved
Hide resolved
PSONALl
force-pushed
the
esp32-mdns-filter
branch
3 times, most recently
from
5876126
to
c21db8d
Compare
|
@PSONALl Please ping me on Slack when you make the updates per above. |
pullapprove
bot
requested review from
andyg-apple,
aronrosenberg,
tcarmelveilleux and
yunhanw-google
PSONALl
force-pushed
the
esp32-mdns-filter
branch
from
c21db8d
to
8969858
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
PSONALl seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Testing