From 09b5825b264883c088dd09753efc1de487d03976 Mon Sep 17 00:00:00 2001 From: skyfirelee <739609084@qq.com> Date: Mon, 10 Jun 2024 02:49:05 +0800 Subject: [PATCH 1/9] Moving client->authenticated to a flag instead of an int (#592) Moving client->authenticated to a flag Fix #589 Signed-off-by: artikell <739609084@qq.com> --- src/acl.c | 4 ++-- src/module.c | 6 +++--- src/networking.c | 12 ++++++++---- src/replication.c | 5 ++--- src/server.h | 2 +- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/acl.c b/src/acl.c index 0c3ccb7f6d..667974211d 100644 --- a/src/acl.c +++ b/src/acl.c @@ -506,7 +506,7 @@ void ACLFreeUserAndKillClients(user *u) { * more defensive to set the default user and put * it in non authenticated mode. */ c->user = DefaultUser; - c->authenticated = 0; + c->flags &= ~CLIENT_AUTHENTICATED; /* We will write replies to this client later, so we can't * close it directly even if async. */ if (c == server.current_client) { @@ -1494,7 +1494,7 @@ void addAuthErrReply(client *c, robj *err) { * The return value is AUTH_OK on success (valid username / password pair) & AUTH_ERR otherwise. */ int checkPasswordBasedAuth(client *c, robj *username, robj *password) { if (ACLCheckUserCredentials(username, password) == C_OK) { - c->authenticated = 1; + c->flags |= CLIENT_AUTHENTICATED; c->user = ACLGetUserByName(username->ptr, sdslen(username->ptr)); moduleNotifyUserChanged(c); return AUTH_OK; diff --git a/src/module.c b/src/module.c index e7416c7926..f149443c80 100644 --- a/src/module.c +++ b/src/module.c @@ -7917,7 +7917,7 @@ int checkModuleAuthentication(client *c, robj *username, robj *password, robj ** } if (c->flags & CLIENT_MODULE_AUTH_HAS_RESULT) { c->flags &= ~CLIENT_MODULE_AUTH_HAS_RESULT; - if (c->authenticated) return AUTH_OK; + if (c->flags & CLIENT_AUTHENTICATED) return AUTH_OK; } return AUTH_ERR; } @@ -9465,7 +9465,7 @@ void revokeClientAuthentication(client *c) { moduleNotifyUserChanged(c); c->user = DefaultUser; - c->authenticated = 0; + c->flags &= ~CLIENT_AUTHENTICATED; /* We will write replies to this client later, so we can't close it * directly even if async. */ if (c == server.current_client) { @@ -9787,7 +9787,7 @@ static int authenticateClientWithUser(ValkeyModuleCtx *ctx, moduleNotifyUserChanged(ctx->client); ctx->client->user = user; - ctx->client->authenticated = 1; + ctx->client->flags |= CLIENT_AUTHENTICATED; if (clientHasModuleAuthInProgress(ctx->client)) { ctx->client->flags |= CLIENT_MODULE_AUTH_HAS_RESULT; diff --git a/src/networking.c b/src/networking.c index 78f36b8170..2bbe012050 100644 --- a/src/networking.c +++ b/src/networking.c @@ -104,14 +104,18 @@ static void clientSetDefaultAuth(client *c) { /* If the default user does not require authentication, the user is * directly authenticated. */ c->user = DefaultUser; - c->authenticated = (c->user->flags & USER_FLAG_NOPASS) && !(c->user->flags & USER_FLAG_DISABLED); + if ((c->user->flags & USER_FLAG_NOPASS) && !(c->user->flags & USER_FLAG_DISABLED)) { + c->flags |= CLIENT_AUTHENTICATED; + } else { + c->flags &= ~CLIENT_AUTHENTICATED; + } } int authRequired(client *c) { /* Check if the user is authenticated. This check is skipped in case * the default user is flagged as "nopass" and is active. */ - int auth_required = - (!(DefaultUser->flags & USER_FLAG_NOPASS) || (DefaultUser->flags & USER_FLAG_DISABLED)) && !c->authenticated; + int auth_required = (!(DefaultUser->flags & USER_FLAG_NOPASS) || (DefaultUser->flags & USER_FLAG_DISABLED)) && + !(c->flags & CLIENT_AUTHENTICATED); return auth_required; } @@ -3642,7 +3646,7 @@ void helloCommand(client *c) { } /* At this point we need to be authenticated to continue. */ - if (!c->authenticated) { + if (!(c->flags & CLIENT_AUTHENTICATED)) { addReplyError(c, "-NOAUTH HELLO must be called with the client already " "authenticated, otherwise the HELLO AUTH " "option can be used to authenticate the client and " diff --git a/src/replication.c b/src/replication.c index dad6a6d0a2..2c47136228 100644 --- a/src/replication.c +++ b/src/replication.c @@ -1721,12 +1721,11 @@ void replicationCreateMasterClient(connection *conn, int dbid) { * to pass the execution to a background thread and unblock after the * execution is done. This is the reason why we allow blocking the replication * connection. */ - server.primary->flags |= CLIENT_PRIMARY; + server.primary->flags |= (CLIENT_PRIMARY | CLIENT_AUTHENTICATED); /* Allocate a private query buffer for the primary client instead of using the shared query buffer. * This is done because the primary's query buffer data needs to be preserved for my sub-replicas to use. */ server.primary->querybuf = sdsempty(); - server.primary->authenticated = 1; server.primary->reploff = server.primary_initial_offset; server.primary->read_reploff = server.primary->reploff; server.primary->user = NULL; /* This client can do everything. */ @@ -3306,7 +3305,7 @@ void replicationResurrectCachedMaster(connection *conn) { server.primary->conn = conn; connSetPrivateData(server.primary->conn, server.primary); server.primary->flags &= ~(CLIENT_CLOSE_AFTER_REPLY | CLIENT_CLOSE_ASAP); - server.primary->authenticated = 1; + server.primary->flags |= CLIENT_AUTHENTICATED; server.primary->last_interaction = server.unixtime; server.repl_state = REPL_STATE_CONNECTED; server.repl_down_since = 0; diff --git a/src/server.h b/src/server.h index a9819a4df8..bf16a1893d 100644 --- a/src/server.h +++ b/src/server.h @@ -427,6 +427,7 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; #define CLIENT_MODULE_PREVENT_REPL_PROP (1ULL << 49) /* Module client do not want to propagate to replica */ #define CLIENT_REPROCESSING_COMMAND (1ULL << 50) /* The client is re-processing the command. */ #define CLIENT_REPLICATION_DONE (1ULL << 51) /* Indicate that replication has been done on the client */ +#define CLIENT_AUTHENTICATED (1ULL << 52) /* Indicate a client has successfully authenticated */ /* Client block type (btype field in client structure) * if CLIENT_BLOCKED flag is set. */ @@ -1238,7 +1239,6 @@ typedef struct client { dictEntry *cur_script; /* Cached pointer to the dictEntry of the script being executed. */ time_t last_interaction; /* Time of the last interaction, used for timeout */ time_t obuf_soft_limit_reached_time; - int authenticated; /* Needed when the default user requires auth. */ int repl_state; /* Replication state if this is a replica. */ int repl_start_cmd_stream_on_ack; /* Install replica write handler on first ACK. */ int repldbfd; /* Replication DB file descriptor. */ From 71dd85dc5a89a4e07d837efa14937990b01e317f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Neal=20Gompa=20=28=E3=83=8B=E3=83=BC=E3=83=AB=E3=83=BB?= =?UTF-8?q?=E3=82=B4=E3=83=B3=E3=83=91=29?= Date: Sun, 9 Jun 2024 18:09:08 -0400 Subject: [PATCH 2/9] src/Makefile: Link libatomic on POWER systems (#607) This ensures that fallbacks for unsupported atomic operations are available for POWER systems. Signed-off-by: Neal Gompa --- src/Makefile | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Makefile b/src/Makefile index 6defebed8d..302ad06b84 100644 --- a/src/Makefile +++ b/src/Makefile @@ -150,6 +150,11 @@ DEBUG=-g -ggdb # Linux ARM32 needs -latomic at linking time ifneq (,$(findstring armv,$(uname_M))) FINAL_LIBS+=-latomic +else +# Linux POWER needs -latomic at linking time +ifneq (,$(findstring ppc,$(uname_M))) + FINAL_LIBS+=-latomic +endif endif ifeq ($(uname_S),SunOS) From 95a753b18d6923d2141fcb01f04ea03966ba10c5 Mon Sep 17 00:00:00 2001 From: Wen Hui Date: Mon, 10 Jun 2024 13:14:37 -0400 Subject: [PATCH 3/9] Add BSD license explicitly (#620) Add "BSD 3-Clause License" in License 1 and License 2 part --------- Signed-off-by: hwware Co-authored-by: Madelyn Olson --- COPYING | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/COPYING b/COPYING index 10928babb3..2058f57e56 100644 --- a/COPYING +++ b/COPYING @@ -1,5 +1,7 @@ # License 1 +BSD 3-Clause License + Copyright (c) 2024-present, Valkey contributors All rights reserved. @@ -13,6 +15,8 @@ THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND # License 2 +BSD 3-Clause License + Copyright (c) 2006-2020, Salvatore Sanfilippo All rights reserved. From a3f1535b5709bba940f62078b6ce4e9c96ab4f87 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Mon, 10 Jun 2024 12:30:57 -0700 Subject: [PATCH 4/9] Fix misuse of safe iterators (#612) Safe iterators must call resetIterators when they are done being used. Fix one issue where a safe iterator was not correctly calling reset during cluster slot caching and fixed a second issue where reset iterator was being called twice. For the double release case, kvstoreIteratorNextDict is responsible for patching up the iterator, but we were calling it a second time in kvstoreIteratorNext. In addition, I added some documentation around initializing iterators, added an assert to prevent double initialization, and remove a function from the public interface which isn't needed and might lead to incorrect usage of the safe iterators. Bumping srgsanky for finding it here: https://github.com/valkey-io/valkey/commit/c4782066e76ba20848fab95fd4909507cc069c3b#r142867004. --------- Signed-off-by: Madelyn Olson --- src/cluster_legacy.c | 4 ++-- src/dict.c | 9 +++++++-- src/kvstore.c | 11 +++-------- src/kvstore.h | 1 - 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/cluster_legacy.c b/src/cluster_legacy.c index ef78f29434..cd3786fe05 100644 --- a/src/cluster_legacy.c +++ b/src/cluster_legacy.c @@ -4888,7 +4888,7 @@ void bitmapClearBit(unsigned char *bitmap, int pos) { * MIGRATE_TO flag the when a primary gets the first slot. */ int clusterPrimariesHaveReplicas(void) { dictIterator di; - dictInitSafeIterator(&di, server.cluster->nodes); + dictInitIterator(&di, server.cluster->nodes); dictEntry *de; int replicas = 0; while ((de = dictNext(&di)) != NULL) { @@ -6509,7 +6509,7 @@ void clusterPromoteSelfToPrimary(void) { int detectAndUpdateCachedNodeHealth(void) { dictIterator di; - dictInitSafeIterator(&di, server.cluster->nodes); + dictInitIterator(&di, server.cluster->nodes); dictEntry *de; clusterNode *node; int overall_health_changed = 0; diff --git a/src/dict.c b/src/dict.c index dc227dbee9..bc92d49564 100644 --- a/src/dict.c +++ b/src/dict.c @@ -943,6 +943,8 @@ unsigned long long dictFingerprint(dict *d) { return hash; } +/* Initiaize a normal iterator. This function should be called when initializing + * an iterator on the stack. */ void dictInitIterator(dictIterator *iter, dict *d) { iter->d = d; iter->table = 0; @@ -952,6 +954,8 @@ void dictInitIterator(dictIterator *iter, dict *d) { iter->nextEntry = NULL; } +/* Initialize a safe iterator, which is allowed to modify the dictionary while iterating. + * You must call dictResetIterator when you are done with a safe iterator. */ void dictInitSafeIterator(dictIterator *iter, dict *d) { dictInitIterator(iter, d); iter->safe = 1; @@ -959,9 +963,10 @@ void dictInitSafeIterator(dictIterator *iter, dict *d) { void dictResetIterator(dictIterator *iter) { if (!(iter->index == -1 && iter->table == 0)) { - if (iter->safe) + if (iter->safe) { dictResumeRehashing(iter->d); - else + assert(iter->d->pauserehash >= 0); + } else assert(iter->fingerprint == dictFingerprint(iter->d)); } } diff --git a/src/kvstore.c b/src/kvstore.c index 8de74c724b..a43b72e1e1 100644 --- a/src/kvstore.c +++ b/src/kvstore.c @@ -48,6 +48,8 @@ #define UNUSED(V) ((void)V) +static dict *kvstoreIteratorNextDict(kvstoreIterator *kvs_it); + struct _kvstore { int flags; dictType dtype; @@ -572,7 +574,7 @@ void kvstoreIteratorRelease(kvstoreIterator *kvs_it) { } /* Returns next dictionary from the iterator, or NULL if iteration is complete. */ -dict *kvstoreIteratorNextDict(kvstoreIterator *kvs_it) { +static dict *kvstoreIteratorNextDict(kvstoreIterator *kvs_it) { if (kvs_it->next_didx == -1) return NULL; /* The dict may be deleted during the iteration process, so here need to check for NULL. */ @@ -600,13 +602,6 @@ dictEntry *kvstoreIteratorNext(kvstoreIterator *kvs_it) { if (!de) { /* No current dict or reached the end of the dictionary. */ dict *d = kvstoreIteratorNextDict(kvs_it); if (!d) return NULL; - if (kvs_it->di.d) { - /* Before we move to the next dict, reset the iter of the previous dict. */ - dictIterator *iter = &kvs_it->di; - dictResetIterator(iter); - /* In the safe iterator context, we may delete entries. */ - freeDictIfNeeded(kvs_it->kvs, kvs_it->didx); - } dictInitSafeIterator(&kvs_it->di, d); de = dictNext(&kvs_it->di); } diff --git a/src/kvstore.h b/src/kvstore.h index d3c5949d1f..e7e21f8aa9 100644 --- a/src/kvstore.h +++ b/src/kvstore.h @@ -40,7 +40,6 @@ uint64_t kvstoreGetHash(kvstore *kvs, const void *key); /* kvstore iterator specific functions */ kvstoreIterator *kvstoreIteratorInit(kvstore *kvs); void kvstoreIteratorRelease(kvstoreIterator *kvs_it); -dict *kvstoreIteratorNextDict(kvstoreIterator *kvs_it); int kvstoreIteratorGetCurrentDictIndex(kvstoreIterator *kvs_it); dictEntry *kvstoreIteratorNext(kvstoreIterator *kvs_it); From e65b2d235c300bb86cc7f960883ad919f75162e6 Mon Sep 17 00:00:00 2001 From: Shivshankar Date: Mon, 10 Jun 2024 16:24:04 -0400 Subject: [PATCH 5/9] Update rewriteConfigSaveOption function code to rewrite multiple save in one line. (#583) Currently, "config rewrite" writes some default value in the config file incase of empty config file specified. But it adds multiple "save" config entries as follows: ``` save 3600 1 save 300 100 save 60 10000 ``` After the fix the save will look like: ``` save 3600 1 300 100 60 10000 ``` --------- Signed-off-by: Shivshankar-Reddy --- src/config.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/config.c b/src/config.c index 13b66f00b5..a609a8f18d 100644 --- a/src/config.c +++ b/src/config.c @@ -1367,11 +1367,11 @@ void rewriteConfigSaveOption(standardConfig *config, const char *name, struct re if (!server.saveparamslen) { rewriteConfigRewriteLine(state, name, sdsnew("save \"\""), 1); } else { + line = sdsnew(name); for (j = 0; j < server.saveparamslen; j++) { - line = sdscatprintf(sdsempty(), "save %ld %d", (long)server.saveparams[j].seconds, - server.saveparams[j].changes); - rewriteConfigRewriteLine(state, name, line, 1); + line = sdscatprintf(line, " %ld %d", (long)server.saveparams[j].seconds, server.saveparams[j].changes); } + rewriteConfigRewriteLine(state, name, line, 1); } /* Mark "save" as processed in case server.saveparamslen is zero. */ From 4bb7cc471a0a4e0948737c99c36aaf7202f4cb4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Wed, 12 Jun 2024 03:52:18 -0700 Subject: [PATCH 6/9] Remove unnecessary clang-format off annotations (#628) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We added some clang-format off comments before we had decided on the format configuration. Now, it turns out that turning formatting off is often not necessary. --------- Signed-off-by: Viktor Söderqvist --- src/acl.c | 36 +++++++++++++----------------------- src/aof.c | 42 +++++++++++++++++++----------------------- src/eval.c | 24 +++++++++++------------- src/listpack.c | 10 +++------- src/networking.c | 8 ++------ src/notify.c | 36 ++++++++++++++++-------------------- src/object.c | 12 +++--------- src/replication.c | 4 +--- src/resp_parser.c | 5 ++--- src/sds.c | 22 +++++++++++++--------- src/sentinel.c | 40 +++++++++++++++++----------------------- src/server.c | 4 +--- src/util.c | 2 -- src/valkey-benchmark.c | 4 +--- src/valkey-cli.c | 4 +--- src/ziplist.c | 6 ++---- 16 files changed, 105 insertions(+), 154 deletions(-) diff --git a/src/acl.c b/src/acl.c index 667974211d..533782acad 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1587,12 +1587,10 @@ static int ACLSelectorCheckKey(aclSelector *selector, const char *key, int keyle listRewind(selector->patterns, &li); int key_flags = 0; - /* clang-format off */ if (keyspec_flags & CMD_KEY_ACCESS) key_flags |= ACL_READ_PERMISSION; if (keyspec_flags & CMD_KEY_INSERT) key_flags |= ACL_WRITE_PERMISSION; if (keyspec_flags & CMD_KEY_DELETE) key_flags |= ACL_WRITE_PERMISSION; if (keyspec_flags & CMD_KEY_UPDATE) key_flags |= ACL_WRITE_PERMISSION; - /* clang-format on */ /* Test this key against every pattern. */ while ((ln = listNext(&li))) { @@ -1618,12 +1616,10 @@ static int ACLSelectorHasUnrestrictedKeyAccess(aclSelector *selector, int flags) listRewind(selector->patterns, &li); int access_flags = 0; - /* clang-format off */ if (flags & CMD_KEY_ACCESS) access_flags |= ACL_READ_PERMISSION; if (flags & CMD_KEY_INSERT) access_flags |= ACL_WRITE_PERMISSION; if (flags & CMD_KEY_DELETE) access_flags |= ACL_WRITE_PERMISSION; if (flags & CMD_KEY_UPDATE) access_flags |= ACL_WRITE_PERMISSION; - /* clang-format on */ /* Test this key against every pattern. */ while ((ln = listNext(&li))) { @@ -2669,15 +2665,13 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username if (object) { le->object = object; } else { - /* clang-format off */ - switch(reason) { + switch (reason) { case ACL_DENIED_CMD: le->object = sdsdup(c->cmd->fullname); break; case ACL_DENIED_KEY: le->object = sdsdup(c->argv[argpos]->ptr); break; case ACL_DENIED_CHANNEL: le->object = sdsdup(c->argv[argpos]->ptr); break; case ACL_DENIED_AUTH: le->object = sdsdup(c->argv[0]->ptr); break; default: le->object = sdsempty(); } - /* clang-format on */ } /* if we have a real client from the network, use it (could be missing on module timers) */ @@ -3058,28 +3052,24 @@ void aclCommand(client *c) { addReplyBulkCString(c, "reason"); char *reasonstr; - /* clang-format off */ - switch(le->reason) { - case ACL_DENIED_CMD: reasonstr="command"; break; - case ACL_DENIED_KEY: reasonstr="key"; break; - case ACL_DENIED_CHANNEL: reasonstr="channel"; break; - case ACL_DENIED_AUTH: reasonstr="auth"; break; - default: reasonstr="unknown"; + switch (le->reason) { + case ACL_DENIED_CMD: reasonstr = "command"; break; + case ACL_DENIED_KEY: reasonstr = "key"; break; + case ACL_DENIED_CHANNEL: reasonstr = "channel"; break; + case ACL_DENIED_AUTH: reasonstr = "auth"; break; + default: reasonstr = "unknown"; } - /* clang-format on */ addReplyBulkCString(c, reasonstr); addReplyBulkCString(c, "context"); char *ctxstr; - /* clang-format off */ - switch(le->context) { - case ACL_LOG_CTX_TOPLEVEL: ctxstr="toplevel"; break; - case ACL_LOG_CTX_MULTI: ctxstr="multi"; break; - case ACL_LOG_CTX_LUA: ctxstr="lua"; break; - case ACL_LOG_CTX_MODULE: ctxstr="module"; break; - default: ctxstr="unknown"; + switch (le->context) { + case ACL_LOG_CTX_TOPLEVEL: ctxstr = "toplevel"; break; + case ACL_LOG_CTX_MULTI: ctxstr = "multi"; break; + case ACL_LOG_CTX_LUA: ctxstr = "lua"; break; + case ACL_LOG_CTX_MODULE: ctxstr = "module"; break; + default: ctxstr = "unknown"; } - /* clang-format on */ addReplyBulkCString(c, ctxstr); addReplyBulkCString(c, "object"); diff --git a/src/aof.c b/src/aof.c index a88b28d827..ac9ffd5fcb 100644 --- a/src/aof.c +++ b/src/aof.c @@ -1994,21 +1994,19 @@ int rioWriteStreamPendingEntry(rio *r, RETRYCOUNT JUSTID FORCE. */ streamID id; streamDecodeID(rawid, &id); - /* clang-format off */ - if (rioWriteBulkCount(r,'*',12) == 0) return 0; - if (rioWriteBulkString(r,"XCLAIM",6) == 0) return 0; - if (rioWriteBulkObject(r,key) == 0) return 0; - if (rioWriteBulkString(r,groupname,groupname_len) == 0) return 0; - if (rioWriteBulkString(r,consumer->name,sdslen(consumer->name)) == 0) return 0; - if (rioWriteBulkString(r,"0",1) == 0) return 0; - if (rioWriteBulkStreamID(r,&id) == 0) return 0; - if (rioWriteBulkString(r,"TIME",4) == 0) return 0; - if (rioWriteBulkLongLong(r,nack->delivery_time) == 0) return 0; - if (rioWriteBulkString(r,"RETRYCOUNT",10) == 0) return 0; - if (rioWriteBulkLongLong(r,nack->delivery_count) == 0) return 0; - if (rioWriteBulkString(r,"JUSTID",6) == 0) return 0; - if (rioWriteBulkString(r,"FORCE",5) == 0) return 0; - /* clang-format on */ + if (rioWriteBulkCount(r, '*', 12) == 0) return 0; + if (rioWriteBulkString(r, "XCLAIM", 6) == 0) return 0; + if (rioWriteBulkObject(r, key) == 0) return 0; + if (rioWriteBulkString(r, groupname, groupname_len) == 0) return 0; + if (rioWriteBulkString(r, consumer->name, sdslen(consumer->name)) == 0) return 0; + if (rioWriteBulkString(r, "0", 1) == 0) return 0; + if (rioWriteBulkStreamID(r, &id) == 0) return 0; + if (rioWriteBulkString(r, "TIME", 4) == 0) return 0; + if (rioWriteBulkLongLong(r, nack->delivery_time) == 0) return 0; + if (rioWriteBulkString(r, "RETRYCOUNT", 10) == 0) return 0; + if (rioWriteBulkLongLong(r, nack->delivery_count) == 0) return 0; + if (rioWriteBulkString(r, "JUSTID", 6) == 0) return 0; + if (rioWriteBulkString(r, "FORCE", 5) == 0) return 0; return 1; } @@ -2021,14 +2019,12 @@ int rioWriteStreamEmptyConsumer(rio *r, size_t groupname_len, streamConsumer *consumer) { /* XGROUP CREATECONSUMER */ - /* clang-format off */ - if (rioWriteBulkCount(r,'*',5) == 0) return 0; - if (rioWriteBulkString(r,"XGROUP",6) == 0) return 0; - if (rioWriteBulkString(r,"CREATECONSUMER",14) == 0) return 0; - if (rioWriteBulkObject(r,key) == 0) return 0; - if (rioWriteBulkString(r,groupname,groupname_len) == 0) return 0; - if (rioWriteBulkString(r,consumer->name,sdslen(consumer->name)) == 0) return 0; - /* clang-format on */ + if (rioWriteBulkCount(r, '*', 5) == 0) return 0; + if (rioWriteBulkString(r, "XGROUP", 6) == 0) return 0; + if (rioWriteBulkString(r, "CREATECONSUMER", 14) == 0) return 0; + if (rioWriteBulkObject(r, key) == 0) return 0; + if (rioWriteBulkString(r, groupname, groupname_len) == 0) return 0; + if (rioWriteBulkString(r, consumer->name, sdslen(consumer->name)) == 0) return 0; return 1; } diff --git a/src/eval.c b/src/eval.c index 464c8ef487..8c6db3f18b 100644 --- a/src/eval.c +++ b/src/eval.c @@ -1221,20 +1221,18 @@ char *ldbRespToHuman_Double(sds *o, char *reply); * char*) so that we can return a modified pointer, as for SDS semantics. */ char *ldbRespToHuman(sds *o, char *reply) { char *p = reply; - /* clang-format off */ - switch(*p) { - case ':': p = ldbRespToHuman_Int(o,reply); break; - case '$': p = ldbRespToHuman_Bulk(o,reply); break; - case '+': p = ldbRespToHuman_Status(o,reply); break; - case '-': p = ldbRespToHuman_Status(o,reply); break; - case '*': p = ldbRespToHuman_MultiBulk(o,reply); break; - case '~': p = ldbRespToHuman_Set(o,reply); break; - case '%': p = ldbRespToHuman_Map(o,reply); break; - case '_': p = ldbRespToHuman_Null(o,reply); break; - case '#': p = ldbRespToHuman_Bool(o,reply); break; - case ',': p = ldbRespToHuman_Double(o,reply); break; + switch (*p) { + case ':': p = ldbRespToHuman_Int(o, reply); break; + case '$': p = ldbRespToHuman_Bulk(o, reply); break; + case '+': p = ldbRespToHuman_Status(o, reply); break; + case '-': p = ldbRespToHuman_Status(o, reply); break; + case '*': p = ldbRespToHuman_MultiBulk(o, reply); break; + case '~': p = ldbRespToHuman_Set(o, reply); break; + case '%': p = ldbRespToHuman_Map(o, reply); break; + case '_': p = ldbRespToHuman_Null(o, reply); break; + case '#': p = ldbRespToHuman_Bool(o, reply); break; + case ',': p = ldbRespToHuman_Double(o, reply); break; } - /* clang-format on */ return p; } diff --git a/src/listpack.c b/src/listpack.c index 640f10142d..be970e1e64 100644 --- a/src/listpack.c +++ b/src/listpack.c @@ -427,19 +427,17 @@ static inline void lpEncodeString(unsigned char *buf, unsigned char *s, uint32_t * lpCurrentEncodedSizeBytes or ASSERT_INTEGRITY_LEN (possibly since 'p' is * a return value of another function that validated its return. */ static inline uint32_t lpCurrentEncodedSizeUnsafe(unsigned char *p) { - /* clang-format off */ if (LP_ENCODING_IS_7BIT_UINT(p[0])) return 1; - if (LP_ENCODING_IS_6BIT_STR(p[0])) return 1+LP_ENCODING_6BIT_STR_LEN(p); + if (LP_ENCODING_IS_6BIT_STR(p[0])) return 1 + LP_ENCODING_6BIT_STR_LEN(p); if (LP_ENCODING_IS_13BIT_INT(p[0])) return 2; if (LP_ENCODING_IS_16BIT_INT(p[0])) return 3; if (LP_ENCODING_IS_24BIT_INT(p[0])) return 4; if (LP_ENCODING_IS_32BIT_INT(p[0])) return 5; if (LP_ENCODING_IS_64BIT_INT(p[0])) return 9; - if (LP_ENCODING_IS_12BIT_STR(p[0])) return 2+LP_ENCODING_12BIT_STR_LEN(p); - if (LP_ENCODING_IS_32BIT_STR(p[0])) return 5+LP_ENCODING_32BIT_STR_LEN(p); + if (LP_ENCODING_IS_12BIT_STR(p[0])) return 2 + LP_ENCODING_12BIT_STR_LEN(p); + if (LP_ENCODING_IS_32BIT_STR(p[0])) return 5 + LP_ENCODING_32BIT_STR_LEN(p); if (p[0] == LP_EOF) return 1; return 0; - /* clang-format on */ } /* Return bytes needed to encode the length of the listpack element pointed by 'p'. @@ -447,7 +445,6 @@ static inline uint32_t lpCurrentEncodedSizeUnsafe(unsigned char *p) { * of the element (excluding the element data itself) * If the element encoding is wrong then 0 is returned. */ static inline uint32_t lpCurrentEncodedSizeBytes(unsigned char *p) { - /* clang-format off */ if (LP_ENCODING_IS_7BIT_UINT(p[0])) return 1; if (LP_ENCODING_IS_6BIT_STR(p[0])) return 1; if (LP_ENCODING_IS_13BIT_INT(p[0])) return 1; @@ -459,7 +456,6 @@ static inline uint32_t lpCurrentEncodedSizeBytes(unsigned char *p) { if (LP_ENCODING_IS_32BIT_STR(p[0])) return 5; if (p[0] == LP_EOF) return 1; return 0; - /* clang-format on */ } /* Skip the current entry returning the next. It is invalid to call this diff --git a/src/networking.c b/src/networking.c index 2bbe012050..ecdeeb6588 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2843,7 +2843,6 @@ sds catClientInfoString(sds s, client *client) { else *p++ = 'S'; } - /* clang-format off */ if (client->flags & CLIENT_PRIMARY) *p++ = 'M'; if (client->flags & CLIENT_PUBSUB) *p++ = 'P'; if (client->flags & CLIENT_MULTI) *p++ = 'x'; @@ -2860,7 +2859,6 @@ sds catClientInfoString(sds s, client *client) { if (client->flags & CLIENT_NO_EVICT) *p++ = 'e'; if (client->flags & CLIENT_NO_TOUCH) *p++ = 'T'; if (p == flags) *p++ = 'N'; - /* clang-format on */ *p++ = '\0'; p = events; @@ -3269,15 +3267,13 @@ NULL listRewind(server.clients, &li); while ((ln = listNext(&li)) != NULL) { client *client = listNodeValue(ln); - /* clang-format off */ - if (addr && strcmp(getClientPeerId(client),addr) != 0) continue; - if (laddr && strcmp(getClientSockname(client),laddr) != 0) continue; + if (addr && strcmp(getClientPeerId(client), addr) != 0) continue; + if (laddr && strcmp(getClientSockname(client), laddr) != 0) continue; if (type != -1 && getClientType(client) != type) continue; if (id != 0 && client->id != id) continue; if (user && client->user != user) continue; if (c == client && skipme) continue; if (max_age != 0 && (long long)(commandTimeSnapshot() / 1000 - client->ctime) < max_age) continue; - /* clang-format on */ /* Kill it. */ if (c == client) { diff --git a/src/notify.c b/src/notify.c index 72018908fc..1cbf9c74ed 100644 --- a/src/notify.c +++ b/src/notify.c @@ -42,8 +42,7 @@ int keyspaceEventsStringToFlags(char *classes) { int c, flags = 0; while ((c = *p++) != '\0') { - /* clang-format off */ - switch(c) { + switch (c) { case 'A': flags |= NOTIFY_ALL; break; case 'g': flags |= NOTIFY_GENERIC; break; case '$': flags |= NOTIFY_STRING; break; @@ -61,7 +60,6 @@ int keyspaceEventsStringToFlags(char *classes) { case 'n': flags |= NOTIFY_NEW; break; default: return -1; } - /* clang-format on */ } return flags; } @@ -73,28 +71,26 @@ int keyspaceEventsStringToFlags(char *classes) { sds keyspaceEventsFlagsToString(int flags) { sds res; - /* clang-format off */ res = sdsempty(); if ((flags & NOTIFY_ALL) == NOTIFY_ALL) { - res = sdscatlen(res,"A",1); + res = sdscatlen(res, "A", 1); } else { - if (flags & NOTIFY_GENERIC) res = sdscatlen(res,"g",1); - if (flags & NOTIFY_STRING) res = sdscatlen(res,"$",1); - if (flags & NOTIFY_LIST) res = sdscatlen(res,"l",1); - if (flags & NOTIFY_SET) res = sdscatlen(res,"s",1); - if (flags & NOTIFY_HASH) res = sdscatlen(res,"h",1); - if (flags & NOTIFY_ZSET) res = sdscatlen(res,"z",1); - if (flags & NOTIFY_EXPIRED) res = sdscatlen(res,"x",1); - if (flags & NOTIFY_EVICTED) res = sdscatlen(res,"e",1); - if (flags & NOTIFY_STREAM) res = sdscatlen(res,"t",1); - if (flags & NOTIFY_MODULE) res = sdscatlen(res,"d",1); - if (flags & NOTIFY_NEW) res = sdscatlen(res,"n",1); + if (flags & NOTIFY_GENERIC) res = sdscatlen(res, "g", 1); + if (flags & NOTIFY_STRING) res = sdscatlen(res, "$", 1); + if (flags & NOTIFY_LIST) res = sdscatlen(res, "l", 1); + if (flags & NOTIFY_SET) res = sdscatlen(res, "s", 1); + if (flags & NOTIFY_HASH) res = sdscatlen(res, "h", 1); + if (flags & NOTIFY_ZSET) res = sdscatlen(res, "z", 1); + if (flags & NOTIFY_EXPIRED) res = sdscatlen(res, "x", 1); + if (flags & NOTIFY_EVICTED) res = sdscatlen(res, "e", 1); + if (flags & NOTIFY_STREAM) res = sdscatlen(res, "t", 1); + if (flags & NOTIFY_MODULE) res = sdscatlen(res, "d", 1); + if (flags & NOTIFY_NEW) res = sdscatlen(res, "n", 1); } - if (flags & NOTIFY_KEYSPACE) res = sdscatlen(res,"K",1); - if (flags & NOTIFY_KEYEVENT) res = sdscatlen(res,"E",1); - if (flags & NOTIFY_KEY_MISS) res = sdscatlen(res,"m",1); + if (flags & NOTIFY_KEYSPACE) res = sdscatlen(res, "K", 1); + if (flags & NOTIFY_KEYEVENT) res = sdscatlen(res, "E", 1); + if (flags & NOTIFY_KEY_MISS) res = sdscatlen(res, "m", 1); return res; - /* clang-format on */ } /* The API provided to the rest of the serer core is a simple function: diff --git a/src/object.c b/src/object.c index 4312524c87..7f93c3768d 100644 --- a/src/object.c +++ b/src/object.c @@ -372,8 +372,7 @@ void incrRefCount(robj *o) { void decrRefCount(robj *o) { if (o->refcount == 1) { - /* clang-format off */ - switch(o->type) { + switch (o->type) { case OBJ_STRING: freeStringObject(o); break; case OBJ_LIST: freeListObject(o); break; case OBJ_SET: freeSetObject(o); break; @@ -383,7 +382,6 @@ void decrRefCount(robj *o) { case OBJ_STREAM: freeStreamObject(o); break; default: serverPanic("Unknown object type"); break; } - /* clang-format on */ zfree(o); } else { if (o->refcount <= 0) serverPanic("decrRefCount against refcount <= 0"); @@ -552,8 +550,7 @@ void dismissObject(robj *o, size_t size_hint) { * so we avoid these pointless loops when they're not going to do anything. */ #if defined(USE_JEMALLOC) && defined(__linux__) if (o->refcount != 1) return; - /* clang-format off */ - switch(o->type) { + switch (o->type) { case OBJ_STRING: dismissStringObject(o); break; case OBJ_LIST: dismissListObject(o, size_hint); break; case OBJ_SET: dismissSetObject(o, size_hint); break; @@ -562,7 +559,6 @@ void dismissObject(robj *o, size_t size_hint) { case OBJ_STREAM: dismissStreamObject(o, size_hint); break; default: break; } - /* clang-format on */ #else UNUSED(o); UNUSED(size_hint); @@ -930,8 +926,7 @@ int getIntFromObjectOrReply(client *c, robj *o, int *target, const char *msg) { } char *strEncoding(int encoding) { - /* clang-format off */ - switch(encoding) { + switch (encoding) { case OBJ_ENCODING_RAW: return "raw"; case OBJ_ENCODING_INT: return "int"; case OBJ_ENCODING_HT: return "hashtable"; @@ -943,7 +938,6 @@ char *strEncoding(int encoding) { case OBJ_ENCODING_STREAM: return "stream"; default: return "unknown"; } - /* clang-format on */ } /* =========================== Memory introspection ========================= */ diff --git a/src/replication.c b/src/replication.c index 2c47136228..4fe8470371 100644 --- a/src/replication.c +++ b/src/replication.c @@ -3149,8 +3149,7 @@ void roleCommand(client *c) { if (replicaIsInHandshakeState()) { replica_state = "handshake"; } else { - /* clang-format off */ - switch(server.repl_state) { + switch (server.repl_state) { case REPL_STATE_NONE: replica_state = "none"; break; case REPL_STATE_CONNECT: replica_state = "connect"; break; case REPL_STATE_CONNECTING: replica_state = "connecting"; break; @@ -3158,7 +3157,6 @@ void roleCommand(client *c) { case REPL_STATE_CONNECTED: replica_state = "connected"; break; default: replica_state = "unknown"; break; } - /* clang-format on */ } addReplyBulkCString(c, replica_state); addReplyLongLong(c, server.primary ? server.primary->reploff : -1); diff --git a/src/resp_parser.c b/src/resp_parser.c index 3c075dece2..326766fc22 100644 --- a/src/resp_parser.c +++ b/src/resp_parser.c @@ -209,7 +209,6 @@ static int parseMap(ReplyParser *parser, void *p_ctx) { /* Parse a reply pointed to by parser->curr_location. */ int parseReply(ReplyParser *parser, void *p_ctx) { - /* clang-format off */ switch (parser->curr_location[0]) { case '$': return parseBulk(parser, p_ctx); case '+': return parseSimpleString(parser, p_ctx); @@ -224,8 +223,8 @@ int parseReply(ReplyParser *parser, void *p_ctx) { case '(': return parseBigNumber(parser, p_ctx); case '=': return parseVerbatimString(parser, p_ctx); case '|': return parseAttributes(parser, p_ctx); - default: if (parser->callbacks.error) parser->callbacks.error(p_ctx); + default: + if (parser->callbacks.error) parser->callbacks.error(p_ctx); } - /* clang-format on */ return C_ERR; } diff --git a/src/sds.c b/src/sds.c index 6793c46caa..c47901d73a 100644 --- a/src/sds.c +++ b/src/sds.c @@ -986,8 +986,7 @@ int is_hex_digit(char c) { /* Helper function for sdssplitargs() that converts a hex digit into an * integer from 0 to 15 */ int hex_digit_to_int(char c) { - /* clang-format off */ - switch(c) { + switch (c) { case '0': return 0; case '1': return 1; case '2': return 2; @@ -998,15 +997,20 @@ int hex_digit_to_int(char c) { case '7': return 7; case '8': return 8; case '9': return 9; - case 'a': case 'A': return 10; - case 'b': case 'B': return 11; - case 'c': case 'C': return 12; - case 'd': case 'D': return 13; - case 'e': case 'E': return 14; - case 'f': case 'F': return 15; + case 'a': + case 'A': return 10; + case 'b': + case 'B': return 11; + case 'c': + case 'C': return 12; + case 'd': + case 'D': return 13; + case 'e': + case 'E': return 14; + case 'f': + case 'F': return 15; default: return 0; } - /* clang-format on */ } /* Split a line into arguments, where every argument can be in the diff --git a/src/sentinel.c b/src/sentinel.c index c2677e8a04..49adcc05c9 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -3015,15 +3015,13 @@ static void populateDict(dict *options_dict, char **options) { } const char *getLogLevel(void) { - /* clang-format off */ - switch (server.verbosity) { + switch (server.verbosity) { case LL_DEBUG: return "debug"; case LL_VERBOSE: return "verbose"; case LL_NOTICE: return "notice"; case LL_WARNING: return "warning"; case LL_NOTHING: return "nothing"; } - /* clang-format on */ return "unknown"; } @@ -3203,8 +3201,7 @@ void sentinelConfigGetCommand(client *c) { } const char *sentinelFailoverStateStr(int state) { - /* clang-format off */ - switch(state) { + switch (state) { case SENTINEL_FAILOVER_STATE_NONE: return "none"; case SENTINEL_FAILOVER_STATE_WAIT_START: return "wait_start"; case SENTINEL_FAILOVER_STATE_SELECT_REPLICA: return "select_slave"; @@ -3214,7 +3211,6 @@ const char *sentinelFailoverStateStr(int state) { case SENTINEL_FAILOVER_STATE_UPDATE_CONFIG: return "update_config"; default: return "unknown"; } - /* clang-format on */ } /* Server instance to RESP representation. */ @@ -3242,23 +3238,21 @@ void addReplySentinelRedisInstance(client *c, sentinelRedisInstance *ri) { fields++; addReplyBulkCString(c, "flags"); - /* clang-format off */ - if (ri->flags & SRI_S_DOWN) flags = sdscat(flags,"s_down,"); - if (ri->flags & SRI_O_DOWN) flags = sdscat(flags,"o_down,"); - if (ri->flags & SRI_PRIMARY) flags = sdscat(flags,"master,"); - if (ri->flags & SRI_REPLICA) flags = sdscat(flags,"slave,"); - if (ri->flags & SRI_SENTINEL) flags = sdscat(flags,"sentinel,"); - if (ri->link->disconnected) flags = sdscat(flags,"disconnected,"); - if (ri->flags & SRI_PRIMARY_DOWN) flags = sdscat(flags,"master_down,"); - if (ri->flags & SRI_FAILOVER_IN_PROGRESS) flags = sdscat(flags,"failover_in_progress,"); - if (ri->flags & SRI_PROMOTED) flags = sdscat(flags,"promoted,"); - if (ri->flags & SRI_RECONF_SENT) flags = sdscat(flags,"reconf_sent,"); - if (ri->flags & SRI_RECONF_INPROG) flags = sdscat(flags,"reconf_inprog,"); - if (ri->flags & SRI_RECONF_DONE) flags = sdscat(flags,"reconf_done,"); - if (ri->flags & SRI_FORCE_FAILOVER) flags = sdscat(flags,"force_failover,"); - if (ri->flags & SRI_SCRIPT_KILL_SENT) flags = sdscat(flags,"script_kill_sent,"); - if (ri->flags & SRI_PRIMARY_REBOOT) flags = sdscat(flags,"master_reboot,"); - /* clang-format on */ + if (ri->flags & SRI_S_DOWN) flags = sdscat(flags, "s_down,"); + if (ri->flags & SRI_O_DOWN) flags = sdscat(flags, "o_down,"); + if (ri->flags & SRI_PRIMARY) flags = sdscat(flags, "master,"); + if (ri->flags & SRI_REPLICA) flags = sdscat(flags, "slave,"); + if (ri->flags & SRI_SENTINEL) flags = sdscat(flags, "sentinel,"); + if (ri->link->disconnected) flags = sdscat(flags, "disconnected,"); + if (ri->flags & SRI_PRIMARY_DOWN) flags = sdscat(flags, "master_down,"); + if (ri->flags & SRI_FAILOVER_IN_PROGRESS) flags = sdscat(flags, "failover_in_progress,"); + if (ri->flags & SRI_PROMOTED) flags = sdscat(flags, "promoted,"); + if (ri->flags & SRI_RECONF_SENT) flags = sdscat(flags, "reconf_sent,"); + if (ri->flags & SRI_RECONF_INPROG) flags = sdscat(flags, "reconf_inprog,"); + if (ri->flags & SRI_RECONF_DONE) flags = sdscat(flags, "reconf_done,"); + if (ri->flags & SRI_FORCE_FAILOVER) flags = sdscat(flags, "force_failover,"); + if (ri->flags & SRI_SCRIPT_KILL_SENT) flags = sdscat(flags, "script_kill_sent,"); + if (ri->flags & SRI_PRIMARY_REBOOT) flags = sdscat(flags, "master_reboot,"); if (sdslen(flags) != 0) sdsrange(flags, 0, -2); /* remove last "," */ addReplyBulkCString(c, flags); diff --git a/src/server.c b/src/server.c index 2853a23ac8..5065e40dd2 100644 --- a/src/server.c +++ b/src/server.c @@ -614,15 +614,13 @@ void updateDictResizePolicy(void) { } const char *strChildType(int type) { - /* clang-format off */ - switch(type) { + switch (type) { case CHILD_TYPE_RDB: return "RDB"; case CHILD_TYPE_AOF: return "AOF"; case CHILD_TYPE_LDB: return "LDB"; case CHILD_TYPE_MODULE: return "MODULE"; default: return "Unknown"; } - /* clang-format on */ } /* Return true if there are active children processes doing RDB saving, diff --git a/src/util.c b/src/util.c index 6006b071df..0d96cac6c3 100644 --- a/src/util.c +++ b/src/util.c @@ -1251,7 +1251,6 @@ static char *i2string_async_signal_safe(int base, int64_t val, char *buf) { int ix; buf = orig_buf - 1; for (ix = 0; ix < 16; ++ix, --buf) { - /* clang-format off */ switch (*buf) { case '0': *buf = 'f'; break; case '1': *buf = 'e'; break; @@ -1270,7 +1269,6 @@ static char *i2string_async_signal_safe(int base, int64_t val, char *buf) { case 'e': *buf = '1'; break; case 'f': *buf = '0'; break; } - /* clang-format on */ } } return buf + 1; diff --git a/src/valkey-benchmark.c b/src/valkey-benchmark.c index d122877a25..802b4c5735 100644 --- a/src/valkey-benchmark.c +++ b/src/valkey-benchmark.c @@ -1098,14 +1098,12 @@ static int fetchClusterConfiguration(void) { *p = '\0'; char *token = line; line = p + 1; - /* clang-format off */ - switch(i++){ + switch (i++) { case 0: name = token; break; case 1: addr = token; break; case 2: flags = token; break; case 3: primary_id = token; break; } - /* clang-format on */ if (i == 8) break; // Slots } if (!flags) { diff --git a/src/valkey-cli.c b/src/valkey-cli.c index f162a2927c..0f92a5fee7 100644 --- a/src/valkey-cli.c +++ b/src/valkey-cli.c @@ -5141,8 +5141,7 @@ static int clusterManagerNodeLoadInfo(clusterManagerNode *node, int opts, char * *p = '\0'; char *token = line; line = p + 1; - /* clang-format off */ - switch(i++){ + switch (i++) { case 0: name = token; break; case 1: addr = token; break; case 2: flags = token; break; @@ -5152,7 +5151,6 @@ static int clusterManagerNodeLoadInfo(clusterManagerNode *node, int opts, char * case 6: config_epoch = token; break; case 7: link_status = token; break; } - /* clang-format on */ if (i == 8) break; // Slots } if (!flags) { diff --git a/src/ziplist.c b/src/ziplist.c index bc3babd819..30efdc6573 100644 --- a/src/ziplist.c +++ b/src/ziplist.c @@ -340,15 +340,13 @@ static inline unsigned int zipEncodingLenSize(unsigned char encoding) { /* Return bytes needed to store integer encoded by 'encoding' */ static inline unsigned int zipIntSize(unsigned char encoding) { - /* clang-format off */ - switch(encoding) { - case ZIP_INT_8B: return 1; + switch (encoding) { + case ZIP_INT_8B: return 1; case ZIP_INT_16B: return 2; case ZIP_INT_24B: return 3; case ZIP_INT_32B: return 4; case ZIP_INT_64B: return 8; } - /* clang-format on */ if (encoding >= ZIP_INT_IMM_MIN && encoding <= ZIP_INT_IMM_MAX) return 0; /* 4 bit immediate */ /* bad encoding, covered by a previous call to ZIP_ASSERT_ENCODING */ valkey_unreachable(); From 627d387ad85ead296c1fa42f9eb2365f67aa1fd8 Mon Sep 17 00:00:00 2001 From: Madelyn Olson Date: Wed, 12 Jun 2024 14:27:42 -0700 Subject: [PATCH 7/9] Improve reliability of querybuf test (#639) We've been seeing some pretty consistent failures from `test-valgrind-test` and `test-sanitizer-address` because of the querybuf test periodically failing. I tracked it down to the test periodically taking too long and the client cron getting triggered. A simple solution is to just disable the cron during the key race condition. I was able to run this locally for 100 iterations without seeing a failure. Example: https://github.com/valkey-io/valkey/actions/runs/9474458354/job/26104103514 and https://github.com/valkey-io/valkey/actions/runs/9474458354/job/26104106830. Signed-off-by: Madelyn Olson --- tests/unit/querybuf.tcl | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unit/querybuf.tcl b/tests/unit/querybuf.tcl index 7e04b87905..913b993b1b 100644 --- a/tests/unit/querybuf.tcl +++ b/tests/unit/querybuf.tcl @@ -33,6 +33,9 @@ start_server {tags {"querybuf slow"}} { # Make sure query buff has size of 0 bytes at start as the client uses the shared qb. assert {[client_query_buffer test_client] == 0} + # Pause cron to prevent premature shrinking (timing issue). + r debug pause-cron 1 + # Send partial command to client to make sure it doesn't use the shared qb. $rd write "*3\r\n\$3\r\nset\r\n\$2\r\na" $rd flush @@ -48,6 +51,9 @@ start_server {tags {"querybuf slow"}} { set MAX_QUERY_BUFFER_SIZE [expr 32768 + 2] ; # 32k + 2, allowing for potential greedy allocation of (16k + 1) * 2 bytes for the query buffer. assert {$orig_test_client_qbuf >= 16384 && $orig_test_client_qbuf <= $MAX_QUERY_BUFFER_SIZE} + # Allow shrinking to occur + r debug pause-cron 0 + # Check that the initial query buffer is resized after 2 sec wait_for_condition 1000 10 { [client_idle_sec test_client] >= 3 && [client_query_buffer test_client] < $orig_test_client_qbuf From b546dd26e5f446d86275152af3e8295e07be22a4 Mon Sep 17 00:00:00 2001 From: Harkrishn Patro Date: Wed, 12 Jun 2024 21:09:01 -0700 Subject: [PATCH 8/9] Allow CLUSTER NODES/INFO/MYID/MYSHARDID during loading state (#596) Allow CLUSTER subcommands NODES, INFO, MYID, MYSHARDID while loading data. It's safe to allow them and it's helpful for clients to get cluster nodes/info information during a node failover and while loading data to monitor the state of the cluster. --------- Signed-off-by: Harkrishn Patro --- src/commands.def | 8 ++++---- src/commands/cluster-info.json | 1 + src/commands/cluster-myid.json | 1 + src/commands/cluster-myshardid.json | 1 + src/commands/cluster-nodes.json | 1 + 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/commands.def b/src/commands.def index c59cb01dc1..06cdb4b87e 100644 --- a/src/commands.def +++ b/src/commands.def @@ -966,13 +966,13 @@ struct COMMAND_STRUCT CLUSTER_Subcommands[] = { {MAKE_CMD("forget","Removes a node from the nodes table.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_FORGET_History,0,CLUSTER_FORGET_Tips,0,clusterCommand,3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE,0,CLUSTER_FORGET_Keyspecs,0,NULL,1),.args=CLUSTER_FORGET_Args}, {MAKE_CMD("getkeysinslot","Returns the key names in a hash slot.","O(N) where N is the number of requested keys","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_GETKEYSINSLOT_History,0,CLUSTER_GETKEYSINSLOT_Tips,1,clusterCommand,4,CMD_STALE,0,CLUSTER_GETKEYSINSLOT_Keyspecs,0,NULL,2),.args=CLUSTER_GETKEYSINSLOT_Args}, {MAKE_CMD("help","Returns helpful text about the different subcommands.","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_HELP_History,0,CLUSTER_HELP_Tips,0,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_HELP_Keyspecs,0,NULL,0)}, -{MAKE_CMD("info","Returns information about the state of a node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_INFO_History,0,CLUSTER_INFO_Tips,1,clusterCommand,2,CMD_STALE,0,CLUSTER_INFO_Keyspecs,0,NULL,0)}, +{MAKE_CMD("info","Returns information about the state of a node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_INFO_History,0,CLUSTER_INFO_Tips,1,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_INFO_Keyspecs,0,NULL,0)}, {MAKE_CMD("keyslot","Returns the hash slot for a key.","O(N) where N is the number of bytes in the key","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_KEYSLOT_History,0,CLUSTER_KEYSLOT_Tips,0,clusterCommand,3,CMD_STALE,0,CLUSTER_KEYSLOT_Keyspecs,0,NULL,1),.args=CLUSTER_KEYSLOT_Args}, {MAKE_CMD("links","Returns a list of all TCP links to and from peer nodes.","O(N) where N is the total number of Cluster nodes","7.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_LINKS_History,0,CLUSTER_LINKS_Tips,1,clusterCommand,2,CMD_STALE,0,CLUSTER_LINKS_Keyspecs,0,NULL,0)}, {MAKE_CMD("meet","Forces a node to handshake with another node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_MEET_History,1,CLUSTER_MEET_Tips,0,clusterCommand,-4,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE,0,CLUSTER_MEET_Keyspecs,0,NULL,3),.args=CLUSTER_MEET_Args}, -{MAKE_CMD("myid","Returns the ID of a node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_MYID_History,0,CLUSTER_MYID_Tips,0,clusterCommand,2,CMD_STALE,0,CLUSTER_MYID_Keyspecs,0,NULL,0)}, -{MAKE_CMD("myshardid","Returns the shard ID of a node.","O(1)","7.2.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_MYSHARDID_History,0,CLUSTER_MYSHARDID_Tips,1,clusterCommand,2,CMD_STALE,0,CLUSTER_MYSHARDID_Keyspecs,0,NULL,0)}, -{MAKE_CMD("nodes","Returns the cluster configuration for a node.","O(N) where N is the total number of Cluster nodes","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_NODES_History,0,CLUSTER_NODES_Tips,1,clusterCommand,2,CMD_STALE,0,CLUSTER_NODES_Keyspecs,0,NULL,0)}, +{MAKE_CMD("myid","Returns the ID of a node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_MYID_History,0,CLUSTER_MYID_Tips,0,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_MYID_Keyspecs,0,NULL,0)}, +{MAKE_CMD("myshardid","Returns the shard ID of a node.","O(1)","7.2.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_MYSHARDID_History,0,CLUSTER_MYSHARDID_Tips,1,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_MYSHARDID_Keyspecs,0,NULL,0)}, +{MAKE_CMD("nodes","Returns the cluster configuration for a node.","O(N) where N is the total number of Cluster nodes","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_NODES_History,0,CLUSTER_NODES_Tips,1,clusterCommand,2,CMD_LOADING|CMD_STALE,0,CLUSTER_NODES_Keyspecs,0,NULL,0)}, {MAKE_CMD("replicas","Lists the replica nodes of a master node.","O(N) where N is the number of replicas.","5.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_REPLICAS_History,0,CLUSTER_REPLICAS_Tips,1,clusterCommand,3,CMD_ADMIN|CMD_STALE,0,CLUSTER_REPLICAS_Keyspecs,0,NULL,1),.args=CLUSTER_REPLICAS_Args}, {MAKE_CMD("replicate","Configure a node as replica of a master node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_REPLICATE_History,0,CLUSTER_REPLICATE_Tips,0,clusterCommand,3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_STALE,0,CLUSTER_REPLICATE_Keyspecs,0,NULL,1),.args=CLUSTER_REPLICATE_Args}, {MAKE_CMD("reset","Resets a node.","O(N) where N is the number of known nodes. The command may execute a FLUSHALL as a side effect.","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_RESET_History,0,CLUSTER_RESET_Tips,0,clusterCommand,-2,CMD_ADMIN|CMD_STALE|CMD_NOSCRIPT,0,CLUSTER_RESET_Keyspecs,0,NULL,1),.args=CLUSTER_RESET_Args}, diff --git a/src/commands/cluster-info.json b/src/commands/cluster-info.json index 2c88760eb7..023d9b46bb 100644 --- a/src/commands/cluster-info.json +++ b/src/commands/cluster-info.json @@ -8,6 +8,7 @@ "container": "CLUSTER", "function": "clusterCommand", "command_flags": [ + "LOADING", "STALE" ], "command_tips": [ diff --git a/src/commands/cluster-myid.json b/src/commands/cluster-myid.json index caa62de756..4ef1ff7de9 100644 --- a/src/commands/cluster-myid.json +++ b/src/commands/cluster-myid.json @@ -8,6 +8,7 @@ "container": "CLUSTER", "function": "clusterCommand", "command_flags": [ + "LOADING", "STALE" ], "reply_schema": { diff --git a/src/commands/cluster-myshardid.json b/src/commands/cluster-myshardid.json index 01c05ba926..0e08417eec 100644 --- a/src/commands/cluster-myshardid.json +++ b/src/commands/cluster-myshardid.json @@ -8,6 +8,7 @@ "container": "CLUSTER", "function": "clusterCommand", "command_flags": [ + "LOADING", "STALE" ], "command_tips": [ diff --git a/src/commands/cluster-nodes.json b/src/commands/cluster-nodes.json index 9c5fcbe9a4..e12bca36b2 100644 --- a/src/commands/cluster-nodes.json +++ b/src/commands/cluster-nodes.json @@ -8,6 +8,7 @@ "container": "CLUSTER", "function": "clusterCommand", "command_flags": [ + "LOADING", "STALE" ], "command_tips": [ From d211078a27ba75f1dfdcdaf7d407f3370f914974 Mon Sep 17 00:00:00 2001 From: uriyage <78144248+uriyage@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:07:07 +0300 Subject: [PATCH 9/9] Fix query buffer resized test flakiness (#646) Added a wait_for_condition to avoid the timing issue. ``` *** [err]: query buffer resized correctly in tests/unit/querybuf.tcl Expected 11 >= 16384 && 11 <= 32770 (context: type eval line 24 cmd {assert {$orig_test_client_qbuf >= 16384 && $orig_test_client_qbuf <= $MAX_QUERY_BUFFER_SIZE}} proc ::test) *** [err]: query buffer resized correctly when not idle in tests/unit/querybuf.tcl Expected 11 > 32768 (context: type eval line 14 cmd {assert {$orig_test_client_qbuf > 32768}} proc ::test) *** [err]: query buffer resized correctly with fat argv in tests/unit/querybuf.tcl query buffer should not be resized when client idle time smaller than 2s ``` Signed-off-by: Uri Yagelnik --- tests/unit/querybuf.tcl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/querybuf.tcl b/tests/unit/querybuf.tcl index 913b993b1b..519743d248 100644 --- a/tests/unit/querybuf.tcl +++ b/tests/unit/querybuf.tcl @@ -39,7 +39,13 @@ start_server {tags {"querybuf slow"}} { # Send partial command to client to make sure it doesn't use the shared qb. $rd write "*3\r\n\$3\r\nset\r\n\$2\r\na" $rd flush - after 100 + # Wait for the client to start using a private query buffer. + wait_for_condition 1000 10 { + [client_query_buffer test_client] > 0 + } else { + fail "client should start using a private query buffer" + } + # send the rest of the command $rd write "a\r\n\$1\r\nb\r\n" $rd flush