From 005e6ffc3d0a6a53549705156ae2dba1c5c64178 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Tue, 6 Feb 2024 15:30:05 +0100 Subject: [PATCH 1/8] gencommands.py: more flexible command handling * Don't require the filename to match the command name * Don't require command name to be in uppercase in the JSON file * Allow non-alphanumeric chars in command name * Correct uppercase ordering of commands where ordering would be ambiguos if strncasecmp would be used (e.g. EVAL_RO vs EVALSHA). --- cmddef.h | 6 +++--- command.c | 28 ++++++++++++++++++++----- gencommands.py | 50 ++++++++++++++++++++++++-------------------- tests/ut_parse_cmd.c | 44 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 31 deletions(-) diff --git a/cmddef.h b/cmddef.h index b3b19ad..26e7485 100644 --- a/cmddef.h +++ b/cmddef.h @@ -98,9 +98,9 @@ COMMAND(DISCARD, "DISCARD", NULL, 1, NONE, 0) COMMAND(DUMP, "DUMP", NULL, 2, INDEX, 1) COMMAND(ECHO, "ECHO", NULL, 2, NONE, 0) COMMAND(EVAL, "EVAL", NULL, -3, KEYNUM, 2) -COMMAND(EVAL_RO, "EVAL_RO", NULL, -3, KEYNUM, 2) COMMAND(EVALSHA, "EVALSHA", NULL, -3, KEYNUM, 2) COMMAND(EVALSHA_RO, "EVALSHA_RO", NULL, -3, KEYNUM, 2) +COMMAND(EVAL_RO, "EVAL_RO", NULL, -3, KEYNUM, 2) COMMAND(EXEC, "EXEC", NULL, 1, NONE, 0) COMMAND(EXISTS, "EXISTS", NULL, -2, INDEX, 1) COMMAND(EXPIRE, "EXPIRE", NULL, -3, INDEX, 1) @@ -125,9 +125,9 @@ COMMAND(GEODIST, "GEODIST", NULL, -4, INDEX, 1) COMMAND(GEOHASH, "GEOHASH", NULL, -2, INDEX, 1) COMMAND(GEOPOS, "GEOPOS", NULL, -2, INDEX, 1) COMMAND(GEORADIUS, "GEORADIUS", NULL, -6, INDEX, 1) -COMMAND(GEORADIUS_RO, "GEORADIUS_RO", NULL, -6, INDEX, 1) COMMAND(GEORADIUSBYMEMBER, "GEORADIUSBYMEMBER", NULL, -5, INDEX, 1) COMMAND(GEORADIUSBYMEMBER_RO, "GEORADIUSBYMEMBER_RO", NULL, -5, INDEX, 1) +COMMAND(GEORADIUS_RO, "GEORADIUS_RO", NULL, -6, INDEX, 1) COMMAND(GEOSEARCH, "GEOSEARCH", NULL, -7, INDEX, 1) COMMAND(GEOSEARCHSTORE, "GEOSEARCHSTORE", NULL, -8, INDEX, 1) COMMAND(GET, "GET", NULL, 2, INDEX, 1) @@ -235,8 +235,8 @@ COMMAND(RENAMENX, "RENAMENX", NULL, 3, INDEX, 1) COMMAND(REPLCONF, "REPLCONF", NULL, -1, NONE, 0) COMMAND(REPLICAOF, "REPLICAOF", NULL, 3, NONE, 0) COMMAND(RESET, "RESET", NULL, 1, NONE, 0) -COMMAND(RESTORE_ASKING, "RESTORE-ASKING", NULL, -4, INDEX, 1) COMMAND(RESTORE, "RESTORE", NULL, -4, INDEX, 1) +COMMAND(RESTORE_ASKING, "RESTORE-ASKING", NULL, -4, INDEX, 1) COMMAND(ROLE, "ROLE", NULL, 1, NONE, 0) COMMAND(RPOP, "RPOP", NULL, -2, INDEX, 1) COMMAND(RPOPLPUSH, "RPOPLPUSH", NULL, 3, INDEX, 1) diff --git a/command.c b/command.c index 856504f..1bfbf06 100644 --- a/command.c +++ b/command.c @@ -34,6 +34,7 @@ #ifndef _WIN32 #include #endif +#include #include #include "command.h" @@ -75,6 +76,16 @@ static cmddef redis_commands[] = { #undef COMMAND }; +static inline void to_upper(char *dst, const char *src, uint32_t len) { + uint32_t i; + for (i = 0; i < len; i++) { + if (src[i] >= 'a' && src[i] <= 'z') + dst[i] = src[i] - ('a' - 'A'); + else + dst[i] = src[i]; + } +} + /* Looks up a command or subcommand in the command table. Arg0 and arg1 are used * to lookup the command. The function returns CMD_UNKNOWN on failure. On * success, the command type is returned and *firstkey and *arity are @@ -82,13 +93,17 @@ static cmddef redis_commands[] = { cmddef *redis_lookup_cmd(const char *arg0, uint32_t arg0_len, const char *arg1, uint32_t arg1_len) { int num_commands = sizeof(redis_commands) / sizeof(cmddef); + /* Compare command name in uppercase. */ + char *cmd = alloca(arg0_len); + to_upper(cmd, arg0, arg0_len); + char *subcmd = NULL; /* Alloca later on demand. */ /* Find the command using binary search. */ int left = 0, right = num_commands - 1; while (left <= right) { int i = (left + right) / 2; cmddef *c = &redis_commands[i]; - int cmp = strncasecmp(c->name, arg0, arg0_len); + int cmp = strncmp(c->name, cmd, arg0_len); if (cmp == 0 && strlen(c->name) > arg0_len) cmp = 1; /* "HGETALL" vs "HGET" */ @@ -97,11 +112,14 @@ cmddef *redis_lookup_cmd(const char *arg0, uint32_t arg0_len, const char *arg1, if (arg1 == NULL) { /* Command has subcommands, but none given. */ return NULL; - } else { - cmp = strncasecmp(c->subname, arg1, arg1_len); - if (cmp == 0 && strlen(c->subname) > arg1_len) - cmp = 1; } + if (subcmd == NULL) { + subcmd = alloca(arg1_len); + to_upper(subcmd, arg1, arg1_len); + } + cmp = strncmp(c->subname, subcmd, arg1_len); + if (cmp == 0 && strlen(c->subname) > arg1_len) + cmp = 1; } if (cmp < 0) { diff --git a/gencommands.py b/gencommands.py index 2e2b09b..3576e7f 100755 --- a/gencommands.py +++ b/gencommands.py @@ -13,6 +13,7 @@ import json import os import sys +import re # Returns a tuple (method, index) where method is one of the following: # @@ -50,32 +51,37 @@ def firstkey(props): def extract_command_info(name, props): (firstkeymethod, firstkeypos) = firstkey(props) container = props.get("container", "") + name = name.upper() subcommand = None if container != "": subcommand = name - name = container + name = container.upper() return (name, subcommand, props["arity"], firstkeymethod, firstkeypos); -# Checks that the filename matches the command. We need this because we rely on -# the alphabetic order. -def check_filename(filename, cmd): - if cmd[1] is None: - expect = "%s" % cmd[0] - else: - expect = "%s-%s" % (cmd[0], cmd[1]) - expect = expect.lower() + ".json" - assert os.path.basename(filename) == expect - def collect_commands_from_files(filenames): - commands = [] + # The keys in the dicts are "command" or "command_subcommand". + commands = dict() + commands_that_have_subcommands = set() for filename in filenames: with open(filename, "r") as f: try: d = json.load(f) for name, props in d.items(): cmd = extract_command_info(name, props) - check_filename(filename, cmd) - commands.append(cmd) + (name, subcmd, _, _, _) = cmd + + # For commands with subcommands, we want only the + # command-subcommand pairs, not the container command alone + if subcmd is not None: + commands_that_have_subcommands.add(name) + if name in commands: + del commands[name] + name += "_" + subcmd + elif name in commands_that_have_subcommands: + continue + + commands[name] = cmd + except json.decoder.JSONDecodeError as err: print("Error processing %s: %s" % (filename, err)) exit(1) @@ -85,18 +91,16 @@ def generate_c_code(commands): print("/* This file was generated using gencommands.py */") print("") print("/* clang-format off */") - commands_that_have_subcommands = set() - for (name, subcmd, arity, firstkeymethod, firstkeypos) in commands: + for key in sorted(commands): + (name, subcmd, arity, firstkeymethod, firstkeypos) = commands[key] + # Make valid C identifier (macro name) + key = re.sub(r'\W', '_', key) if subcmd is None: - if name in commands_that_have_subcommands: - continue # only include the command with its subcommands print("COMMAND(%s, \"%s\", NULL, %d, %s, %d)" % - (name.replace("-", "_"), name, arity, firstkeymethod, firstkeypos)) + (key, name, arity, firstkeymethod, firstkeypos)) else: - commands_that_have_subcommands.add(name) - print("COMMAND(%s_%s, \"%s\", \"%s\", %d, %s, %d)" % - (name.replace("-", "_"), subcmd.replace("-", "_"), - name, subcmd, arity, firstkeymethod, firstkeypos)) + print("COMMAND(%s, \"%s\", \"%s\", %d, %s, %d)" % + (key, name, subcmd, arity, firstkeymethod, firstkeypos)) # MAIN diff --git a/tests/ut_parse_cmd.c b/tests/ut_parse_cmd.c index d662be9..1d80813 100644 --- a/tests/ut_parse_cmd.c +++ b/tests/ut_parse_cmd.c @@ -13,6 +13,11 @@ /* Helper for the macro ASSERT_KEYS below. */ void check_keys(char **keys, int numkeys, struct cmd *command, char *file, int line) { + if (command->result != CMD_PARSE_OK) { + fprintf(stderr, "%s:%d: Command parsing failed: %s\n", file, line, + command->errstr); + assert(0); + } int actual_numkeys = (int)hiarray_n(command->keys); if (actual_numkeys != numkeys) { fprintf(stderr, "%s:%d: Expected %d keys but got %d\n", file, line, @@ -155,6 +160,42 @@ void test_redis_parse_cmd_xread_ok(void) { command_destroy(c); } +void test_redis_parse_cmd_restore_ok(void) { + /* The ordering of RESTORE and RESTORE-ASKING in the lookup-table was wrong + * in a previous version, leading to the command not being found. */ + struct cmd *c = command_get(); + int len = redisFormatCommand(&c->cmd, "restore k 0 xxx"); + ASSERT_MSG(len >= 0, "Format command error"); + c->clen = len; + redis_parse_cmd(c); + ASSERT_KEYS(c, "k"); + command_destroy(c); +} + +void test_redis_parse_cmd_restore_asking_ok(void) { + /* The ordering of RESTORE and RESTORE-ASKING in the lookup-table was wrong + * in a previous version, leading to the command not being found. */ + struct cmd *c = command_get(); + int len = redisFormatCommand(&c->cmd, "restore-asking k 0 xxx"); + ASSERT_MSG(len >= 0, "Format command error"); + c->clen = len; + redis_parse_cmd(c); + ASSERT_KEYS(c, "k"); + command_destroy(c); +} + +void test_redis_parse_cmd_georadius_ro_ok(void) { + /* The position of GEORADIUS_RO was wrong in a previous version of the + * lookup-table, leading to the command not being found. */ + struct cmd *c = command_get(); + int len = redisFormatCommand(&c->cmd, "georadius_ro k 0 0 0 km"); + ASSERT_MSG(len >= 0, "Format command error"); + c->clen = len; + redis_parse_cmd(c); + ASSERT_KEYS(c, "k"); + command_destroy(c); +} + int main(void) { test_redis_parse_error_nonresp(); test_redis_parse_cmd_get(); @@ -166,5 +207,8 @@ int main(void) { test_redis_parse_cmd_xgroup_destroy_ok(); test_redis_parse_cmd_xreadgroup_ok(); test_redis_parse_cmd_xread_ok(); + test_redis_parse_cmd_restore_ok(); + test_redis_parse_cmd_restore_asking_ok(); + test_redis_parse_cmd_georadius_ro_ok(); return 0; } From 02cfa8f2ded4fb8a962085bc285950d722d67a0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Tue, 6 Feb 2024 16:02:13 +0100 Subject: [PATCH 2/8] gencommands.py: accept multiple JSON files --- gencommands.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/gencommands.py b/gencommands.py index 3576e7f..dc679a7 100755 --- a/gencommands.py +++ b/gencommands.py @@ -7,7 +7,7 @@ # describing the commands. This is done manually when commands have been added # to Redis. # -# Usage: ./gencommands.py path/to/redis > cmddef.h +# Usage: ./gencommands.py path/to/redis/src/commands/*.json > cmddef.h import glob import json @@ -105,18 +105,24 @@ def generate_c_code(commands): # MAIN if len(sys.argv) < 2 or sys.argv[1] == "--help": - print("Usage: %s REDIS-DIR > cmddef.h" % sys.argv[0]) + print("Usage: %s path/to/redis/src/commands/*.json > cmddef.h" % sys.argv[0]) exit(1) -redisdir = sys.argv[1] -jsondir = os.path.join(redisdir, "src", "commands") -if not os.path.isdir(jsondir): - print("The path %s doesn't point to a Redis source directory." % redisdir) - exit(1) +# Fine all JSON files +filenames = [] +for filename in sys.argv[1:]: + if os.path.isdir(filename): + # A redis repo root dir (accepted for backward compatibility) + jsondir = os.path.join(filename, "src", "commands") + if not os.path.isdir(jsondir): + print("The directory %s is not a Redis source directory." % filename) + exit(1) + + filenames += glob.glob(os.path.join(jsondir, "*.json")) + else: + filenames.append(filename) # Collect all command info -filenames = glob.glob(os.path.join(jsondir, "*.json")) -filenames.sort() commands = collect_commands_from_files(filenames) # Print C code From aeba0c0656bdbfd517c5ba0a1bbfa51d248191e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Wed, 7 Feb 2024 18:12:45 +0100 Subject: [PATCH 3/8] gencommands.py: Handle JSON files without key specs and with space in command name --- gencommands.py | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/gencommands.py b/gencommands.py index dc679a7..a4028a2 100755 --- a/gencommands.py +++ b/gencommands.py @@ -26,9 +26,35 @@ # number of keys is zero in which case there are no # keys (example EVAL) def firstkey(props): - if not "key_specs" in props or len(props["key_specs"]) == 0: - # No keys + if not "key_specs" in props: + # Key specs missing. Best-effort fallback to "arguments" for modules. To + # avoid returning UNKNOWN istead of NONE for official Redis commands + # without keys, we check for "arity" which is always defined in Redis + # but not in the Redis Stack modules which also lack key specs. + if "arguments" in props and "arity" not in props: + args = props["arguments"] + for i in range(1, len(args)): + arg = args[i - 1] + if not "type" in arg: + return ("NONE", 0) + if arg["type"] == "key": + return ("INDEX", i) + elif arg["type"] == "string": + if "name" in arg and arg["name"] == "key": + # add-hoc case for RediSearch + return ("INDEX", i) + if "optional" in arg and arg["optional"]: + return ("UNKNOWN", 0) + if "multiple" in arg and arg["multiple"]: + return ("UNKNOWN", 0) + else: + # Too complex for this fallback. + return ("UNKNOWN", 0) return ("NONE", 0) + + if len(props["key_specs"]) == 0: + return ("NONE", 0) + # We detect the first key spec and only if the begin_search is by index. # Otherwise we return -1 for unknown (for example if the first key is # indicated by a keyword like KEYS or STREAMS). @@ -56,7 +82,18 @@ def extract_command_info(name, props): if container != "": subcommand = name name = container.upper() - return (name, subcommand, props["arity"], firstkeymethod, firstkeypos); + else: + # Ad-hoc handling of command and subcommand in the same string, + # sepatated by a space. This form is used in e.g. RediSearch's JSON file + # in commands like "FT.CONFIG GET". + tokens = name.split(maxsplit=1) + if len(tokens) > 1: + name, subcommand = tokens + if firstkeypos > 0: + firstkeypos += 1 + + arity = props["arity"] if "arity" in props else -1 + return (name, subcommand, arity, firstkeymethod, firstkeypos); def collect_commands_from_files(filenames): # The keys in the dicts are "command" or "command_subcommand". From 793024ebd737edc83b184775f54c29cee2cccdfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 8 Feb 2024 15:09:03 +0100 Subject: [PATCH 4/8] gencommands.py: Accept cmddef.h as input file --- gencommands.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/gencommands.py b/gencommands.py index a4028a2..2d731f6 100755 --- a/gencommands.py +++ b/gencommands.py @@ -8,6 +8,9 @@ # to Redis. # # Usage: ./gencommands.py path/to/redis/src/commands/*.json > cmddef.h +# +# Additional JSON files can be added to define custom commands. For convenience, +# files on the output format like cmddef.h can also be used as input files. import glob import json @@ -95,12 +98,35 @@ def extract_command_info(name, props): arity = props["arity"] if "arity" in props else -1 return (name, subcommand, arity, firstkeymethod, firstkeypos); +# Parses a file with lines like +# COMMAND(identifier, cmd, subcmd, arity, firstkeymethod, firstkeypos) +def collect_command_from_cmddef_h(f, commands): + for line in f: + m = re.match(r'^COMMAND\(\S+, *"(\S+)", NULL, *(-?\d+), *(\w+), *(\d+)\)', line) + if m: + commands[m.group(1)] = (m.group(1), None, int(m.group(2)), m.group(3), int(m.group(4))) + continue + m = re.match(r'^COMMAND\(\S+, *"(\S+)", *"(\S+)", *(-?\d+), *(\w+), *(\d)\)', line) + if m: + key = m.group(1) + "_" + m.group(2) + commands[key] = (m.group(1), m.group(2), int(m.group(3)), m.group(4), int(m.group(5))) + continue + if re.match(r'^(?:/\*.*\*/)?\s*$', line): + # Comment or blank line + continue + else: + print("Error processing line: %s" % (line)) + exit(1) + def collect_commands_from_files(filenames): # The keys in the dicts are "command" or "command_subcommand". commands = dict() commands_that_have_subcommands = set() for filename in filenames: with open(filename, "r") as f: + if filename.endswith(".h"): + collect_command_from_cmddef_h(f, commands) + continue try: d = json.load(f) for name, props in d.items(): @@ -145,7 +171,7 @@ def generate_c_code(commands): print("Usage: %s path/to/redis/src/commands/*.json > cmddef.h" % sys.argv[0]) exit(1) -# Fine all JSON files +# Find all JSON files filenames = [] for filename in sys.argv[1:]: if os.path.isdir(filename): From eeef58e34d96e877e19a5613ffd4814747b7ba04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 8 Feb 2024 17:22:52 +0100 Subject: [PATCH 5/8] Try to fix windows build --- command.c | 4 +++- win32.h | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/command.c b/command.c index 1bfbf06..442cfce 100644 --- a/command.c +++ b/command.c @@ -32,9 +32,11 @@ #include #include #ifndef _WIN32 +#include #include +#else +#include #endif -#include #include #include "command.h" diff --git a/win32.h b/win32.h index 57a5309..f1fa94c 100644 --- a/win32.h +++ b/win32.h @@ -42,6 +42,10 @@ #define strncasecmp _strnicmp #endif +#ifndef alloca +#define alloca _alloca +#endif + #endif /* _MSC_VER */ #ifdef _WIN32 From a4d964a808857981ca8913049968e9dc20e2f418 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 8 Feb 2024 17:23:21 +0100 Subject: [PATCH 6/8] Spelling --- gencommands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gencommands.py b/gencommands.py index 2d731f6..4f2284a 100755 --- a/gencommands.py +++ b/gencommands.py @@ -31,7 +31,7 @@ def firstkey(props): if not "key_specs" in props: # Key specs missing. Best-effort fallback to "arguments" for modules. To - # avoid returning UNKNOWN istead of NONE for official Redis commands + # avoid returning UNKNOWN instead of NONE for official Redis commands # without keys, we check for "arity" which is always defined in Redis # but not in the Redis Stack modules which also lack key specs. if "arguments" in props and "arity" not in props: From b9da4740b3164573cbb25bb4b0913ced10f728d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Thu, 8 Feb 2024 17:30:29 +0100 Subject: [PATCH 7/8] Add some documentation in the README file --- README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/README.md b/README.md index 88f34a8..105ad2f 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,17 @@ as described in the CMake docs; a specific path can be set using a flag like: See `examples/using_cmake_separate/build.sh` or `examples/using_cmake_externalproject/build.sh` for alternative CMake builds. +### Extend the list of supported commands + +The list of commands and the position of the first key in the command line is +defined in `cmddef.h` which is included in this repo. It has been generated +using the JSON files describing the syntax of each command in the Redis +repository, which makes sure hiredis-cluster supports all commands in Redis, at +least in terms of cluster routing. To add support for custom commands defined in +Redis modules, you can regenerate `cmddef.h` using the script `gencommands.py`. +Use the JSON files from Redis and any additional files on the same format as +arguments to the script. For details, see the comments inside `gencommands.py`. + ### Alternative build using Makefile directly When a simpler build setup is preferred a provided Makefile can be used directly From 1f11233b2da6d231f73ce8b4e5c58a0096454fc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Viktor=20S=C3=B6derqvist?= Date: Fri, 9 Feb 2024 09:59:23 +0100 Subject: [PATCH 8/8] Update comment about the JSON format in gencommands.py --- gencommands.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/gencommands.py b/gencommands.py index 4f2284a..4291b57 100755 --- a/gencommands.py +++ b/gencommands.py @@ -9,8 +9,21 @@ # # Usage: ./gencommands.py path/to/redis/src/commands/*.json > cmddef.h # -# Additional JSON files can be added to define custom commands. For convenience, -# files on the output format like cmddef.h can also be used as input files. +# Additional JSON files can be added to define custom commands. The JSON file +# format is not fully documented but hopefully the format can be understood from +# reading the existing JSON files. Alternatively, you can read the source code +# of this script to see what it does. +# +# The key specifications part is documented here: +# https://redis.io/docs/reference/key-specs/ +# +# The discussion where this JSON format was added in Redis is here: +# https://github.com/redis/redis/issues/9359 +# +# For convenience, files on the output format like cmddef.h can also be used as +# input files to this script. It can be used for adding more commands to the +# existing set of commands, but please do not abuse it. Do not to write commands +# information directly in this format. import glob import json