add linkClient(): adds the client and caches the list node.

We have this operation in two places: when caching the master and
when linking a new client after the client creation. By having an API
for this we avoid incurring in errors when modifying one of the two
places forgetting the other. The function is also a good place where to
document why we cache the linked list node.

Related to #4497 and #4210.

3 files changed, 14 insertions, 8 deletions

diff --git a/src/networking.c b/src/networking.c
index 0dfc0767d..82c763c95 100644
--- a/src/networking.c
+++ b/src/networking.c
@@ -67,6 +67,16 @@ int listMatchObjects(void *a, void *b) {
     return equalStringObjects(a,b);
 }
 
+/* This function links the client to the global linked list of clients.
+ * unlinkClient() does the opposite, among other things. */
+void linkClient(client *c) {
+    listAddNodeTail(server.clients,c);
+    /* Note that we remember the linked list node where the client is stored,
+     * this way removing the client in unlinkClient() will not require
+     * a linear scan, but just a constant time operation. */
+    c->client_list_node = listLast(server.clients);
+}
+
 client *createClient(int fd) {
     client *c = zmalloc(sizeof(client));
 
@@ -133,14 +143,10 @@ client *createClient(int fd) {
     c->pubsub_channels = dictCreate(&objectKeyPointerValueDictType,NULL);
     c->pubsub_patterns = listCreate();
     c->peerid = NULL;
+    c->client_list_node = NULL;
     listSetFreeMethod(c->pubsub_patterns,decrRefCountVoid);
     listSetMatchMethod(c->pubsub_patterns,listMatchObjects);
-    if (fd != -1) {
-        listAddNodeTail(server.clients,c);
-        c->client_list_node = listLast(server.clients);
-    } else {
-        c->client_list_node = NULL;
-    }
+    if (fd != -1) linkClient(c);
     initClientMultiState(c);
     return c;
 }
diff --git a/src/replication.c b/src/replication.c
index 773d6e704..17a8bb357 100644
--- a/src/replication.c
+++ b/src/replication.c
@@ -2214,8 +2214,7 @@ void replicationResurrectCachedMaster(int newfd) {
     server.repl_down_since = 0;
 
     /* Re-add to the list of clients. */
-    listAddNodeTail(server.clients,server.master);
-    server.master->client_list_node = listLast(server.clients);
+    linkClient(server.master);
     if (aeCreateFileEvent(server.el, newfd, AE_READABLE,
                           readQueryFromClient, server.master)) {
         serverLog(LL_WARNING,"Error resurrecting the cached master, impossible to add the readable handler: %s", strerror(errno));
diff --git a/src/server.h b/src/server.h
index 2100d7071..7b0d22eaf 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1394,6 +1394,7 @@ int handleClientsWithPendingWrites(void);
 int clientHasPendingReplies(client *c);
 void unlinkClient(client *c);
 int writeToClient(int fd, client *c, int handler_installed);
+void linkClient(client *c);
 
 #ifdef __GNUC__
 void addReplyErrorFormat(client *c, const char *fmt, ...)