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

Ignore unknown address types when looking up hosts #34067

Merged
merged 3 commits into from
Jul 2, 2016

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jun 3, 2016

Previously, any function using a ToSocketAddrs input would fail if
passed a hostname that resolves to an address type different from the
ones recognized by Rust.

This also changes the LookupHost iterator to only include the known
address types, as a result, it doesn't have to return Results any
more, which are likely misinterpreted as failed name lookups.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

}
match result {
Some(r) => Some(r),
None => self.next(),
}
Copy link
Member

Choose a reason for hiding this comment

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

Change the implementation to an iterative one, please. Its a trivially optimisable tail-call which will not get optimised out in debug builds.

@nagisa
Copy link
Member

nagisa commented Jun 4, 2016

Seems to me like we’re losing on the errors reported by getaddrinfo with this change.

@tbu-
Copy link
Contributor Author

tbu- commented Jun 4, 2016

@nagisa No, this is only removing the error type when iterating over the addrinfo structure. The io::Error that happens when calling getaddrinfo is still returned to the caller.

@alexcrichton
Copy link
Member

Why change the iterator? If the real bug here is that unknown addresses cause too many errors then it sounds like we need to explicitly ignore those real addresses. Hiding errors seems... bad?

@tbu-
Copy link
Contributor Author

tbu- commented Jun 5, 2016

We're not hiding an error any function is returning. gethostbyname returns a data structure that can contain addresses of potentially multiple types (I don't know if there exists a case where it can be different from an IPv4 or IPv6 address). This PR changes that we just skip over these unknown addresses in the data structure. Previously, any unknown address would e.g. have caused the ToSocketAddrs implementation to return no addresses at all. I don't know why it would be of interest to the user that the domain resolved to an address Rust can't handle (and cannot handle in the future, because it would be a breaking change to add new variants to the SocketAddr enum).

@alexcrichton
Copy link
Member

I don't know if there exists a case where it can be different from an IPv4 or IPv6 address

Is... there motivation for this PR? Presumably you ran across this somehow?

@tbu-
Copy link
Contributor Author

tbu- commented Jun 7, 2016

The motivation of this is that I don't know what to do with the Results yielded by the lookup_host iterator. It seems there's nothing sensible you can do with them.

The standard library's way to deal with them seems like a bad idea, the ToSocketAddrs returns no addresses if any of the Results is an error. Even the doc example seems to assume that any error is fatal. If anything else can returned other than IPv4/IPv6 addresses, then ignoring them seems like a good way for a function that can only return IP addresses. If nothing else can be returned, then it would be an API violation that the user of the Rust API cannot do anything about, returning a Result in this case is not helpful either.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 27, 2016
@alexcrichton
Copy link
Member

The libs team discussed this during triage yesterday and the conclusion was that this implementation seems fine for now, but the docs need to be bolstered with respect to the behavior here. Can you be sure to update relevant documentation to indicate that unknown address types are ignored?

tbu- added 3 commits June 29, 2016 11:39
Previously, any function using a `ToSocketAddrs` input would fail if
passed a hostname that resolves to an address type different from the
ones recognized by Rust.

This also changes the `LookupHost` iterator to only include the known
address types, as a result, it doesn't have to return `Result`s any
more, which are likely misinterpreted as failed name lookups.
@tbu- tbu- force-pushed the pr_lookup_host_ignore_other_addresses branch from e4fc282 to 6aa0182 Compare June 29, 2016 09:55
@tbu-
Copy link
Contributor Author

tbu- commented Jun 29, 2016

@alexcrichton Updated the documentation.

@alexcrichton
Copy link
Member

@bors: r+ 6aa0182

@bors
Copy link
Contributor

bors commented Jun 30, 2016

⌛ Testing commit 6aa0182 with merge fbafb96...

@bors
Copy link
Contributor

bors commented Jun 30, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@tbu-
Copy link
Contributor Author

tbu- commented Jun 30, 2016

That doesn't look like this PR's fault.

@alexcrichton
Copy link
Member

@bors: retry

On Thu, Jun 30, 2016 at 6:30 AM, tbu- notifications@github.com wrote:

That doesn't look like this PR's fault.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34067 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95Av3zwD6PbxG79GORzPYo63PjqL7ks5qQ8TwgaJpZM4It6XF
.

@bors
Copy link
Contributor

bors commented Jul 1, 2016

⌛ Testing commit 6aa0182 with merge bb88a57...

@bors
Copy link
Contributor

bors commented Jul 1, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@tbu-
Copy link
Contributor Author

tbu- commented Jul 1, 2016

Something is seriously wrong with the bots, but it's not me:

command did not execute successfully: "git" "submodule" "update"
expected success, got: exit code: 1


Makefile:23: recipe for target 'all' failed
>> rustjob: found 0 remaining processes
fatal: Needed a single revision
Unable to find current revision in submodule path 'src/llvm'
make: *** [all] Error 1

@alexcrichton
Copy link
Member

@bors
Copy link
Contributor

bors commented Jul 1, 2016

⌛ Testing commit 6aa0182 with merge 1b514ac...

@bors
Copy link
Contributor

bors commented Jul 1, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Jul 1, 2016 at 12:27 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/1643


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34067 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95PmP71Tnr_XY4-AajfSEcE1oIWXVks5qRWoEgaJpZM4It6XF
.

@bors
Copy link
Contributor

bors commented Jul 2, 2016

⌛ Testing commit 6aa0182 with merge 32a6373...

bors added a commit that referenced this pull request Jul 2, 2016
…alexcrichton

Ignore unknown address types when looking up hosts

Previously, any function using a `ToSocketAddrs` input would fail if
passed a hostname that resolves to an address type different from the
ones recognized by Rust.

This also changes the `LookupHost` iterator to only include the known
address types, as a result, it doesn't have to return `Result`s any
more, which are likely misinterpreted as failed name lookups.
@bors bors merged commit 6aa0182 into rust-lang:master Jul 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants