Repository: valkey-io/valkey
File: src/cluster_legacy.c (C)

Code before:
            explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData);
            explen += sizeof(clusterMsgModule) - 3 + ntohl(hdr_light->data.module.msg.len);
        } else {
            explen = sizeof(clusterMsg) - sizeof(union clusterMsgData);
            explen += sizeof(clusterMsgModule) - 3 + ntohl(hdr->data.module.msg.len);
        }
    } else {
        /* We don't know this type of packet, so we assume it's well formed. */
        explen = totlen;
    }

    if (totlen != explen) {
        serverLog(LL_WARNING, "Received invalid %s packet of length %lld but expected length %lld",
                  clusterGetMessageTypeString(type), (unsigned long long)totlen, (unsigned long long)explen);
        return 0;
    }

    return 1;
}

/* When iterating through the slot bitmap, group every 64 bits as
 * a word to speedup. */
static inline int clusterExtractSlotFromWord(uint64_t *slot_word, size_t slot_word_index) {
    /* Get the index of the least-significant set bit, in this 64-bit word */
    const unsigned bit = (unsigned)__builtin_ctzll(*slot_word);
    const int slot = (int)((slot_word_index << 6) | bit);
    *slot_word &= *slot_word - 1; /* clear that bit */
    return slot;
}

/* When this function is called, there is a packet to process starting
 * at link->rcvbuf. Releasing the buffer is up to the caller, so this
 * function should just handle the higher level stuff of processing the
 * packet, modifying the cluster state if needed.
 *
 * The function returns 1 if the link is still valid after the packet
 * was processed, otherwise 0 if the link was freed since the packet
 * processing lead to some inconsistency error (for instance a PONG
 * received from the wrong sender ID). */
int clusterProcessPacket(clusterLink *link) {
    /* Validate that the packet is well-formed */
    if (!clusterIsValidPacket(link)) {
        clusterMsg *hdr = (clusterMsg *)link->rcvbuf;
        uint16_t type = ntohs(hdr->type);
        if (server.debug_cluster_close_link_on_packet_drop &&
            (type == server.cluster_drop_packet_filter || server.cluster_drop_packet_filter == -2)) {
            freeClusterLink(link);
            serverLog(LL_WARNING, "Closing link for matching packet type %s", clusterGetMessageTypeString(type));
            return 0;
        }
        return 1;

Review comment (refactor):
Can we simplify the types a bit?
* Why cast to `unsigned` here when the result is `int` and we mask onto an `int` below?  Just leave it `int`.
* Why is `slot_word_index` a `size_t`?  Can't we just use `int` for that as well?

Code after:
            clusterMsgLight *hdr_light = (clusterMsgLight *)link->rcvbuf;
            explen = sizeof(clusterMsgLight) - sizeof(union clusterMsgData);
            explen += sizeof(clusterMsgModule) - 3 + ntohl(hdr_light->data.module.msg.len);
        } else {
            explen = sizeof(clusterMsg) - sizeof(union clusterMsgData);
            explen += sizeof(clusterMsgModule) - 3 + ntohl(hdr->data.module.msg.len);
        }
    } else {
        /* We don't know this type of packet, so we assume it's well formed. */
        explen = totlen;
    }

    if (totlen != explen) {
        serverLog(LL_WARNING, "Received invalid %s packet of length %lld but expected length %lld",
                  clusterGetMessageTypeString(type), (unsigned long long)totlen, (unsigned long long)explen);
        return 0;
    }

    return 1;
}

/* When iterating through the slot bitmap, group every 64 bits as
 * a word to speedup. */
static inline int clusterExtractSlotFromWord(uint64_t *slot_word, size_t slot_word_index) {
    /* Get the index of the least-significant set bit, in this 64-bit word */
    const unsigned bit = (unsigned)__builtin_ctzll(*slot_word);
    const int slot = (int)((slot_word_index << 6) | bit);
    *slot_word &= *slot_word - 1; /* clear that bit */
    return slot;
}

/* When this function is called, there is a packet to process starting
 * at link->rcvbuf. Releasing the buffer is up to the caller, so this
 * function should just handle the higher level stuff of processing the
 * packet, modifying the cluster state if needed.
 *
 * The function returns 1 if the link is still valid after the packet
 * was processed, otherwise 0 if the link was freed since the packet
 * processing lead to some inconsistency error (for instance a PONG
 * received from the wrong sender ID). */
int clusterProcessPacket(clusterLink *link) {
    /* Validate that the packet is well-formed */
    if (!clusterIsValidPacket(link)) {
        clusterMsg *hdr = (clusterMsg *)link->rcvbuf;
        uint16_t type = ntohs(hdr->type);
        if (server.debug_cluster_close_link_on_packet_drop &&
            (type == server.cluster_drop_packet_filter || server.cluster_drop_packet_filter == -2)) {
            freeClusterLink(link);
            serverLog(LL_WARNING, "Closing link for matching packet type %s", clusterGetMessageTypeString(type));
            return 0;
        }