Remove redundant process bind + fix tautological dns: test assertion - #48
Merged
Conversation
- TunnelService.connect() no longer duplicates bindProcessToNetwork(): it now happens once, early in connectWifi(), before MTU detection runs (so probes actually reach the SOCKS host). The post-establish() call was fully redundant and left inconsistent log tags behind. - SocksTunnelTest's dns: block test previously asserted against DNS_ADDRESS (198.18.0.2, the unrelated mapdns constant) instead of the dns: block's actual value (socksHost). It passed for the wrong reason. Now uses a host distinct from DNS_ADDRESS so the assertion can only pass if the dns: block's address is actually correct.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Follow-up cleanup after the two [Grok] commits (5f298ca, d3b2fdd):
TunnelService.kt —
connect()had a duplicatebindProcessToNetwork()call left over after the early bind was added inconnectWifi(). The early bind (before MTU detection) is the one that matters; this removes the redundant post-establish()bind and its stale log tag, keeping onlysetUnderlyingNetworks()there since that still needs to run post-establish for the VPN network association.SocksTunnelTest.kt — the new
buildConfig contains dns block for DNS resolutiontest assertedconfig.contains("address: ${$}{SocksTunnel.DNS_ADDRESS}"), i.e. checking for198.18.0.2— the mapdns constant, not thedns:block's actual address (socksHost). It passed coincidentally because that string already appears elsewhere in the config. Now uses a host distinct fromDNS_ADDRESSso the assertion can only pass if thedns:block is actually correct.