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

Accept host names in announced addresses #48

Closed
wants to merge 1 commit into from

Conversation

majedev
Copy link
Contributor

@majedev majedev commented May 27, 2016

Fixes #46. Now correctly maps xxx://:port and xxx://0.0.0.0:port to xxx://<client remote address>:port

@@ -329,10 +329,12 @@ func (s *querysrv) handleAnnounce(ctx context.Context, remote net.IP, deviceID p
}

ip := net.ParseIP(host)
if len(ip) == 0 || ip.IsUnspecified() {
uri.Host = net.JoinHostPort(remote.String(), port)
if host == "" || ip.IsUnspecified() {
Copy link
Member

Choose a reason for hiding this comment

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

Nil ip is not unspecified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Judging by go lang source, ip.IsUnspecified returns false for nil ip

Copy link
Member

Choose a reason for hiding this comment

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

Yes, hence why the len(ip)==0 was there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But net.ParseIP(host) returns nil for FQDN's as well, becuase they are not IP's. The if condition is specific for empty host names xxx://:port and unspecified IP addresses xxx://0.0.0.0:port. Any other value for the host part will be kept as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I think about it, tcp6://:port and tcp6://[::]:port addresses might not be mapped correctly to IPv6 client address with this implementation..

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, it should be fine. I'll leave for @calmh to merge, looks good to me.

@calmh
Copy link
Member

calmh commented May 27, 2016

@st-review merge it

@st-review
Copy link

👌 Merged as 60c7dee. Thanks, @majedev!

st-review pushed a commit that referenced this pull request May 27, 2016
@st-review st-review closed this May 27, 2016
calmh pushed a commit to syncthing/syncthing that referenced this pull request Jun 2, 2016
@syncthing syncthing locked and limited conversation to collaborators Jan 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants