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

Add support for getaddrinfo #278

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Conversation

bikallem
Copy link
Contributor

Fixes part of #258

@bikallem
Copy link
Contributor Author

@talex5 do you know if the ocaml-ci has ipv6 enabled? or how to enable it? The test for this PR seems to be failing as I expect Ipv6 address but the machine is not returning any.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks reasonable to me.

I think we should call this getaddrinfo rather than addrinfo though, as that's the name everyone uses for it (including OCaml's Unix module). Also, it may access the network so the getting part is quite important.

do you know if the ocaml-ci has ipv6 enabled? or how to enable it? The test for this PR seems to be failing as I expect Ipv6 address but the machine is not returning any.

What gets returned for localhost will depend on the system configuration, so it's probably best to test it using numeric addresses.

lib_eio/net.mli Outdated Show resolved Hide resolved
lib_eio_luv/eio_luv.ml Outdated Show resolved Hide resolved
tests/network.md Outdated Show resolved Hide resolved
tests/network.md Outdated Show resolved Hide resolved
@bikallem
Copy link
Contributor Author

@talex5 I have renamed addrinfo to getaddrinfo as suggested and removed the luv specific test from network.md.

@SquidDev
Copy link
Contributor

SquidDev commented Aug 11, 2022

One thing possibly worth mentioning is that users may wish to override this method to point to a non-libc implementation (especially on Alpine, where musl's getaddrinfo implementation is intentionally limited).

Supporting this use case isn't a problem (capabilities make this very easy to do), but I do feel a bit weird about calling this function getaddrinfo when it no longer uses the POSIX function - I'd personally prefer something like resolve_addr.

@bikallem
Copy link
Contributor Author

@SquidDev Thanks for the feedback. Don't really have any personal preference but I agree with @talex5 - getaddrinfo is the more common term for what we are doing here since both sets of backend (luv and unix) both name it as such.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Either getaddrinfo or resolve_addr is fine with me. I don't think there's any problem with getaddrinfo calling an alternative implementation as long as it does roughly the same thing. Otherwise we'd have to rename everything else too (e.g. unlink and mkdir might not be calling the glibc function either, if it's a remote directory, or a directory inside a zip archive).

lib_eio/mock/net.ml Outdated Show resolved Hide resolved
lib_eio/mock/net.ml Outdated Show resolved Hide resolved
lib_eio/net.mli Outdated Show resolved Hide resolved
@bikallem bikallem force-pushed the getaddrinfo branch 3 times, most recently from e510251 to caf4ee9 Compare August 15, 2022 08:06
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I added one test for port numbers and squash-rebased it on main.

@talex5 talex5 merged commit db4156d into ocaml-multicore:main Aug 15, 2022
@bikallem bikallem deleted the getaddrinfo branch August 15, 2022 08:44
@talex5 talex5 mentioned this pull request Aug 17, 2022
talex5 added a commit to talex5/opam-repository that referenced this pull request Aug 26, 2022
CHANGES:

New features:

- Add `Eio.Condition` (@TheLortex @talex5 ocaml-multicore/eio#277).
  Allows a fiber to wait for some condition to become true.

- Add `Eio.Net.getaddrinfo` and `getnameinfo` (@bikallem @talex5 ocaml-multicore/eio#278 ocaml-multicore/eio#288 ocaml-multicore/eio#291).
  Convert between host names and addresses.

- Add `Eio.Debug` (@talex5 ocaml-multicore/eio#276).
  Currently, this allows overriding the `traceln` function.

- `Buf_write.create`: make switch optional (@talex5 ocaml-multicore/eio#283).
  This makes things easier for people porting code from Faraday.

Bug fixes:

- Allow sharing of libuv poll handles (@patricoferris @talex5 ocaml-multicore/eio#279).
  Luv doesn't allow two callers to watch the same file handle, so we need to handle that in Eio.

Other changes:

- Upgrade to uring 0.4 (@talex5 ocaml-multicore/eio#290).

- Mention `Mutex`, `Semaphore` and `Condition` in the README (@talex5 ocaml-multicore/eio#281).
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.

None yet

4 participants