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

Exclude link-local IPv4s from editor host list. #27569

Merged
merged 1 commit into from
May 14, 2019

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Mar 31, 2019

IPv4 has link-local addresses like IPv6 (block 169.254.0.0/16).
Those addresses should not be considered a valid option when selecting the remote_host setting for the debugger.

IPv4 has link-local addresses like IPv6 (block 169.254.0.0/16).
Those addresses should not be considered a valid option when selecting
the `remote_host` setting for the debugger.
@follower
Copy link
Contributor

follower commented Apr 5, 2019

I might be missing something but why are link-local addresses not considered a valid option?

For example, if two Mac devices are connected directly via an Ethernet cable--when a DHCP server is not enabled--eventually they will self-assign link-local addresses which means its possible to create a connection without explicitly configuring anything.

While I've not used this approach with anything Godot-related yet, its still seems like it would a valid option.

@Faless
Copy link
Collaborator Author

Faless commented Apr 5, 2019

they will self-assign link-local addresses which means its possible to create a connection without explicitly configuring anything.

@follower Have you ever tried that out? In my very limited experience with the matter, I was not able to connect to a link-local address (169.254.0.0/16).
A friend of mine was in fact unable to listen/connect to it's own link-local address (on Windows).
We could change that if you can confirm it works.

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Apr 8, 2019
@slapin
Copy link
Contributor

slapin commented Apr 8, 2019

I think this is OS-dependent. Also there might be multicast 224/8 addresses and other addresses which might not be wanted depending on particular system settings. The naive solution might break things, too.
I think currently the best bet would be

  1. Have list of all listen-able addresses.
  2. Have a helper function which filters these and reports them according to RFC. This thing might be already available and also can be implemented in user's application.
  3. It is up to user to decide which addresses can be used or can't be used for listenning (UI option).
    The user should be able to do final decision on what addresses to use. Godot can't handle all existing broken setups everywhere. But also there is no reason to prevent it from working on non-standard setups which work for other applications. However Godot can supply the information (list of addresses) and reasonable defaults.

@Faless
Copy link
Collaborator Author

Faless commented Apr 8, 2019

Also there might be multicast 224/8 addresses and other addresses which might not be wanted depending on particular system settings.

@slapin it wouldn't make sense to list multicast addresses, as TCP can't work with multicast, so this is certainly not a problem (and multicast addresses, are already excluded).

2\. Have a helper function which filters these and reports them according to RFC. This thing might be already available and also can be implemented in user's application.

This is exactly what I'm trying to do here, this PR do not change the result from get_local_addreses, it just filter the value in editor settings.

3\. The user should be able to do final decision on what addresses to use. Godot can't handle all existing broken setups everywhere.

I don't think having a text edit there is a good idea. It would just be confusing to many people, and prone to errors. All for very exceptional cases (i.e. using editor-debugger connection on a broken network setup).

Command line options are not affected,
I think it's more important to have an 'listen for debugger' button instead, which you can use for any weird setup (and lunch your game app accordingly via CLI). And that yes, can have a line dit (pre-filled with last/default value, probably just a wildcard).
But this is up for debate of course :)

@mhilbrunner
Copy link
Member

mhilbrunner commented May 1, 2019

I agree with @Faless - having a simple button that works in 99% of cases is preferable as long as CLI options exist.

Note that we currently also exclude IPv6 link local adresses. IPv4 link local adresses in comparison are IMO both uncommon and nonstandard.

We can discuss adding UI options for more control over this, but as this is just for the Editor debugger, and only does for IPv4 (where again, link local addresses are esoteric anyhow) what is already done for IPv6, this is fine to merge by me.

Further discussion on more elaborate handling should IMO be done in a PR for that or an issue.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Fine to merge by me, as stated above :)

@slapin
Copy link
Contributor

slapin commented May 1, 2019 via email

@mhilbrunner mhilbrunner merged commit bdf79f4 into godotengine:master May 14, 2019
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.

5 participants