From 570d1f44255cd918f9fc5a6f93d21b63b9f63d96 Mon Sep 17 00:00:00 2001 From: ctiller Date: Mon, 12 Jan 2015 16:29:52 -0800 Subject: [PATCH] Return bound port number from grpc_server_add_http2_port. Allows tests to bind to port 0 and still have clients connect to them. Change on 2015/01/12 by ctiller ------------- Created by MOE: http://code.google.com/p/moe-java MOE_MIGRATED_REVID=83800669 --- include/grpc/grpc.h | 3 +- src/core/iomgr/sockaddr_utils.c | 28 ++++++++ src/core/iomgr/sockaddr_utils.h | 6 ++ src/core/iomgr/tcp_server.h | 5 +- src/core/iomgr/tcp_server_posix.c | 87 ++++++++++++++++------- src/core/surface/server_chttp2.c | 16 +++-- test/core/end2end/dualstack_socket_test.c | 70 ++++++++++-------- 7 files changed, 156 insertions(+), 59 deletions(-) diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index 323a944f93f..085393084d2 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -428,7 +428,8 @@ grpc_call_error grpc_server_request_call(grpc_server *server, void *tag_new); grpc_server *grpc_server_create(grpc_completion_queue *cq, const grpc_channel_args *args); -/* Add a http2 over tcp listener; returns 1 on success, 0 on failure +/* Add a http2 over tcp listener. + Returns bound port number on success, 0 on failure. REQUIRES: server not started */ int grpc_server_add_http2_port(grpc_server *server, const char *addr); diff --git a/src/core/iomgr/sockaddr_utils.c b/src/core/iomgr/sockaddr_utils.c index f709d35162b..eca14a4f398 100644 --- a/src/core/iomgr/sockaddr_utils.c +++ b/src/core/iomgr/sockaddr_utils.c @@ -153,3 +153,31 @@ int grpc_sockaddr_to_string(char **out, const struct sockaddr *addr, errno = save_errno; return ret; } + +int grpc_sockaddr_get_port(const struct sockaddr *addr) { + switch (addr->sa_family) { + case AF_INET: + return ntohs(((struct sockaddr_in *)addr)->sin_port); + case AF_INET6: + return ntohs(((struct sockaddr_in6 *)addr)->sin6_port); + default: + gpr_log(GPR_ERROR, "Unknown socket family %d in %s", addr->sa_family, + __FUNCTION__); + return 0; + } +} + +int grpc_sockaddr_set_port(const struct sockaddr *addr, int port) { + switch (addr->sa_family) { + case AF_INET: + ((struct sockaddr_in *)addr)->sin_port = htons(port); + return 1; + case AF_INET6: + ((struct sockaddr_in6 *)addr)->sin6_port = htons(port); + return 1; + default: + gpr_log(GPR_ERROR, "Unknown socket family %d in %s", addr->sa_family, + __FUNCTION__); + return 0; + } +} diff --git a/src/core/iomgr/sockaddr_utils.h b/src/core/iomgr/sockaddr_utils.h index 753d0c824a4..3f5b770e865 100644 --- a/src/core/iomgr/sockaddr_utils.h +++ b/src/core/iomgr/sockaddr_utils.h @@ -57,6 +57,12 @@ int grpc_sockaddr_is_wildcard(const struct sockaddr *addr, int *port_out); void grpc_sockaddr_make_wildcards(int port, struct sockaddr_in *wild4_out, struct sockaddr_in6 *wild6_out); +/* Return the IP port number of a sockaddr */ +int grpc_sockaddr_get_port(const struct sockaddr *addr); + +/* Set IP port number of a sockaddr */ +int grpc_sockaddr_set_port(const struct sockaddr *addr, int port); + /* Converts a sockaddr into a newly-allocated human-readable string. Currently, only the AF_INET and AF_INET6 families are recognized. diff --git a/src/core/iomgr/tcp_server.h b/src/core/iomgr/tcp_server.h index 1968246b751..d881e146b96 100644 --- a/src/core/iomgr/tcp_server.h +++ b/src/core/iomgr/tcp_server.h @@ -52,7 +52,8 @@ grpc_tcp_server *grpc_tcp_server_create(); void grpc_tcp_server_start(grpc_tcp_server *server, grpc_pollset *pollset, grpc_tcp_server_cb cb, void *cb_arg); -/* Add a port to the server, returning true on success, or false otherwise. +/* Add a port to the server, returning port number on success, or negative + on failure. The :: and 0.0.0.0 wildcard addresses are treated identically, accepting both IPv4 and IPv6 connections, but :: is the preferred style. This usually @@ -60,6 +61,8 @@ void grpc_tcp_server_start(grpc_tcp_server *server, grpc_pollset *pollset, but not dualstack sockets. For raw access to the underlying sockets, see grpc_tcp_server_get_fd(). */ +/* TODO(ctiller): deprecate this, and make grpc_tcp_server_add_ports to handle + all of the multiple socket port matching logic in one place */ int grpc_tcp_server_add_port(grpc_tcp_server *s, const struct sockaddr *addr, int addr_len); diff --git a/src/core/iomgr/tcp_server_posix.c b/src/core/iomgr/tcp_server_posix.c index 5ed517748a3..753e24c38e5 100644 --- a/src/core/iomgr/tcp_server_posix.c +++ b/src/core/iomgr/tcp_server_posix.c @@ -154,6 +154,9 @@ static int get_max_accept_queue_size() { /* Prepare a recently-created socket for listening. */ static int prepare_socket(int fd, const struct sockaddr *addr, int addr_len) { + struct sockaddr_storage sockname_temp; + socklen_t sockname_len; + if (fd < 0) { goto error; } @@ -179,13 +182,18 @@ static int prepare_socket(int fd, const struct sockaddr *addr, int addr_len) { goto error; } - return 1; + sockname_len = sizeof(sockname_temp); + if (getsockname(fd, (struct sockaddr *)&sockname_temp, &sockname_len) < 0) { + goto error; + } + + return grpc_sockaddr_get_port((struct sockaddr *)&sockname_temp); error: if (fd >= 0) { close(fd); } - return 0; + return -1; } /* event manager callback when reads are ready */ @@ -234,39 +242,64 @@ error: static int add_socket_to_server(grpc_tcp_server *s, int fd, const struct sockaddr *addr, int addr_len) { server_port *sp; + int port; - if (!prepare_socket(fd, addr, addr_len)) { - return 0; - } - - gpr_mu_lock(&s->mu); - GPR_ASSERT(!s->cb && "must add ports before starting server"); - /* append it to the list under a lock */ - if (s->nports == s->port_capacity) { - s->port_capacity *= 2; - s->ports = gpr_realloc(s->ports, sizeof(server_port *) * s->port_capacity); + port = prepare_socket(fd, addr, addr_len); + if (port >= 0) { + gpr_mu_lock(&s->mu); + GPR_ASSERT(!s->cb && "must add ports before starting server"); + /* append it to the list under a lock */ + if (s->nports == s->port_capacity) { + s->port_capacity *= 2; + s->ports = + gpr_realloc(s->ports, sizeof(server_port *) * s->port_capacity); + } + sp = &s->ports[s->nports++]; + sp->server = s; + sp->fd = fd; + sp->emfd = grpc_fd_create(fd); + GPR_ASSERT(sp->emfd); + gpr_mu_unlock(&s->mu); } - sp = &s->ports[s->nports++]; - sp->server = s; - sp->fd = fd; - sp->emfd = grpc_fd_create(fd); - GPR_ASSERT(sp->emfd); - gpr_mu_unlock(&s->mu); - return 1; + return port; } int grpc_tcp_server_add_port(grpc_tcp_server *s, const struct sockaddr *addr, int addr_len) { - int ok = 0; + int allocated_port1 = -1; + int allocated_port2 = -1; + int i; int fd; grpc_dualstack_mode dsmode; struct sockaddr_in6 addr6_v4mapped; struct sockaddr_in wild4; struct sockaddr_in6 wild6; struct sockaddr_in addr4_copy; + struct sockaddr *allocated_addr = NULL; + struct sockaddr_storage sockname_temp; + socklen_t sockname_len; int port; + /* Check if this is a wildcard port, and if so, try to keep the port the same + as some previously created listener. */ + if (grpc_sockaddr_get_port(addr) == 0) { + for (i = 0; i < s->nports; i++) { + sockname_len = sizeof(sockname_temp); + if (0 == getsockname(s->ports[i].fd, (struct sockaddr *)&sockname_temp, + &sockname_len)) { + port = grpc_sockaddr_get_port((struct sockaddr *)&sockname_temp); + if (port > 0) { + allocated_addr = malloc(addr_len); + memcpy(allocated_addr, addr, addr_len); + grpc_sockaddr_set_port(allocated_addr, port); + addr = allocated_addr; + break; + } + } + } + } + if (grpc_sockaddr_to_v4mapped(addr, &addr6_v4mapped)) { addr = (const struct sockaddr *)&addr6_v4mapped; addr_len = sizeof(addr6_v4mapped); @@ -280,12 +313,15 @@ int grpc_tcp_server_add_port(grpc_tcp_server *s, const struct sockaddr *addr, addr = (struct sockaddr *)&wild6; addr_len = sizeof(wild6); fd = grpc_create_dualstack_socket(addr, SOCK_STREAM, 0, &dsmode); - ok |= add_socket_to_server(s, fd, addr, addr_len); + allocated_port1 = add_socket_to_server(s, fd, addr, addr_len); if (fd >= 0 && dsmode == GRPC_DSMODE_DUALSTACK) { - return ok; + goto done; } /* If we didn't get a dualstack socket, also listen on 0.0.0.0. */ + if (port == 0 && allocated_port1 > 0) { + grpc_sockaddr_set_port((struct sockaddr *)&wild4, allocated_port1); + } addr = (struct sockaddr *)&wild4; addr_len = sizeof(wild4); } @@ -299,8 +335,11 @@ int grpc_tcp_server_add_port(grpc_tcp_server *s, const struct sockaddr *addr, addr = (struct sockaddr *)&addr4_copy; addr_len = sizeof(addr4_copy); } - ok |= add_socket_to_server(s, fd, addr, addr_len); - return ok; + allocated_port2 = add_socket_to_server(s, fd, addr, addr_len); + +done: + gpr_free(allocated_addr); + return allocated_port1 >= 0 ? allocated_port1 : allocated_port2; } int grpc_tcp_server_get_fd(grpc_tcp_server *s, int index) { diff --git a/src/core/surface/server_chttp2.c b/src/core/surface/server_chttp2.c index a0961bd4493..47fca827f38 100644 --- a/src/core/surface/server_chttp2.c +++ b/src/core/surface/server_chttp2.c @@ -76,6 +76,8 @@ int grpc_server_add_http2_port(grpc_server *server, const char *addr) { grpc_tcp_server *tcp = NULL; size_t i; int count = 0; + int port_num = -1; + int port_temp; resolved = grpc_blocking_resolve_address(addr, "http"); if (!resolved) { @@ -88,9 +90,15 @@ int grpc_server_add_http2_port(grpc_server *server, const char *addr) { } for (i = 0; i < resolved->naddrs; i++) { - if (grpc_tcp_server_add_port(tcp, - (struct sockaddr *)&resolved->addrs[i].addr, - resolved->addrs[i].len)) { + port_temp = grpc_tcp_server_add_port( + tcp, (struct sockaddr *)&resolved->addrs[i].addr, + resolved->addrs[i].len); + if (port_temp >= 0) { + if (port_num == -1) { + port_num = port_temp; + } else { + GPR_ASSERT(port_num == port_temp); + } count++; } } @@ -108,7 +116,7 @@ int grpc_server_add_http2_port(grpc_server *server, const char *addr) { /* Register with the server only upon success */ grpc_server_add_listener(server, tcp, start, destroy); - return 1; + return port_num; /* Error path: cleanup and return */ error: diff --git a/test/core/end2end/dualstack_socket_test.c b/test/core/end2end/dualstack_socket_test.c index b443caa2a67..9837300f67a 100644 --- a/test/core/end2end/dualstack_socket_test.c +++ b/test/core/end2end/dualstack_socket_test.c @@ -73,26 +73,35 @@ void test_connect(const char *server_host, const char *client_host, int port, cq_verifier *v_client; cq_verifier *v_server; gpr_timespec deadline; + int got_port; - gpr_join_host_port(&client_hostport, client_host, port); gpr_join_host_port(&server_hostport, server_host, port); - gpr_log(GPR_INFO, "Testing with server=%s client=%s (expecting %s)", - server_hostport, client_hostport, expect_ok ? "success" : "failure"); /* Create server. */ server_cq = grpc_completion_queue_create(); server = grpc_server_create(server_cq, NULL); - GPR_ASSERT(grpc_server_add_http2_port(server, server_hostport)); + GPR_ASSERT((got_port = grpc_server_add_http2_port(server, server_hostport)) > + 0); + if (port == 0) { + port = got_port; + } else { + GPR_ASSERT(port == got_port); + } grpc_server_start(server); - gpr_free(server_hostport); v_server = cq_verifier_create(server_cq); /* Create client. */ + gpr_join_host_port(&client_hostport, client_host, port); client_cq = grpc_completion_queue_create(); client = grpc_channel_create(client_hostport, NULL); - gpr_free(client_hostport); v_client = cq_verifier_create(client_cq); + gpr_log(GPR_INFO, "Testing with server=%s client=%s (expecting %s)", + server_hostport, client_hostport, expect_ok ? "success" : "failure"); + + gpr_free(client_hostport); + gpr_free(server_hostport); + if (expect_ok) { /* Normal deadline, shouldn't be reached. */ deadline = ms_from_now(60000); @@ -170,8 +179,7 @@ void test_connect(const char *server_host, const char *client_host, int port, int main(int argc, char **argv) { int do_ipv6 = 1; - int i; - int port = grpc_pick_unused_port_or_die(); + int fixed_port; grpc_test_init(argc, argv); grpc_init(); @@ -181,30 +189,34 @@ int main(int argc, char **argv) { do_ipv6 = 0; } - for (i = 0; i <= 1; i++) { + for (fixed_port = 0; fixed_port <= 1; fixed_port++) { + int port = fixed_port ? grpc_pick_unused_port_or_die() : 0; + /* For coverage, test with and without dualstack sockets. */ - grpc_forbid_dualstack_sockets_for_testing = i; - - /* :: and 0.0.0.0 are handled identically. */ - test_connect("::", "127.0.0.1", port, 1); - test_connect("::", "::ffff:127.0.0.1", port, 1); - test_connect("::", "localhost", port, 1); - test_connect("0.0.0.0", "127.0.0.1", port, 1); - test_connect("0.0.0.0", "::ffff:127.0.0.1", port, 1); - test_connect("0.0.0.0", "localhost", port, 1); - if (do_ipv6) { - test_connect("::", "::1", port, 1); - test_connect("0.0.0.0", "::1", port, 1); - } + for (grpc_forbid_dualstack_sockets_for_testing = 0; + grpc_forbid_dualstack_sockets_for_testing <= 1; + grpc_forbid_dualstack_sockets_for_testing++) { + /* :: and 0.0.0.0 are handled identically. */ + test_connect("::", "127.0.0.1", port, 1); + test_connect("::", "::ffff:127.0.0.1", port, 1); + test_connect("::", "localhost", port, 1); + test_connect("0.0.0.0", "127.0.0.1", port, 1); + test_connect("0.0.0.0", "::ffff:127.0.0.1", port, 1); + test_connect("0.0.0.0", "localhost", port, 1); + if (do_ipv6) { + test_connect("::", "::1", port, 1); + test_connect("0.0.0.0", "::1", port, 1); + } + + /* These only work when the families agree. */ + test_connect("127.0.0.1", "127.0.0.1", port, 1); + if (do_ipv6) { + test_connect("::1", "::1", port, 1); + test_connect("::1", "127.0.0.1", port, 0); + test_connect("127.0.0.1", "::1", port, 0); + } - /* These only work when the families agree. */ - test_connect("127.0.0.1", "127.0.0.1", port, 1); - if (do_ipv6) { - test_connect("::1", "::1", port, 1); - test_connect("::1", "127.0.0.1", port, 0); - test_connect("127.0.0.1", "::1", port, 0); } - } grpc_shutdown();