[GitHub] [incubator-kvrocks] mapleFU commented on a diff in pull request #881: [WIP] Implement the command `hello`

[GitBox <gi...@apache.org>]

mapleFU commented on code in PR #881:
URL: https://github.com/apache/incubator-kvrocks/pull/881#discussion_r973560062


##########
src/redis_cmd.cc:
##########
@@ -4132,6 +4151,78 @@ class CommandEcho : public Commander {
   }
 };
 
+/* HELLO [<protocol-version> [AUTH <password>] [SETNAME <name>] ] */
+class CommandHello final : public Commander {
+public:
+  Status Execute(Server *svr, Connection *conn, std::string *output) override {
+    size_t next_arg = 1;
+    if (args_.size() >= 2) {
+      int64_t protocol;
+      auto parseResult = ParseInt<int64_t>(args_[next_arg], /* base= */ 10);
+      ++next_arg;
+      if (!parseResult.IsOK()) {
+        *output = Redis::Error("Protocol version is not an integer or out of range");
+        return parseResult.ToStatus();
+      }
+      protocol = parseResult.GetValue();
+
+      // In redis, it will check protocol < 2 or protocol > 3,
+      // but kvrocks only supports REPL2 by now.
+      if (protocol != 2) {
+        *output = Redis::Error("-NOPROTO unsupported protocol version");
+        return Status::OK();
+      }
+    }
+
+    // Handling AUTH and SETNAME
+    for (; next_arg < args_.size(); ++next_arg) {
+      size_t moreargs = args_.size() - next_arg - 1;
+      const std::string& opt = args_[next_arg];
+      if (opt == "AUTH" && moreargs != 0) {
+        const auto& user_password = args_[next_arg + 1];
+        auto authResult = AuthenticateUser(conn, svr->GetConfig(), user_password);
+        switch (authResult) {
+        case AuthResult::INVALID_PASSWORD:
+          *output = Redis::Error("ERR invalid password");
+          break;
+        case AuthResult::NO_REQUIRE_PASS:
+          *output = Redis::Error("ERR Client sent AUTH, but no password is set");
+          break;
+        case AuthResult::OK:
+          break;
+        }
+        if (authResult != AuthResult::OK) {
+          return Status::OK();
+        }
+        next_arg += 1;
+      } else if (opt == "SETNAME" && moreargs != 0) {
+        const std::string& ns = args_[next_arg + 1];
+        conn->SetNamespace(ns);

Review Comment:
   I think 
   ```
          const std::string& ns = args_[next_arg + 1];
           conn->SetNamespace(ns);
   ```
   
   can represent that `args_[next_arg + 1]` means the `name` of connection, so I think just leave it here is ok.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org