From b3dc23c5a8365a5e61b9ef8113970c1c36de6a13 Mon Sep 17 00:00:00 2001 From: xhe Date: Tue, 9 Jun 2020 19:46:49 +0800 Subject: [PATCH 01/17] add a read-only variant for HELLO As discussed in https://github.com/antirez/redis/issues/7364, it is good to have a HELLO command variant, which does not switch the current proto version of a redis server. While `HELLO` will work, it introduced a certain difficulty on parsing options of the command. We will need to offset the index of authentication and setname option by -1. So 0 is marked a special version meaning non-switching. And we do not need to change the code much. --- src/networking.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/networking.c b/src/networking.c index 8fee298c6..e16a39a14 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,7 +2763,7 @@ void helloCommand(client *c) { long long ver; if (getLongLongFromObject(c->argv[1],&ver) != C_OK || - ver < 2 || ver > 3) + (ver != 0 && ver < 2) || ver > 3) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; @@ -2797,7 +2797,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - c->resp = ver; + if (ver != 0) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); @@ -2807,7 +2807,7 @@ void helloCommand(client *c) { addReplyBulkCString(c,REDIS_VERSION); addReplyBulkCString(c,"proto"); - addReplyLongLong(c,ver); + addReplyLongLong(c,c->resp); addReplyBulkCString(c,"id"); addReplyLongLong(c,c->id); From 2e8f8c9b0c08d8c7c8b4dc72d65285d2761c5183 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 13:13:25 +0800 Subject: [PATCH 02/17] HELLO without protover Signed-off-by: xhe --- src/networking.c | 11 +++++------ src/sentinel.c | 2 +- src/server.c | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/networking.c b/src/networking.c index e16a39a14..37d4bf3d3 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2758,18 +2758,17 @@ NULL } } -/* HELLO [AUTH ] [SETNAME ] */ +/* HELLO [protocol-version] [AUTH ] [SETNAME ] */ void helloCommand(client *c) { - long long ver; + long long ver = 0; - if (getLongLongFromObject(c->argv[1],&ver) != C_OK || - (ver != 0 && ver < 2) || ver > 3) - { + if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && + (ver < 2 || ver > 3)) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; } - for (int j = 2; j < c->argc; j++) { + for (int j = ver == 0 ? 1 : 2; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { diff --git a/src/sentinel.c b/src/sentinel.c index 9764f9004..1c1b4bf5b 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -469,7 +469,7 @@ struct redisCommand sentinelcmds[] = { {"client",clientCommand,-2,"admin random @connection",0,NULL,0,0,0,0,0}, {"shutdown",shutdownCommand,-1,"admin",0,NULL,0,0,0,0,0}, {"auth",authCommand,-2,"no-auth fast @connection",0,NULL,0,0,0,0,0}, - {"hello",helloCommand,-2,"no-auth fast @connection",0,NULL,0,0,0,0,0}, + {"hello",helloCommand,-1,"no-auth fast @connection",0,NULL,0,0,0,0,0}, {"acl",aclCommand,-2,"admin",0,NULL,0,0,0,0,0,0}, {"command",commandCommand,-1, "random @connection", 0,NULL,0,0,0,0,0,0} }; diff --git a/src/server.c b/src/server.c index f28c6e114..0eabe8ea2 100644 --- a/src/server.c +++ b/src/server.c @@ -869,7 +869,7 @@ struct redisCommand redisCommandTable[] = { "admin no-script random ok-loading ok-stale @connection", 0,NULL,0,0,0,0,0,0}, - {"hello",helloCommand,-2, + {"hello",helloCommand,-1, "no-auth no-script fast no-monitor ok-loading ok-stale no-slowlog @connection", 0,NULL,0,0,0,0,0,0}, From 7a7c60459e0853b1880bd1692fb15c4b67e00fa9 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 15:26:24 +0800 Subject: [PATCH 03/17] add a test Signed-off-by: xhe --- tests/unit/tracking.tcl | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index fc2800791..deecc1d06 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -135,6 +135,26 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} } + test {HELLO without protover} { + set reply [r HELLO 3] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 3}} + + set reply [r HELLO 2] + assert {[lindex $reply 2] eq {proto 2}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 2}} + + set reply [r HELLO] + assert {[lindex $reply 2] eq {proto 2}} + } + test {RESP3 based basic invalidation} { r CLIENT TRACKING off r CLIENT TRACKING on From 50d750733e52dd9fff26fbcf8274d8d6c81ebfca Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 15:29:17 +0800 Subject: [PATCH 04/17] prefer ! Signed-off-by: xhe --- src/networking.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 37d4bf3d3..d66eb2cea 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2768,7 +2768,7 @@ void helloCommand(client *c) { return; } - for (int j = ver == 0 ? 1 : 2; j < c->argc; j++) { + for (int j = !ver ? 1 : 2; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { @@ -2796,7 +2796,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - if (ver != 0) c->resp = ver; + if (!!ver) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); From 60f13e7a865f7c0d4908b25803f42058c7a19201 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 16:50:08 +0800 Subject: [PATCH 05/17] try to fix the test Signed-off-by: xhe --- tests/unit/tracking.tcl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index deecc1d06..4d7da755d 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -146,13 +146,13 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} set reply [r HELLO 2] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} set reply [r HELLO] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} set reply [r HELLO] - assert {[lindex $reply 2] eq {proto 2}} + assert {[lindex $reply 6] eq 2} } test {RESP3 based basic invalidation} { From 456c347d4520bce417e48d0d01a87035483755a5 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:22 +0800 Subject: [PATCH 06/17] simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index d66eb2cea..0953b8e5e 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2796,7 +2796,7 @@ void helloCommand(client *c) { } /* Let's switch to the specified RESP mode. */ - if (!!ver) c->resp = ver; + if (ver) c->resp = ver; addReplyMapLen(c,6 + !server.sentinel_mode); addReplyBulkCString(c,"server"); From c07d3bd8dd8ba108f743c09b059f157e9799d5c5 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:36 +0800 Subject: [PATCH 07/17] simplify Co-authored-by: Oran Agra --- src/networking.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/networking.c b/src/networking.c index 0953b8e5e..132596f9e 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2761,6 +2761,7 @@ NULL /* HELLO [protocol-version] [AUTH ] [SETNAME ] */ void helloCommand(client *c) { long long ver = 0; + int next_arg = 1; if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && (ver < 2 || ver > 3)) { From 723b4a15a34ce25490882f40ef51b24a5b61304c Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:45 +0800 Subject: [PATCH 08/17] simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 132596f9e..32ffb3ce8 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,7 +2763,7 @@ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; - if (c->argc >= 2 && getLongLongFromObject(c->argv[1],&ver) == C_OK && + if (c->argc >= 2 && getLongLongFromObject(c->argv[next_arg++],&ver) == C_OK && (ver < 2 || ver > 3)) { addReplyError(c,"-NOPROTO unsupported protocol version"); return; From 98f39a37fb14f2b1c1965de143011402728dafbc Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:03:53 +0800 Subject: [PATCH 09/17] simplify Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 32ffb3ce8..e29256488 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2769,7 +2769,7 @@ void helloCommand(client *c) { return; } - for (int j = !ver ? 1 : 2; j < c->argc; j++) { + for (int j = next_arg; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { From ef14c18c8e83f3f11f5436e3dbeee9dfd5665de1 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 17:31:50 +0800 Subject: [PATCH 10/17] fix the test Signed-off-by: xhe --- tests/unit/tracking.tcl | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index 4d7da755d..1beaf71a9 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -146,13 +146,16 @@ start_server {tags {"tracking"}} { assert {[lindex $reply 2] eq {proto 3}} set reply [r HELLO 2] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} set reply [r HELLO] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} set reply [r HELLO] - assert {[lindex $reply 6] eq 2} + assert {[lindex $reply 4] eq "proto"} + assert {[lindex $reply 5] eq 2} } test {RESP3 based basic invalidation} { From 4e36925c662bba9c83661caa166d4699bcbef17c Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:16:28 +0800 Subject: [PATCH 11/17] correction Co-authored-by: Oran Agra --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index e29256488..6d571aad1 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2758,7 +2758,7 @@ NULL } } -/* HELLO [protocol-version] [AUTH ] [SETNAME ] */ +/* HELLO [ [AUTH ] [SETNAME ] ] */ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; From 955e00fbec2bf9cb2dbe53fceef00941681bfbd4 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:23:30 +0800 Subject: [PATCH 12/17] ask protover for authentication Signed-off-by: xhe --- src/networking.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/networking.c b/src/networking.c index 6d571aad1..66e91c521 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2769,6 +2769,11 @@ void helloCommand(client *c) { return; } + if (!ver && next_arg < c->argc) { + addReplyError(c,"Authentication needs to provide an protocol version"); + return; + } + for (int j = next_arg; j < c->argc; j++) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; From f6711b7da5445f3e4ba3595c9c0153183184078e Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 19:25:30 +0800 Subject: [PATCH 13/17] reword Signed-off-by: xhe --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index 66e91c521..0760a4669 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2770,7 +2770,7 @@ void helloCommand(client *c) { } if (!ver && next_arg < c->argc) { - addReplyError(c,"Authentication needs to provide an protocol version"); + addReplyError(c,"Need to provide an protocol version for other arguments"); return; } From 78eaf503fdbafe94d1905301ba47294326b278e4 Mon Sep 17 00:00:00 2001 From: xhe Date: Thu, 24 Dec 2020 20:13:57 +0800 Subject: [PATCH 14/17] address comment Signed-off-by: xhe --- src/networking.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/networking.c b/src/networking.c index 0760a4669..c44976d12 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2763,15 +2763,16 @@ void helloCommand(client *c) { long long ver = 0; int next_arg = 1; - if (c->argc >= 2 && getLongLongFromObject(c->argv[next_arg++],&ver) == C_OK && - (ver < 2 || ver > 3)) { - addReplyError(c,"-NOPROTO unsupported protocol version"); - return; - } + if (c->argc >= 2) { + if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, + "The second argument should the protocol version if provided") != C_OK) { + return; + } - if (!ver && next_arg < c->argc) { - addReplyError(c,"Need to provide an protocol version for other arguments"); - return; + if (ver < 2 || ver > 3) { + addReplyError(c,"-NOPROTO unsupported protocol version"); + return; + } } for (int j = next_arg; j < c->argc; j++) { From 4617960863c2baf3545ca74009b03358fa2363e1 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 24 Dec 2020 14:33:53 +0200 Subject: [PATCH 15/17] resolve hung test. --- tests/unit/tracking.tcl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/tracking.tcl b/tests/unit/tracking.tcl index 1beaf71a9..b315293a0 100644 --- a/tests/unit/tracking.tcl +++ b/tests/unit/tracking.tcl @@ -156,6 +156,9 @@ start_server {tags {"tracking"}} { set reply [r HELLO] assert {[lindex $reply 4] eq "proto"} assert {[lindex $reply 5] eq 2} + + # restore RESP3 for next test + r HELLO 3 } test {RESP3 based basic invalidation} { From fae5ceef2ab65797af7d7778143efb8e4d598301 Mon Sep 17 00:00:00 2001 From: xhe Date: Fri, 25 Dec 2020 01:40:06 +0800 Subject: [PATCH 16/17] reword Co-authored-by: Itamar Haber --- src/networking.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/networking.c b/src/networking.c index c44976d12..187fafbb4 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2765,7 +2765,7 @@ void helloCommand(client *c) { if (c->argc >= 2) { if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, - "The second argument should the protocol version if provided") != C_OK) { + "Protocol version is not an integer or out of range") != C_OK) { return; } From e6c1aeaf08b14076ccdf26aaef65b76b1597ee77 Mon Sep 17 00:00:00 2001 From: xhe Date: Fri, 25 Dec 2020 10:17:55 +0800 Subject: [PATCH 17/17] fix the format Signed-off-by: xhe --- src/networking.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/networking.c b/src/networking.c index 187fafbb4..ff71559a4 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2765,9 +2765,9 @@ void helloCommand(client *c) { if (c->argc >= 2) { if (getLongLongFromObjectOrReply(c, c->argv[next_arg++], &ver, - "Protocol version is not an integer or out of range") != C_OK) { + "Protocol version is not an integer or out of range") != C_OK) { return; - } + } if (ver < 2 || ver > 3) { addReplyError(c,"-NOPROTO unsupported protocol version");