Fix --interface-id and improve minmdns address policy #37360
+247
−113
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
…sier to understand and extend - make AddressPolicy cleaner by following a pattern seen with GetDefaultAdvertiser - explicit SetDefaultAddressPolicy calls are not required - no ifdefs needed to include a particular implementation of a default policy, only gn linking controls this - rename things a bit, DefaultAddressPolicy will now become BuiltinAddressPolicy to better reflect the fact that it is a wrapper for the builtin InterfaceIterator in InetInterface.cpp - provide a SingleInterfaceAddressPolicy which is stacked on top of an existing policy and restricts the interface id - apply the SingleInterfaceAddressPolicy when the --interface-id argument is provided *and* the minmdns dnssd backend is selected Signed-off-by: Maciej Grela <[email protected]>
Signed-off-by: Maciej Grela <[email protected]>
Signed-off-by: Maciej Grela <[email protected]>
Signed-off-by: Maciej Grela <[email protected]>
|
PR #37360: Size comparison from 7912cad to c937b09 Full report (3 builds for cc32xx, stm32)
|
Signed-off-by: Maciej Grela <[email protected]>
enkiusz
force-pushed
the
advertise-interface-selection
branch
from
c937b09 to
05cc5a4
Compare
|
PR #37360: Size comparison from f957643 to 05cc5a4 Increases above 0.2%:
Full report (52 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Signed-off-by: Maciej Grela <[email protected]>
|
PR #37360: Size comparison from f957643 to d81d570 Increases above 0.2%:
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Align CHIP_MDNS_MINIMAL macro handling with best practices. Signed-off-by: Maciej Grela <[email protected]>
andy31415
reviewed
Feb 5, 2025
| @@ -25,9 +25,22 @@ namespace { | |||
| AddressPolicy * gAddressPolicy = nullptr; | |||
| } // namespace | |||
|
|
|||
| // This will be resolved at link time if a default policy is set | |||
| #ifndef CHIP_MINMDNS_NONE_POLICY | |||
| AddressPolicy * GetDefaultAddressPolicy(); | |||
There was a problem hiding this comment.
this should go in some header rather than declared here and then in other per-policy headers in a repeated manner.
We want to ensure a single declaration placement. Probably in AddressPolicy.h . Full documentation on usage should be described there.
andy31415
reviewed
Feb 5, 2025
| namespace mdns { | ||
| namespace Minimal { | ||
|
|
||
| void SetDefaultAddressPolicy(); | ||
| AddressPolicy * GetDefaultAddressPolicy(); |
There was a problem hiding this comment.
This should probably be removed from here. Likely we may not need this header at all.
andy31415
reviewed
Feb 5, 2025
| void SetAddressPolicy(); | ||
|
|
||
| } // namespace LibNl | ||
| AddressPolicy * GetDefaultAddressPolicy(); |
There was a problem hiding this comment.
Same here: this header should probably not exist at all. Declaration of this should be in AddressPolicy.h
andy31415
reviewed
Feb 5, 2025
Comment on lines
+37
to
+52
| if (!mParent->Next(&id, &type)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| while (id != mSelectedIface) | ||
| { | ||
| if (!mParent->Next(&id, &type)) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| *out_id = id; | ||
| *out_type = type; | ||
| return true; |
There was a problem hiding this comment.
Feels like the code could be shorter somehow... did not test, but maybe this works:
Suggested change
| if (!mParent->Next(&id, &type)) | |
| { | |
| return false; | |
| } | |
| while (id != mSelectedIface) | |
| { | |
| if (!mParent->Next(&id, &type)) | |
| { | |
| return false; | |
| } | |
| } | |
| *out_id = id; | |
| *out_type = type; | |
| return true; | |
| while (mParent->Next(&id, &type)) | |
| { | |
| if (id != mSelectedIface) | |
| { | |
| continue; | |
| } | |
| *out_id = id; | |
| *out_type = type; | |
| return true; | |
| } | |
| return false; |
andy31415
reviewed
Feb 5, 2025
| @@ -85,6 +92,11 @@ static_library("server") { | |||
| "${chip_root}/src/transport", | |||
| ] | |||
|
|
|||
| if (chip_mdns_minimal) { | |||
There was a problem hiding this comment.
I think we have mdns as minimal or platform ... so this may pull a mdns dependency even for thread (at least CI link errors seem to indicate that).
|
PR #37360: Size comparison from f957643 to 086bc71 Increases above 0.2%:
Full report (32 builds for cc32xx, esp32, linux, nrfconnect, telink)
|
bzbarsky-apple
requested changes
Feb 6, 2025
| @@ -111,18 +111,16 @@ def wait(self, timeout=None): | |||
| self.cv_stopped.wait() | |||
|
|
|||
| def __startServer(self, runner, command): | |||
| app_cmd = command + ['--interface-id', str(-1)] | |||
There was a problem hiding this comment.
Isn't this going to break running YAML tests on Darwin, by no longer passing that argument?
| @@ -51,9 +51,6 @@ + (NSTask *)doStartAppWithName:(NSString *)name arguments:(NSArray<NSString *> * | |||
| __auto_type * appTask = [self createTaskForPath:executable]; | |||
|
|
|||
| __auto_type * forcedArguments = @[ | |||
| // Make sure we only advertise on the local interface. | |||
There was a problem hiding this comment.
Again, this is going to break the Darwin tests, causing a lot of random failures due to cross-talk between runners. Stopping here.
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.
This PR makes the --interface-id work on linux. Before even when --interface-id was specified the minmdns still advertises on all interfaces. This has been fixed by implementing a new address policy which, when stacked upon the default address policy, filters the available interfaces.
Additionally, some refactoring was performed on the address policy code to make it easier to understand and maintain (I hope). This PR also serves as an RFC for these changes to gather feedback on the refactored address policy approach.
After positive review I will move forward and try to add an automated test for --interface-id as a next step.
Testing
Manual testing on devcontainer so far.