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

Support ipv6 hosts #279

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support ipv6 hosts #279

wants to merge 2 commits into from

Conversation

BChabanne
Copy link

The PR following #278

This is adding support for ipv6 in capnp-rpc-unix.

This has been tested with ocurrent rpc on the server and the client.

  1. Unix.getaddrinfo is used instead of Unix.gethostbyname as discussed in the issue
  2. Unix.socket now needs to dynamically chose the socket_domain based on the address
  3. parse_tcp needs to be able to parse ipv6 address.

For 3. I don’t think this is the best way to do : the format of ip6 host would be tcp:::1:7000 which is not so readable.
It is probably better to do tcp:[::1]:7000 but this require more change than simply adding ~rev:true.

Tell me what do you think of it

Copy link
Contributor

@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.

Seems reasonable. Note that I'm hoping to resurrect #256, which will change all this.

match Unix.getaddrinfo host "" [Unix.AI_SOCKTYPE Unix.SOCK_STREAM] with
| {ai_addr = ADDR_INET(addr, _) ; _} :: _ -> addr
| [] -> Capnp_rpc.Debug.failf "No addresses found for host name %S" host
| _ -> Capnp_rpc.Debug.failf "Unknown host %S" host
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit odd to say "unknown host" where there are multiple matches.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed this branch was only taken for Unix sockets.
I updated the match to make it explicit.
However, the message is maybe not so relevant anymore. I kept it as it was before, but maybe it deserves to be reworded a bit

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