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

addrinfo: use default resolver #70

Merged
merged 3 commits into from
Jun 25, 2023
Merged

Conversation

achille-roussel
Copy link
Contributor

@achille-roussel achille-roussel commented Jun 24, 2023

This PR modifies wasirun to use net.DefaultResolver so we can configure the DNS servers for SockAddressInfo and propagate the context for asynchronous cancellation.

if err != nil {
return 0, wasi.ECANCELED // TODO: better errors on name resolution failure
}

addrs := make([]wasi.AddressInfo, 0, 16)
addrs4 := make([]wasi.AddressInfo, 0, 8)
addrs6 := make([]wasi.AddressInfo, 0, 8)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this. Why have two slices? or why not append directly into results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two slices so that we can put all the IPv4 addresses first; this is a cheaper and simpler way to do the partial ordering that was implemented with a stable sort in the previous version.

results is an output parameter, we can't append to the slice, which is why we use copy after collecting the addresses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I forgot about the need to have ipv4 first 👍🏻

@achille-roussel achille-roussel merged commit d1f5b98 into main Jun 25, 2023
@achille-roussel achille-roussel deleted the addrinfo-use-default-resolver branch June 25, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants