code review from Ping, small diff and better code

Signed-off-by: Binbin <[email protected]>

src/cluster_legacy.c

@@ -1676,12 +1676,8 @@ int clusterCountNonFailingReplicas(clusterNode *n) {
     return ok_replicas;
 }
 
-/* Low level cleanup of the node structure.
- *
- * When delete = 1 is passed, it is called by clusterDelNode and we will delete
- * the node from the cluster nodes dict. When delete = 0 is passed, we only free
- * the node resources because the node is not added to the cluster nodes dict. */
-void freeClusterNode(clusterNode *n, int delete) {
+/* Low level cleanup of the node structure. Only called by clusterDelNode(). */
+void freeClusterNode(clusterNode *n) {
     sds nodename;
     int j;
 
@@ -1693,11 +1689,9 @@ void freeClusterNode(clusterNode *n, int delete) {
     if (nodeIsReplica(n) && n->replicaof) clusterNodeRemoveReplica(n->replicaof, n);
 
     /* Unlink from the set of nodes. */
-    if (delete) {
-        nodename = sdsnewlen(n->name, CLUSTER_NAMELEN);
-        serverAssert(dictDelete(server.cluster->nodes, nodename) == DICT_OK);
-        sdsfree(nodename);
-    }
+    nodename = sdsnewlen(n->name, CLUSTER_NAMELEN);
+    serverAssert(dictDelete(server.cluster->nodes, nodename) == DICT_OK);
+    sdsfree(nodename);
     sdsfree(n->hostname);
     sdsfree(n->human_nodename);
     sdsfree(n->announce_client_ipv4);
@@ -1760,7 +1754,7 @@ void clusterDelNode(clusterNode *delnode) {
     clusterRemoveNodeFromShard(delnode);
 
     /* 4) Free the node, unlinking it from the cluster. */
-    freeClusterNode(delnode, 1);
+    freeClusterNode(delnode);
 }
 
 /* Node lookup by name */
@@ -3251,6 +3245,15 @@ int clusterProcessPacket(clusterLink *link) {
         if (type == CLUSTERMSG_TYPE_MEET) {
             if (!sender) {
                 if (!link->node) {
+                    char ip[NET_IP_STR_LEN] = {0};
+                    if (nodeIp2String(ip, link, hdr->myip) != C_OK) {
+                        /* Unable to retrieve the node's IP address from the connection. Without a
+                         * valid IP, the node becomes unusable in the cluster. This failure might be
+                         * due to the connection being closed. */
+                        freeClusterLink(link);
+                        return 0;
+                    }
+
                     /* Add this node if it is new for us and the msg type is MEET.
                      * In this stage we don't try to add the node with the right
                      * flags, replicaof pointer, and so forth, as this details will be
@@ -3259,17 +3262,7 @@ int clusterProcessPacket(clusterLink *link) {
                      * we want to send extensions right away in the return PONG in order
                      * to reduce the amount of time needed to stabilize the shard ID. */
                     clusterNode *node = createClusterNode(NULL, CLUSTER_NODE_HANDSHAKE);
-                    if (nodeIp2String(node->ip, link, hdr->myip) != C_OK) {
-                        /* Unable to retrieve the node's IP address from the connection. Without a
-                         * valid IP, the node becomes unusable in the cluster. This failure might be
-                         * due to the connection being closed. To avoid leaving the cluster in an
-                         * inconsistent state, we close the link and free the node. */
-                        serverLog(LL_NOTICE, "Closing link and freeing node due to failure to retrieve IP "
-                                             "from the connection, possibly caused by a closed connection.");
-                        freeClusterLink(link);
-                        freeClusterNode(node, 0);
-                        return 0;
-                    }
+                    memcpy(node->ip, ip, sizeof(ip));
                     getClientPortFromClusterMsg(hdr, &node->tls_port, &node->tcp_port);
                     node->cport = ntohs(hdr->cport);
                     if (hdr->mflags[0] & CLUSTERMSG_FLAG0_EXT_DATA) {