sub-command support for ACL CAT and COMMAND LIST. redisCommand always…

… stores fullname (redis#10127)

Summary of changes:
1. Rename `redisCommand->name` to `redisCommand->declared_name`, it is a
  const char * for native commands and SDS for module commands.
2. Store the [sub]command fullname in `redisCommand->fullname` (sds).
3. List subcommands in `ACL CAT`
4. List subcommands in `COMMAND LIST`
5. `moduleUnregisterCommands` now will also free the module subcommands.
6. RM_GetCurrentCommandName returns full command name

Other changes:
1. Add `addReplyErrorArity` and `addReplyErrorExpireTime`
2. Remove `getFullCommandName` function that now is useless.
3. Some cleanups about `fullname` since now it is SDS.
4. Delete `populateSingleCommand` function from server.h that is useless.
5. Added tests to cover this change.
6. Add some module unload tests and fix the leaks
7. Make error messages uniform, make sure they always contain the full command
  name and that it's quoted.
7. Fixes some typos

see the history in redis#9504, fixes redis#10124

Co-authored-by: Oran Agra <[email protected]>
Co-authored-by: guybe7 <[email protected]>

src/acl.c

@@ -644,9 +644,7 @@ sds ACLDescribeSelectorCommandRulesSingleCommands(aclSelector *selector, aclSele
         int fakebit = ACLGetSelectorCommandBit(fake_selector,cmd->id);
         if (userbit != fakebit) {
             rules = sdscatlen(rules, userbit ? "+" : "-", 1);
-            sds fullname = getFullCommandName(cmd);
-            rules = sdscat(rules,fullname);
-            sdsfree(fullname);
+            rules = sdscatsds(rules,cmd->fullname);
             rules = sdscatlen(rules," ",1);
             ACLChangeSelectorPerm(fake_selector,cmd,userbit);
         }
@@ -660,9 +658,7 @@ sds ACLDescribeSelectorCommandRulesSingleCommands(aclSelector *selector, aclSele
         {
             for (int j = 0; selector->allowed_firstargs[cmd->id][j]; j++) {
                 rules = sdscatlen(rules,"+",1);
-                sds fullname = getFullCommandName(cmd);
-                rules = sdscat(rules,fullname);
-                sdsfree(fullname);
+                rules = sdscatsds(rules,cmd->fullname);
                 rules = sdscatlen(rules,"|",1);
                 rules = sdscatsds(rules,selector->allowed_firstargs[cmd->id][j]);
                 rules = sdscatlen(rules," ",1);
@@ -994,18 +990,18 @@ aclSelector *aclCreateSelectorFromOpSet(const char *opset, size_t opsetlen) {
  *              commands. For instance ~* allows all the keys. The pattern
  *              is a glob-style pattern like the one of KEYS.
  *              It is possible to specify multiple patterns.
- * %R~<pattern> Add key read pattern that specifies which keys can be read 
+ * %R~<pattern> Add key read pattern that specifies which keys can be read
  *              from.
  * %W~<pattern> Add key write pattern that specifies which keys can be
- *              written to. 
+ *              written to.
  * allkeys      Alias for ~*
  * resetkeys    Flush the list of allowed keys patterns.
  * &<pattern>   Add a pattern of channels that can be mentioned as part of
  *              Pub/Sub commands. For instance &* allows all the channels. The
  *              pattern is a glob-style pattern like the one of PSUBSCRIBE.
  *              It is possible to specify multiple patterns.
  * allchannels              Alias for &*
- * resetchannels            Flush the list of allowed keys patterns.
+ * resetchannels            Flush the list of allowed channel patterns.
  */
 int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
     if (!strcasecmp(op,"allkeys") ||
@@ -1456,9 +1452,12 @@ int ACLAuthenticateUser(client *c, robj *username, robj *password) {
  * should have an assigned ID (that is used to index the bitmap). This function
  * creates such an ID: it uses sequential IDs, reusing the same ID for the same
  * command name, so that a command retains the same ID in case of modules that
- * are unloaded and later reloaded. */
-unsigned long ACLGetCommandID(const char *cmdname) {
-    sds lowername = sdsnew(cmdname);
+ * are unloaded and later reloaded.
+ *
+ * The function does not take ownership of the 'cmdname' SDS string.
+ * */
+unsigned long ACLGetCommandID(sds cmdname) {
+    sds lowername = sdsdup(cmdname);
     sdstolower(lowername);
     if (commandId == NULL) commandId = raxNew();
     void *id = raxFind(commandId,(unsigned char*)lowername,sdslen(lowername));
@@ -2374,7 +2373,7 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username
         le->object = object;
     } else {
         switch(reason) {
-            case ACL_DENIED_CMD: le->object = getFullCommandName(c->cmd); break;
+            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;
@@ -2435,6 +2434,26 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username
  * ACL related commands
  * ==========================================================================*/
 
+/* ACL CAT category */
+void aclCatWithFlags(client *c, dict *commands, uint64_t cflag, int *arraylen) {
+    dictEntry *de;
+    dictIterator *di = dictGetIterator(commands);
+
+    while ((de = dictNext(di)) != NULL) {
+        struct redisCommand *cmd = dictGetVal(de);
+        if (cmd->flags & CMD_MODULE) continue;
+        if (cmd->acl_categories & cflag) {
+            addReplyBulkCBuffer(c, cmd->fullname, sdslen(cmd->fullname));
+            (*arraylen)++;
+        }
+
+        if (cmd->subcommands_dict) {
+            aclCatWithFlags(c, cmd->subcommands_dict, cflag, arraylen);
+        }
+    }
+    dictReleaseIterator(di);
+}
+
 /* Add the formatted response from a single selector to the ACL GETUSER
  * response. This function returns the number of fields added. 
  * 
@@ -2686,22 +2705,12 @@ void aclCommand(client *c) {
     } else if (!strcasecmp(sub,"cat") && c->argc == 3) {
         uint64_t cflag = ACLGetCommandCategoryFlagByName(c->argv[2]->ptr);
         if (cflag == 0) {
-            addReplyErrorFormat(c, "Unknown category '%s'", (char*)c->argv[2]->ptr);
+            addReplyErrorFormat(c, "Unknown category '%.128s'", (char*)c->argv[2]->ptr);
             return;
         }
         int arraylen = 0;
         void *dl = addReplyDeferredLen(c);
-        dictIterator *di = dictGetIterator(server.orig_commands);
-        dictEntry *de;
-        while ((de = dictNext(di)) != NULL) {
-            struct redisCommand *cmd = dictGetVal(de);
-            if (cmd->flags & CMD_MODULE) continue;
-            if (cmd->acl_categories & cflag) {
-                addReplyBulkCString(c,cmd->name);
-                arraylen++;
-            }
-        }
-        dictReleaseIterator(di);
+        aclCatWithFlags(c, server.orig_commands, cflag, &arraylen);
         setDeferredArrayLen(c,dl,arraylen);
     } else if (!strcasecmp(sub,"genpass") && (c->argc == 2 || c->argc == 3)) {
         #define GENPASS_MAX_BITS 4096
@@ -2809,7 +2818,7 @@ void aclCommand(client *c) {
             sds err = sdsempty();
             if (result == ACL_DENIED_CMD) {
                 err = sdscatfmt(err, "This user has no permissions to run "
-                    "the '%s' command or its subcommand", c->cmd->name);
+                    "the '%s' command", c->cmd->fullname);
             } else if (result == ACL_DENIED_KEY) {
                 err = sdscatfmt(err, "This user has no permissions to access "
                     "the '%s' key", c->argv[idx + 3]->ptr);

src/cluster.c

@@ -5131,8 +5131,7 @@ NULL
     } else if ((!strcasecmp(c->argv[1]->ptr,"addslotsrange") ||
                !strcasecmp(c->argv[1]->ptr,"delslotsrange")) && c->argc >= 4) {
         if (c->argc % 2 == 1) {
-            addReplyErrorFormat(c,"wrong number of arguments for '%s' command",
-                            c->cmd->name);
+            addReplyErrorArity(c);
             return;
         }
         /* CLUSTER ADDSLOTSRANGE <start slot> <end slot> [<start slot> <end slot> ...] */

src/expire.c

@@ -571,14 +571,14 @@ void expireGenericCommand(client *c, long long basetime, int unit) {
      * overflow by either unit conversion or basetime addition. */
     if (unit == UNIT_SECONDS) {
         if (when > LLONG_MAX / 1000 || when < LLONG_MIN / 1000) {
-            addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
+            addReplyErrorExpireTime(c);
             return;
         }
         when *= 1000;
     }
 
     if (when > LLONG_MAX - basetime) {
-        addReplyErrorFormat(c, "invalid expire time in %s", c->cmd->name);
+        addReplyErrorExpireTime(c);
         return;
     }
     when += basetime;

src/latency.c

@@ -522,33 +522,24 @@ void fillCommandCDF(client *c, struct hdr_histogram* histogram) {
 
 /* latencyCommand() helper to produce for all commands,
  * a per command cumulative distribution of latencies. */
-void latencyAllCommandsFillCDF(client *c) {
-    dictIterator *di = dictGetSafeIterator(server.commands);
+void latencyAllCommandsFillCDF(client *c, dict *commands, int *command_with_data) {
+    dictIterator *di = dictGetSafeIterator(commands);
     dictEntry *de;
     struct redisCommand *cmd;
-    void *replylen = addReplyDeferredLen(c);
-    int command_with_data = 0;
+
     while((de = dictNext(di)) != NULL) {
         cmd = (struct redisCommand *) dictGetVal(de);
         if (cmd->latency_histogram) {
-            addReplyBulkCString(c,cmd->name);
+            addReplyBulkCBuffer(c, cmd->fullname, sdslen(cmd->fullname));
             fillCommandCDF(c, cmd->latency_histogram);
-            command_with_data++;
+            (*command_with_data)++;
         }
 
         if (cmd->subcommands) {
-            for (int j = 0; cmd->subcommands[j].name; j++) {
-                struct redisCommand *sub = cmd->subcommands+j;
-                if (sub->latency_histogram) {
-                    addReplyBulkSds(c,getFullCommandName(sub));
-                    fillCommandCDF(c, sub->latency_histogram);
-                    command_with_data++;
-                }
-            }
+            latencyAllCommandsFillCDF(c, cmd->subcommands_dict, command_with_data);
         }
     }
     dictReleaseIterator(di);
-    setDeferredMapLen(c,replylen,command_with_data);
 }
 
 /* latencyCommand() helper to produce for a specific command set,
@@ -564,20 +555,24 @@ void latencySpecificCommandsFillCDF(client *c) {
         }
 
         if (cmd->latency_histogram) {
-            addReplyBulkSds(c,getFullCommandName(cmd));
+            addReplyBulkCBuffer(c, cmd->fullname, sdslen(cmd->fullname));
             fillCommandCDF(c, cmd->latency_histogram);
             command_with_data++;
         }
 
-        if (cmd->subcommands) {
-            for (int j = 0; cmd->subcommands[j].name; j++) {
-                struct redisCommand *sub = cmd->subcommands+j;
+        if (cmd->subcommands_dict) {
+            dictEntry *de;
+            dictIterator *di = dictGetSafeIterator(cmd->subcommands_dict);
+
+            while ((de = dictNext(di)) != NULL) {
+                struct redisCommand *sub = dictGetVal(de);
                 if (sub->latency_histogram) {
-                    addReplyBulkSds(c,getFullCommandName(sub));
+                    addReplyBulkCBuffer(c, sub->fullname, sdslen(sub->fullname));
                     fillCommandCDF(c, sub->latency_histogram);
                     command_with_data++;
                 }
             }
+            dictReleaseIterator(di);
         }
     }
     setDeferredMapLen(c,replylen,command_with_data);
@@ -725,7 +720,10 @@ void latencyCommand(client *c) {
     } else if (!strcasecmp(c->argv[1]->ptr,"histogram") && c->argc >= 2) {
         /* LATENCY HISTOGRAM*/
         if (c->argc == 2) {
-            latencyAllCommandsFillCDF(c);
+            int command_with_data = 0;
+            void *replylen = addReplyDeferredLen(c);
+            latencyAllCommandsFillCDF(c, server.commands, &command_with_data);
+            setDeferredMapLen(c, replylen, command_with_data);
         } else {
             latencySpecificCommandsFillCDF(c);
         }