From 0aec32cfe07b3b07cbcae206574e8ec5d9814bc2 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Fri, 14 Oct 2022 08:43:00 -0700 Subject: [PATCH 1/2] Variadic CONFIG GET/SET Redis 7.0.0 allows for getting and setting multiple config values as an atomic operation. In order to support this while maintaining backward compatibility, the CONFIG command is reworked to also accept an array of values or keys and values. See: #2068 --- README.markdown | 8 ++- library.c | 9 +++ library.h | 1 + redis.c | 43 ++---------- redis.stub.php | 24 ++++++- redis_arginfo.h | 6 +- redis_commands.c | 150 +++++++++++++++++++++++++++++++++++++++++ redis_commands.h | 3 + redis_legacy_arginfo.h | 4 +- tests/RedisTest.php | 36 +++++++++- 10 files changed, 234 insertions(+), 50 deletions(-) diff --git a/README.markdown b/README.markdown index 3d31c8f6f9..d580c27fa0 100644 --- a/README.markdown +++ b/README.markdown @@ -559,17 +559,19 @@ _**Description**_: Get or Set the Redis server configuration parameters. ##### *Prototype* ~~~php -$redis->config($operation, ?string $key = NULL, ?string $value = NULL): mixed; +$redis->config(string $operation, string|array|null $key = NULL, ?string $value = NULL): mixed; ~~~ ##### *Return value* -*Associative array* for `GET`, key -> value -*bool* for `SET`, and `RESETSTAT` +*Associative array* for `GET`, key(s) -> value(s) +*bool* for `SET`, `RESETSTAT`, and `REWRITE` ##### *Examples* ~~~php $redis->config("GET", "*max-*-entries*"); +$redis->config("SET", ['timeout', 'loglevel']); $redis->config("SET", "dir", "/var/run/redis/dumps/"); +$redis->config("SET", ['timeout' => 128, 'loglevel' => 'warning']); $redis->config('RESETSTAT'); ~~~ diff --git a/library.c b/library.c index bbdca77d25..3c8f5c4dae 100644 --- a/library.c +++ b/library.c @@ -1151,6 +1151,15 @@ PHP_REDIS_API int redis_type_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *r return SUCCESS; } +PHP_REDIS_API int +redis_config_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx) { + FailableResultCallback cb = ctx; + + ZEND_ASSERT(cb == redis_boolean_response || cb == redis_mbulk_reply_zipped_raw); + + return cb(INTERNAL_FUNCTION_PARAM_PASSTHRU, redis_sock, z_tab, ctx); +} + PHP_REDIS_API int redis_info_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx) { char *response; int response_len; diff --git a/library.h b/library.h index b67d72f876..036cb45615 100644 --- a/library.h +++ b/library.h @@ -67,6 +67,7 @@ PHP_REDIS_API int redis_bulk_double_response(INTERNAL_FUNCTION_PARAMETERS, Redis PHP_REDIS_API int redis_string_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx); PHP_REDIS_API int redis_ping_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx); PHP_REDIS_API int redis_info_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx); +PHP_REDIS_API int redis_config_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx); PHP_REDIS_API void redis_parse_info_response(char *response, zval *z_ret); PHP_REDIS_API void redis_parse_client_list_response(char *response, zval *z_ret); PHP_REDIS_API int redis_type_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, zval *z_tab, void *ctx); diff --git a/redis.c b/redis.c index 9ce8018b21..788deff35a 100644 --- a/redis.c +++ b/redis.c @@ -2707,45 +2707,10 @@ PHP_METHOD(Redis, setOption) /* }}} */ /* {{{ proto boolean Redis::config(string op, string key [, mixed value]) */ -PHP_METHOD(Redis, config) -{ - zend_string *op, *key = NULL, *val = NULL; - FailableResultCallback cb; - RedisSock *redis_sock; - zval *object; - int cmd_len; - char *cmd; - - if (zend_parse_method_parameters(ZEND_NUM_ARGS(), getThis(), "OS|SS", &object, - redis_ce, &op, &key, &val) == FAILURE) - { - RETURN_FALSE; - } - - if ((redis_sock = redis_sock_get(object, 0)) == NULL) { - RETURN_FALSE; - } - - if (zend_string_equals_literal_ci(op, "GET") && key != NULL) { - cmd_len = REDIS_SPPRINTF(&cmd, "CONFIG", "SS", op, key); - cb = redis_mbulk_reply_zipped_raw; - } else if (zend_string_equals_literal_ci(op, "RESETSTAT") || - zend_string_equals_literal_ci(op, "REWRITE")) - { - cmd_len = REDIS_SPPRINTF(&cmd, "CONFIG", "s", ZSTR_VAL(op), ZSTR_LEN(op)); - cb = redis_boolean_response; - } else if (zend_string_equals_literal_ci(op, "SET") && key != NULL && val != NULL) { - cmd_len = REDIS_SPPRINTF(&cmd, "CONFIG", "SSS", op, key, val); - cb = redis_boolean_response; - } else { - RETURN_FALSE; - } - - REDIS_PROCESS_REQUEST(redis_sock, cmd, cmd_len) - if (IS_ATOMIC(redis_sock)) { - cb(INTERNAL_FUNCTION_PARAM_PASSTHRU, redis_sock, NULL, NULL); - } - REDIS_PROCESS_RESPONSE(redis_boolean_response); +/* {{{ proto public function config(string $op, string ...$args) }}} */ +// CONFIG SET/GET +PHP_METHOD(Redis, config) { + REDIS_PROCESS_CMD(config, redis_config_response); } /* }}} */ diff --git a/redis.stub.php b/redis.stub.php index b41ff83101..61ceca0ddb 100644 --- a/redis.stub.php +++ b/redis.stub.php @@ -81,7 +81,29 @@ public function close(): bool; public function command(string $opt = null, string|array $arg): mixed; - public function config(string $operation, ?string $key = NULL, mixed $value = null): mixed; + /** + Execute the Redis CONFIG command in a variety of ways. What the command does in particular depends + on the `$operation` qualifier. + + Operations that PhpRedis supports are: RESETSTAT, REWRITE, GET, and SET. + + @param string $operation The CONFIG subcommand to execute + @param array|string|null $key_or_setting Can either be a setting string for the GET/SET operation or + an array of settings or settings and values. + Note: Redis 7.0.0 is required to send an array of settings. + @param ?string $value The setting value when the operation is SET. + + + config('GET', 'timeout'); + $redis->config('GET', ['timeout', 'databases']); + + $redis->config('SET', 'timeout', 30); + $redis->config('SET', ['timeout' => 30, 'loglevel' => 'warning']); + ?> + + */ + public function config(string $operation, array|string|null $key_or_settings = NULL, ?string $value = NULL): mixed; public function connect(string $host, int $port = 6379, float $timeout = 0, string $persistent_id = null, int $retry_interval = 0, float $read_timeout = 0, array $context = null): bool; diff --git a/redis_arginfo.h b/redis_arginfo.h index 41eb4738ae..d59539d434 100644 --- a/redis_arginfo.h +++ b/redis_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: bd81918bd558ec53399e7f64647c39f288f3413e */ + * Stub hash: a024c59eff58030ac224fc22cc4040b6e926a643 */ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Redis___construct, 0, 0, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, options, IS_ARRAY, 0, "null") @@ -127,8 +127,8 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Redis_config, 0, 1, IS_MIXED, 0) ZEND_ARG_TYPE_INFO(0, operation, IS_STRING, 0) - ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, key, IS_STRING, 1, "NULL") - ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, value, IS_MIXED, 0, "null") + ZEND_ARG_TYPE_MASK(0, key_or_settings, MAY_BE_ARRAY|MAY_BE_STRING|MAY_BE_NULL, "NULL") + ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, value, IS_STRING, 1, "NULL") ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_Redis_connect, 0, 1, _IS_BOOL, 0) diff --git a/redis_commands.c b/redis_commands.c index e415ae5c1e..6b23acbe5f 100644 --- a/redis_commands.c +++ b/redis_commands.c @@ -32,6 +32,14 @@ #include +/* Config operations */ +typedef enum redisConfigOp { + REDIS_CFG_RESETSTAT, + REDIS_CFG_REWRITE, + REDIS_CFG_GET, + REDIS_CFG_SET, +} redisConfigOp; + /* Georadius sort type */ typedef enum geoSortType { SORT_NONE, @@ -728,6 +736,148 @@ int redis_zrangebyscore_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, return SUCCESS; } +static int redis_get_config_op(enum redisConfigOp *dst, zend_string *op) { + if (zend_string_equals_literal_ci(op, "RESETSTAT")) + *dst = REDIS_CFG_RESETSTAT; + else if (zend_string_equals_literal_ci(op, "REWRITE")) + *dst = REDIS_CFG_REWRITE; + else if (zend_string_equals_literal_ci(op, "GET")) + *dst = REDIS_CFG_GET; + else if (zend_string_equals_literal_ci(op, "SET")) + *dst = REDIS_CFG_SET; + else + return FAILURE; + + return SUCCESS; +} + +static int redis_build_config_get_cmd(smart_string *dst, zval *val) { + zend_string *zstr; + int ncfg; + zval *zv; + + if (val == NULL || (Z_TYPE_P(val) != IS_STRING && Z_TYPE_P(val) != IS_ARRAY)) { + php_error_docref(NULL, E_WARNING, "Must pass a string or array of values to CONFIG GET"); + return FAILURE; + } else if (Z_TYPE_P(val) == IS_ARRAY && zend_hash_num_elements(Z_ARRVAL_P(val)) == 0) { + php_error_docref(NULL, E_WARNING, "Cannot pass an empty array to CONFIG GET"); + return FAILURE; + } + + ncfg = Z_TYPE_P(val) == IS_STRING ? 1 : zend_hash_num_elements(Z_ARRVAL_P(val)); + + REDIS_CMD_INIT_SSTR_STATIC(dst, 1 + ncfg, "CONFIG"); + REDIS_CMD_APPEND_SSTR_STATIC(dst, "GET"); + + if (Z_TYPE_P(val) == IS_STRING) { + redis_cmd_append_sstr_zstr(dst, Z_STR_P(val)); + } else { + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(val), zv) { + ZVAL_DEREF(zv); + + zstr = zval_get_string(zv); + redis_cmd_append_sstr_zstr(dst, zstr); + zend_string_release(zstr); + } ZEND_HASH_FOREACH_END(); + } + + return SUCCESS; +} + +static int redis_build_config_set_cmd(smart_string *dst, zval *key, zend_string *val) { + zend_string *zkey, *zstr; + zval *zv; + + /* Legacy case: CONFIG SET */ + if (key != NULL && val != NULL) { + REDIS_CMD_INIT_SSTR_STATIC(dst, 3, "CONFIG"); + REDIS_CMD_APPEND_SSTR_STATIC(dst, "SET"); + + zstr = zval_get_string(key); + redis_cmd_append_sstr_zstr(dst, zstr); + zend_string_release(zstr); + + redis_cmd_append_sstr_zstr(dst, val); + + return SUCCESS; + } + + /* Now we must have an array with at least one element */ + if (key == NULL || Z_TYPE_P(key) != IS_ARRAY || zend_hash_num_elements(Z_ARRVAL_P(key)) == 0) { + php_error_docref(NULL, E_WARNING, "Must either pass two strings to CONFIG SET or a non-empty array of values"); + return FAILURE; + } + + REDIS_CMD_INIT_SSTR_STATIC(dst, 1 + (2 * zend_hash_num_elements(Z_ARRVAL_P(key))), "CONFIG"); + REDIS_CMD_APPEND_SSTR_STATIC(dst, "SET"); + + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(key), zkey, zv) { + if (zkey == NULL) + goto fail; + + ZVAL_DEREF(zv); + + redis_cmd_append_sstr_zstr(dst, zkey); + + zstr = zval_get_string(zv); + redis_cmd_append_sstr_zstr(dst, zstr); + zend_string_release(zstr); + } ZEND_HASH_FOREACH_END(); + + return SUCCESS; + +fail: + php_error_docref(NULL, E_WARNING, "Must pass an associate array of config keys and values"); + efree(dst->c); + memset(dst, 0, sizeof(*dst)); + return FAILURE; +} + +int +redis_config_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, + char **cmd, int *cmd_len, short *slot, void **ctx) +{ + zend_string *op = NULL, *arg = NULL; + smart_string cmdstr = {0}; + enum redisConfigOp cfg_op; + int res = FAILURE; + zval *key = NULL; + + ZEND_PARSE_PARAMETERS_START(1, 3) + Z_PARAM_STR(op) + Z_PARAM_OPTIONAL + Z_PARAM_ZVAL_OR_NULL(key) + Z_PARAM_STR_OR_NULL(arg) + ZEND_PARSE_PARAMETERS_END_EX(return FAILURE); + + if (redis_get_config_op(&cfg_op, op) != SUCCESS) { + php_error_docref(NULL, E_WARNING, "Unknown operation '%s'", ZSTR_VAL(op)); + return FAILURE; + } + + switch (cfg_op) { + case REDIS_CFG_RESETSTAT: /* fallthrough */ + case REDIS_CFG_REWRITE: + REDIS_CMD_INIT_SSTR_STATIC(&cmdstr, 1, "CONFIG"); + redis_cmd_append_sstr_zstr(&cmdstr, op); + *ctx = redis_boolean_response; + res = SUCCESS; + break; + case REDIS_CFG_GET: + res = redis_build_config_get_cmd(&cmdstr, key); + *ctx = redis_mbulk_reply_zipped_raw; + break; + case REDIS_CFG_SET: + res = redis_build_config_set_cmd(&cmdstr, key, arg); + *ctx = redis_boolean_response; + break; + } + + *cmd = cmdstr.c; + *cmd_len = cmdstr.len; + return res; +} + int redis_zrandmember_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, char **cmd, int *cmd_len, short *slot, void **ctx) diff --git a/redis_commands.h b/redis_commands.h index ac63bc4113..ad76835613 100644 --- a/redis_commands.h +++ b/redis_commands.h @@ -110,6 +110,9 @@ int redis_zrangebyscore_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, char *kw, char **cmd, int *cmd_len, int *withscores, short *slot, void **ctx); +int redis_config_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, + char **cmd, int *cmd_len, short *slot, void **ctx); + int redis_zrandmember_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, char **cmd, int *cmd_len, short *slot, void **ctx); diff --git a/redis_legacy_arginfo.h b/redis_legacy_arginfo.h index 18db4f2584..27aec07433 100644 --- a/redis_legacy_arginfo.h +++ b/redis_legacy_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: bd81918bd558ec53399e7f64647c39f288f3413e */ + * Stub hash: a024c59eff58030ac224fc22cc4040b6e926a643 */ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Redis___construct, 0, 0, 0) ZEND_ARG_INFO(0, options) @@ -117,7 +117,7 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_INFO_EX(arginfo_class_Redis_config, 0, 0, 1) ZEND_ARG_INFO(0, operation) - ZEND_ARG_INFO(0, key) + ZEND_ARG_INFO(0, key_or_settings) ZEND_ARG_INFO(0, value) ZEND_END_ARG_INFO() diff --git a/tests/RedisTest.php b/tests/RedisTest.php index c240bc9f72..eab3e2f724 100644 --- a/tests/RedisTest.php +++ b/tests/RedisTest.php @@ -5612,9 +5612,9 @@ public function testConfig() { /* Ensure invalid calls are handled by PhpRedis */ foreach (['notacommand', 'get', 'set'] as $cmd) { - $this->assertFalse($this->redis->config($cmd)); + $this->assertFalse(@$this->redis->config($cmd)); } - $this->assertFalse($this->redis->config('set', 'foo')); + $this->assertFalse(@$this->redis->config('set', 'foo')); /* REWRITE. We don't care if it actually works, just that the command be attempted */ @@ -5624,6 +5624,38 @@ public function testConfig() { $this->assertPatternMatch($this->redis->getLastError(), '/.*config.*/'); $this->redis->clearLastError(); } + + if (!$this->minVersionCheck("7.0.0")) + return; + + /* Test getting multiple values */ + $settings = $this->redis->config('get', ['timeout', 'databases', 'set-max-intset-entries']); + $this->assertTrue(is_array($settings) && isset($settings['timeout']) && + isset($settings['databases']) && isset($settings['set-max-intset-entries'])); + + /* Short circuit if the above assertion would have failed */ + if ( ! is_array($settings) || ! isset($settings['timeout']) || ! isset($settings['set-max-intset-entries'])) + return; + + list($timeout, $max_intset) = [$settings['timeout'], $settings['set-max-intset-entries']]; + + $updates = [ + ['timeout' => (string)($timeout + 30), 'set-max-intset-entries' => (string)($max_intset + 128)], + ['timeout' => (string)($timeout), 'set-max-intset-entries' => (string)$max_intset], + ]; + + foreach ($updates as $update) { + $this->assertTrue($this->redis->config('set', $update)); + $vals = $this->redis->config('get', array_keys($update)); + ksort($vals); + ksort($update); + $this->assertEquals($vals, $update); + } + + /* Make sure PhpRedis catches malformed multiple get/set calls */ + $this->assertFalse(@$this->redis->config('get', [])); + $this->assertFalse(@$this->redis->config('set', [])); + $this->assertFalse(@$this->redis->config('set', [0, 1, 2])); } public function testReconnectSelect() { From 78fbff695a6d0f142cfa2175846f37c8a87e4279 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Tue, 18 Oct 2022 23:08:36 -0700 Subject: [PATCH 2/2] Simplify logic by removing CONFIG enum. --- redis_commands.c | 57 ++++++++++++------------------------------------ 1 file changed, 14 insertions(+), 43 deletions(-) diff --git a/redis_commands.c b/redis_commands.c index 6b23acbe5f..801e5059b8 100644 --- a/redis_commands.c +++ b/redis_commands.c @@ -32,14 +32,6 @@ #include -/* Config operations */ -typedef enum redisConfigOp { - REDIS_CFG_RESETSTAT, - REDIS_CFG_REWRITE, - REDIS_CFG_GET, - REDIS_CFG_SET, -} redisConfigOp; - /* Georadius sort type */ typedef enum geoSortType { SORT_NONE, @@ -736,21 +728,6 @@ int redis_zrangebyscore_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, return SUCCESS; } -static int redis_get_config_op(enum redisConfigOp *dst, zend_string *op) { - if (zend_string_equals_literal_ci(op, "RESETSTAT")) - *dst = REDIS_CFG_RESETSTAT; - else if (zend_string_equals_literal_ci(op, "REWRITE")) - *dst = REDIS_CFG_REWRITE; - else if (zend_string_equals_literal_ci(op, "GET")) - *dst = REDIS_CFG_GET; - else if (zend_string_equals_literal_ci(op, "SET")) - *dst = REDIS_CFG_SET; - else - return FAILURE; - - return SUCCESS; -} - static int redis_build_config_get_cmd(smart_string *dst, zval *val) { zend_string *zstr; int ncfg; @@ -839,7 +816,6 @@ redis_config_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, { zend_string *op = NULL, *arg = NULL; smart_string cmdstr = {0}; - enum redisConfigOp cfg_op; int res = FAILURE; zval *key = NULL; @@ -850,29 +826,24 @@ redis_config_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock, Z_PARAM_STR_OR_NULL(arg) ZEND_PARSE_PARAMETERS_END_EX(return FAILURE); - if (redis_get_config_op(&cfg_op, op) != SUCCESS) { + if (zend_string_equals_literal_ci(op, "RESETSTAT") || + zend_string_equals_literal_ci(op, "REWRITE")) + { + REDIS_CMD_INIT_SSTR_STATIC(&cmdstr, 1, "CONFIG"); + redis_cmd_append_sstr_zstr(&cmdstr, op); + *ctx = redis_boolean_response; + res = SUCCESS; + } else if (zend_string_equals_literal_ci(op, "GET")) { + res = redis_build_config_get_cmd(&cmdstr, key); + *ctx = redis_mbulk_reply_zipped_raw; + } else if (zend_string_equals_literal_ci(op, "SET")) { + res = redis_build_config_set_cmd(&cmdstr, key, arg); + *ctx = redis_boolean_response; + } else { php_error_docref(NULL, E_WARNING, "Unknown operation '%s'", ZSTR_VAL(op)); return FAILURE; } - switch (cfg_op) { - case REDIS_CFG_RESETSTAT: /* fallthrough */ - case REDIS_CFG_REWRITE: - REDIS_CMD_INIT_SSTR_STATIC(&cmdstr, 1, "CONFIG"); - redis_cmd_append_sstr_zstr(&cmdstr, op); - *ctx = redis_boolean_response; - res = SUCCESS; - break; - case REDIS_CFG_GET: - res = redis_build_config_get_cmd(&cmdstr, key); - *ctx = redis_mbulk_reply_zipped_raw; - break; - case REDIS_CFG_SET: - res = redis_build_config_set_cmd(&cmdstr, key, arg); - *ctx = redis_boolean_response; - break; - } - *cmd = cmdstr.c; *cmd_len = cmdstr.len; return res;