Skip to content

Add support for getaddrinfo#278

Merged
talex5 merged 1 commit intoocaml-multicore:mainfrom
bikallem:getaddrinfo
Aug 15, 2022
Merged

Add support for getaddrinfo#278
talex5 merged 1 commit intoocaml-multicore:mainfrom
bikallem:getaddrinfo

Conversation

@bikallem
Copy link
Copy Markdown
Contributor

Fixes part of #258

@bikallem
Copy link
Copy Markdown
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
Copy Markdown
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.

@bikallem
Copy link
Copy Markdown
Contributor Author

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

@SquidDev
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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).

Copy link
Copy Markdown
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.

4 participants