Why is there an index parameter? Even if the listener has siblings, I think only the listener->fd should be returned here.

Then the sibling fd's become un-accessible. Given that we create siblings
because a single add_port caused multiple listening sockets to be created
(due to ipv6 dual stack for example), it seems like a general API would
need this.

On Thu, Jan 7, 2016 at 5:06 PM Dan Born notifications@github.com wrote:

In src/core/iomgr/tcp_server.h
#4555 (comment):

@@ -80,4 +80,8 @@ int grpc_tcp_listener_get_port(grpc_tcp_listener *listener);
void grpc_tcp_listener_ref(grpc_tcp_listener *listener);
void grpc_tcp_listener_unref(grpc_tcp_listener *listener);

+/* potentially (trivially) implementable: */
+int grpc_tcp_listener_get_fd(grpc_tcp_listener *listener, unsigned index);

Why is there an index parameter? Even if the listener has siblings, I
think only the listener->fd should be returned here.

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc/pull/4555/files#r49148448.

It seems like what you are trying for is to have the "head" sibling (is_sibling==0) be the exposed API and the others (is_sibling==1) be hidden.

This is OK as long as you pass the index and the head sibling to tcp_server_posix.c:on_read and on_accept_cb.

grpc_tcp_server_get_fd() doesn't index into siblings. I'm not sure that's right.

grpc_tcp_listener has a refcount, but is also a singly-linked list node. What do you do when the refcount of a node in the middle of a singly-linked list drops to zero? This can be OK if the grpc_tcp_listener API never access grpc_tcp_listener::next, because grpc_tcp_server owns refs and is the only API that accesses grpc_tcp_listener::next. I just want to double-check that this is intentionally part of the design.

If grpc_tcp_server_get_fd() can be made to access all the FDs (including siblings), do we need grpc_tcp_listener_get_fd()? on_accept_cb could take the grpc_tcp_server* and an index. I'd also consider moving the refcount into the grpc_tcp_server, so grpc_tcp_listener_{ref,unref}() ref the grpc_tcp_server. For example, in the current scheme, the server can be deleted before the listeners so that grpc_tcp_listener::server points to a deleted object.

It might be worth taking a step back and rethinking some of the design and API goals.

The only use I've seen of grpc_tcp_listener in existing code is to get the port. You could have add_port() return the port, and keep listener completely private. For the new stuff, you'd go with my earlier idea of passing the grpc_tcp_server to on_accept_cb with an index and fixing grpc_tcp_server_get_fd().

That main thing to do is clarify whether grpc_tcp_listener is private or not. It's sort of stuck in between right now, not completely making sense either way.

I could send a pull request showing what this would look like.