From 01a9118d0c29e76df08581c2978d7f82e2b3e508 Mon Sep 17 00:00:00 2001 From: Pawel Wodkowski Date: Thu, 7 Jun 2018 22:21:03 +0200 Subject: [PATCH] jsonrpc: fix closed connection hadling The spdk_jsonrpc_server_conn_remove() was just swapping last connection with that is being removed. This was fine but not for BSD queues which rely on its own address. Simple fix would be to add STAILQ_INIT and STAILQ_SWAP for queued requests but we can hit the same nasty and easy to overlook bug in future. Instead convert connection array to linked list and move around only pointers. Fixes #322 Change-Id: Ibb359d281f6164bcd17df37ba9d31ffdb46c2e0a Signed-off-by: Pawel Wodkowski Reviewed-on: https://review.gerrithub.io/414257 Tested-by: SPDK Automated Test System Reviewed-by: Dariusz Stojaczyk Reviewed-by: Daniel Verkamp Reviewed-by: Ben Walker --- lib/jsonrpc/jsonrpc_internal.h | 9 ++++-- lib/jsonrpc/jsonrpc_server_tcp.c | 47 ++++++++++++++++---------------- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/lib/jsonrpc/jsonrpc_internal.h b/lib/jsonrpc/jsonrpc_internal.h index b0ea21e29..2caaba2e7 100644 --- a/lib/jsonrpc/jsonrpc_internal.h +++ b/lib/jsonrpc/jsonrpc_internal.h @@ -80,13 +80,18 @@ struct spdk_jsonrpc_server_conn { STAILQ_HEAD(, spdk_jsonrpc_request) send_queue; struct spdk_jsonrpc_request *send_request; + + TAILQ_ENTRY(spdk_jsonrpc_server_conn) link; }; struct spdk_jsonrpc_server { int sockfd; spdk_jsonrpc_handle_request_fn handle_request; - struct spdk_jsonrpc_server_conn conns[SPDK_JSONRPC_MAX_CONNS]; - int num_conns; + + TAILQ_HEAD(, spdk_jsonrpc_server_conn) free_conns; + TAILQ_HEAD(, spdk_jsonrpc_server_conn) conns; + + struct spdk_jsonrpc_server_conn conns_array[SPDK_JSONRPC_MAX_CONNS]; }; /* jsonrpc_server_tcp */ diff --git a/lib/jsonrpc/jsonrpc_server_tcp.c b/lib/jsonrpc/jsonrpc_server_tcp.c index 64f92f6b6..4c65dc7ca 100644 --- a/lib/jsonrpc/jsonrpc_server_tcp.c +++ b/lib/jsonrpc/jsonrpc_server_tcp.c @@ -40,13 +40,20 @@ spdk_jsonrpc_server_listen(int domain, int protocol, spdk_jsonrpc_handle_request_fn handle_request) { struct spdk_jsonrpc_server *server; - int rc, val, flag; + int rc, val, flag, i; server = calloc(1, sizeof(struct spdk_jsonrpc_server)); if (server == NULL) { return NULL; } + TAILQ_INIT(&server->free_conns); + TAILQ_INIT(&server->conns); + + for (i = 0; i < SPDK_JSONRPC_MAX_CONNS; i++) { + TAILQ_INSERT_TAIL(&server->free_conns, &server->conns_array[i], link); + } + server->handle_request = handle_request; server->sockfd = socket(domain, SOCK_STREAM, protocol); @@ -93,12 +100,12 @@ spdk_jsonrpc_server_listen(int domain, int protocol, void spdk_jsonrpc_server_shutdown(struct spdk_jsonrpc_server *server) { - int i; + struct spdk_jsonrpc_server_conn *conn; close(server->sockfd); - for (i = 0; i < server->num_conns; i++) { - close(server->conns[i].sockfd); + TAILQ_FOREACH(conn, &server->conns, link) { + close(conn->sockfd); } free(server); @@ -119,29 +126,27 @@ static void spdk_jsonrpc_server_conn_remove(struct spdk_jsonrpc_server_conn *conn) { struct spdk_jsonrpc_server *server = conn->server; - int conn_idx = conn - server->conns; spdk_jsonrpc_server_conn_close(conn); pthread_spin_destroy(&conn->queue_lock); assert(STAILQ_EMPTY(&conn->send_queue)); - /* Swap conn with the last entry in conns */ - server->conns[conn_idx] = server->conns[server->num_conns - 1]; - server->num_conns--; + TAILQ_REMOVE(&server->conns, conn, link); + TAILQ_INSERT_HEAD(&server->free_conns, conn, link); } static int spdk_jsonrpc_server_accept(struct spdk_jsonrpc_server *server) { struct spdk_jsonrpc_server_conn *conn; - int rc, conn_idx, flag; + int rc, flag; rc = accept(server->sockfd, NULL, NULL); if (rc >= 0) { - assert(server->num_conns < SPDK_JSONRPC_MAX_CONNS); - conn_idx = server->num_conns; - conn = &server->conns[conn_idx]; + conn = TAILQ_FIRST(&server->free_conns); + assert(conn != NULL); + conn->server = server; conn->sockfd = rc; conn->closed = false; @@ -159,8 +164,8 @@ spdk_jsonrpc_server_accept(struct spdk_jsonrpc_server *server) return -1; } - server->num_conns++; - + TAILQ_REMOVE(&server->free_conns, conn, link); + TAILQ_INSERT_TAIL(&server->conns, conn, link); return 0; } @@ -332,12 +337,10 @@ more: int spdk_jsonrpc_server_poll(struct spdk_jsonrpc_server *server) { - int rc, i; - struct spdk_jsonrpc_server_conn *conn; - - for (i = 0; i < server->num_conns; i++) { - conn = &server->conns[i]; + int rc; + struct spdk_jsonrpc_server_conn *conn, *conn_tmp; + TAILQ_FOREACH_SAFE(conn, &server->conns, link, conn_tmp) { if (conn->closed) { struct spdk_jsonrpc_request *request; @@ -365,13 +368,11 @@ spdk_jsonrpc_server_poll(struct spdk_jsonrpc_server *server) } /* Check listen socket */ - if (server->num_conns < SPDK_JSONRPC_MAX_CONNS) { + if (!TAILQ_EMPTY(&server->free_conns)) { spdk_jsonrpc_server_accept(server); } - for (i = 0; i < server->num_conns; i++) { - conn = &server->conns[i]; - + TAILQ_FOREACH(conn, &server->conns, link) { if (conn->closed) { continue; }