Ignore unknown address types when looking up hosts by tbu- · Pull Request #34067 · rust-lang/rust

tbu- · 2016-06-03T21:11:44Z

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 · 2016-06-03T21:11:48Z

r? @aturon

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

nagisa · 2016-06-04T15:17:15Z

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

nagisa · 2016-06-04T15:22:25Z

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

tbu- · 2016-06-04T18:35:14Z

@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 · 2016-06-05T06:29:21Z

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- · 2016-06-05T09:21:19Z

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 · 2016-06-07T07:59:05Z

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- · 2016-06-07T08:13:50Z

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 · 2016-06-28T17:13:50Z

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?

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- · 2016-06-29T09:56:36Z

@alexcrichton Updated the documentation.

alexcrichton · 2016-06-29T21:16:17Z

@bors: r+ 6aa0182

bors · 2016-06-30T10:49:46Z

⌛ Testing commit 6aa0182 with merge fbafb96...

bors · 2016-06-30T12:53:15Z

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

tbu- · 2016-06-30T13:30:18Z

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

alexcrichton · 2016-06-30T16:18:18Z

@bors: retry

rustbuild: Clean out tmp directory on make clean #34577

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 · 2016-07-01T12:21:03Z

⌛ Testing commit 6aa0182 with merge bb88a57...

bors · 2016-07-01T12:25:18Z

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

tbu- · 2016-07-01T13:01:48Z

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 · 2016-07-01T15:57:28Z

@bors: retry

Cascading failure on bots when interrupted when performing git operations #34595

bors · 2016-07-01T19:21:58Z

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

bors · 2016-07-01T19:26:57Z

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

alexcrichton · 2016-07-01T19:50:42Z

@bors: retry

Cascading failure on bots when interrupted when performing git operations #34595

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 · 2016-07-02T01:43:29Z

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

…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 · 2016-07-02T04:39:14Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

rust-highfive assigned aturon Jun 3, 2016

nagisa reviewed Jun 4, 2016
View reviewed changes

alexcrichton added the T-libs-api [DEPRECATED; DO NOT USE] label Jun 27, 2016

tbu- added 3 commits June 29, 2016 11:39

Convert a simple tail call to a loop
a03a82e

Update documentation to reflect ignoring of unknown addresses
6aa0182

tbu-force-pushed the pr_lookup_host_ignore_other_addresses branch from e4fc282 to 6aa0182Compare June 29, 2016 09:55

bors merged commit 6aa0182 into rust-lang:masterJul 2, 2016

Uh oh!

Uh oh!

Conversation

tbu- commented Jun 3, 2016

Uh oh!

rust-highfive commented Jun 3, 2016

Uh oh!

nagisaJun 4, 2016

Choose a reason for hiding this comment

Uh oh!

nagisa commented Jun 4, 2016

Uh oh!

tbu- commented Jun 4, 2016

Uh oh!

alexcrichton commented Jun 5, 2016

Uh oh!

tbu- commented Jun 5, 2016

Uh oh!

alexcrichton commented Jun 7, 2016

Uh oh!

tbu- commented Jun 7, 2016

Uh oh!

alexcrichton commented Jun 28, 2016

Uh oh!

tbu- commented Jun 29, 2016

Uh oh!

alexcrichton commented Jun 29, 2016

Uh oh!

bors commented Jun 30, 2016

Uh oh!

bors commented Jun 30, 2016

Uh oh!

tbu- commented Jun 30, 2016

Uh oh!

alexcrichton commented Jun 30, 2016

Uh oh!

bors commented Jul 1, 2016

Uh oh!

bors commented Jul 1, 2016

Uh oh!

tbu- commented Jul 1, 2016

Uh oh!

alexcrichton commented Jul 1, 2016

Uh oh!

bors commented Jul 1, 2016

Uh oh!

bors commented Jul 1, 2016

Uh oh!

alexcrichton commented Jul 1, 2016

Uh oh!

bors commented Jul 2, 2016

Uh oh!

bors commented Jul 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants