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

Closed
wants to merge 2 commits into from
Closed

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

@talex5
Copy link
Contributor

talex5 commented Nov 25, 2024

Since #284 was merged, this is mostly working now. For example:

$ dune exec -- ./test-bin/calc.exe serve \
    --capnp-secret-key-file /tmp/key \
    --capnp-listen-address tcp:ip6-localhost:7000
+Waiting for incoming connections at:
+capnp://sha-256:Et007wZtip7kit6MMunAcwrf5zIxsVR6t4K_KmpFQ8w@ip6-localhost:7000

It would be good to be able to parse numerical IPv6 addresses, as in 'tcp:[::1]:7000', though. I think it does need the brackets; using the rev trick you can ask for tcp:::1:7000 as a listen address, but it prints it back with the brackets and only accepts URLs with the brackets when connecting.

@talex5
Copy link
Contributor

talex5 commented Nov 26, 2024

Fixed in #296. I found Ipaddr.with_port_of_string does what we need.

Thanks!

@talex5 talex5 closed this Nov 26, 2024
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