Make minmdns use a distinct policy for endpoint and IP choices #22168
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removes the minmdns sever implementation for logic regarding interface selection, use the central header for this.
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton and
jepenven-silabs
pullapprove
bot
requested review from
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yunhanw-google
woody-apple
approved these changes
Aug 25, 2022
|
PR #22168: Size comparison from 80d134e to 591ccaa Increases above 0.2%:
Increases (17 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, linux, mbed, psoc6, telink)
Decreases (2 builds for cc13x2_26x2, cyw30739)
Full report (32 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
…r. Address policy default implementation is optional
|
PR #22168: Size comparison from 3d7cc78 to cc1def8 Increases above 0.2%:
Increases (24 builds for bl602, cc13x2_26x2, cyw30739, linux, mbed, psoc6)
Decreases (8 builds for cc13x2_26x2, psoc6, telink)
Full report (34 builds for bl602, cc13x2_26x2, cyw30739, k32w, linux, mbed, psoc6, telink)
|
|
PR #22168: Size comparison from 3d7cc78 to 2db31ba Increases above 0.2%:
Increases (29 builds for bl602, cyw30739, efr32, esp32, linux, mbed, nrfconnect, psoc6)
Decreases (6 builds for psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
There was a problem hiding this comment.
@andy31415 Should some of this policy infrastructure be shared by the group multicast bits too?
tcarmelveilleux
approved these changes
Aug 27, 2022
Maybe - I do not know what parts multicast needs. I believe it may need to, but I have not heard complains about this, only for minmdns (which may very well mean that groups are not as tested). I would mark it as a potential followup, but expect this to be maybe only past 1.0. |
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…ct-chip#22168) * Use AllInterfacesListenIterator header for minmdns. Removes the minmdns sever implementation for logic regarding interface selection, use the central header for this. * Start adding some policies and a default policy impl * Restyle * Compile in the default minmdns policy * Use policies for IP address iteration * Switch interface iteration for Advertiser_ImplMinimalMdns * Completely remove the use of AllInterfaceListenIterator * Add file to header dependency * make testipresponder initialize memory * Switch address policy to be a virtual base class that can be overrider. Address policy default implementation is optional * Disable ipv4 flag in minmdns example: build flags should control server capabilities * Address review comment: address iterator type filtering happens in the policy * Remove FIXME text in code * Fix TestIPResponder
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reason for 1.0
Problem
MinMDNS attempts to listen and advertise all endpoints and IP addresses on a device.
This is not always appropriate, one example being linux which supports
deprecatedandtemporaryIPv6 addresses, which should generally not be reported/used.Change overview
This is part of preparation for #22046 - either core SDK or app-integration logic should allow for defining distinct MinMDNS policies for endpoint and ip selection.
This splits the endpoint and address selection into a separate library which is compile-time enabled, with the expectation that we can build an alternate policy (e.g. for linux using libnl for more info on ip addresses) or let the app integration decide to link their own library policy.
Testing
Manually checked with minmdns client/server examples.
CI will validate as linux integration tests rely on minmdns.