Skip to content

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