Remove Parser::statusToInt(). (#21288)

The motivation is that the int errno is implementation-specific (in this case defined by http-parser). Removing this method from the abstract Parser interface and changing ParserCallbacks::setAndCheckCallbackStatus* methods to return ParserStatus instead of this implementation-specific int will make it simpler to add another Parser implementation, one not based on http-parser. Instead of applying statusToInt() within setAndCheckCallbackStatus() and setAndCheckCallbackStatusOr() before returning in ConnectionImpl, return ParserStatus, and apply statusToInt() within LegacyHttpParserImpl. This allows statusToInt() to be moved to legacy_parser_impl.cc anonymous namespace, right next to its counterpart intToStatus(). Tracking issue: #21245 Signed-off-by: Bence Béky <bnc@google.com>
envoyproxy · May 16, 2022 · 932ba9d · 932ba9d
1 parent 3efaecf
commit 932ba9d
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 40 deletions.
diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc
@@ -453,20 +453,19 @@ Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool e
   return okStatus();
 }
 
-int ConnectionImpl::setAndCheckCallbackStatus(Status&& status) {
+ParserStatus ConnectionImpl::setAndCheckCallbackStatus(Status&& status) {
   ASSERT(codec_status_.ok());
   codec_status_ = std::move(status);
-  return codec_status_.ok() ? parser_->statusToInt(ParserStatus::Success)
-                            : parser_->statusToInt(ParserStatus::Error);
+  return codec_status_.ok() ? ParserStatus::Success : ParserStatus::Error;
 }
 
-int ConnectionImpl::setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) {
+ParserStatus ConnectionImpl::setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) {
   ASSERT(codec_status_.ok());
   if (statusor.ok()) {
-    return parser_->statusToInt(statusor.value());
+    return statusor.value();
   } else {
     codec_status_ = std::move(statusor.status());
-    return parser_->statusToInt(ParserStatus::Error);
+    return ParserStatus::Error;
   }
 }
 

diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h
@@ -349,8 +349,8 @@ class ConnectionImpl : public virtual Connection,
   Envoy::StatusOr<ParserStatus> onHeadersComplete() override;
   void bufferBody(const char* data, size_t length) override;
   StatusOr<ParserStatus> onMessageComplete() override;
-  int setAndCheckCallbackStatus(Http::Status&& status) override;
-  int setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) override;
+  ParserStatus setAndCheckCallbackStatus(Http::Status&& status) override;
+  ParserStatus setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) override;
 
   // Connection specific callback methods.
   virtual Envoy::StatusOr<ParserStatus> onHeadersCompleteBase() PURE;

diff --git a/source/common/http/http1/legacy_parser_impl.cc b/source/common/http/http1/legacy_parser_impl.cc
@@ -11,6 +11,7 @@ namespace Envoy {
 namespace Http {
 namespace Http1 {
 namespace {
+
 ParserStatus intToStatus(int rc) {
   // See
   // https://github.com/nodejs/http-parser/blob/5c5b3ac62662736de9e71640a8dc16da45b32503/http_parser.h#L72.
@@ -29,6 +30,26 @@ ParserStatus intToStatus(int rc) {
     return ParserStatus::Unknown;
   }
 }
+
+int statusToInt(const ParserStatus status) {
+  // See
+  // https://github.com/nodejs/http-parser/blob/5c5b3ac62662736de9e71640a8dc16da45b32503/http_parser.h#L72.
+  switch (status) {
+  case ParserStatus::Error:
+    return -1;
+  case ParserStatus::Success:
+    return 0;
+  case ParserStatus::NoBody:
+    return 1;
+  case ParserStatus::NoBodyData:
+    return 2;
+  case ParserStatus::Paused:
+    return 31;
+  default:
+    PANIC("not implemented");
+  }
+}
+
 } // namespace
 
 class LegacyHttpParserImpl::Impl {
@@ -44,32 +65,32 @@ class LegacyHttpParserImpl::Impl {
         [](http_parser* parser) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onMessageBegin();
-          return conn_impl->setAndCheckCallbackStatus(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatus(std::move(status)));
         },
         [](http_parser* parser, const char* at, size_t length) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onUrl(at, length);
-          return conn_impl->setAndCheckCallbackStatus(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatus(std::move(status)));
         },
         [](http_parser* parser, const char* at, size_t length) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onStatus(at, length);
-          return conn_impl->setAndCheckCallbackStatus(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatus(std::move(status)));
         },
         [](http_parser* parser, const char* at, size_t length) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onHeaderField(at, length);
-          return conn_impl->setAndCheckCallbackStatus(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatus(std::move(status)));
         },
         [](http_parser* parser, const char* at, size_t length) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onHeaderValue(at, length);
-          return conn_impl->setAndCheckCallbackStatus(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatus(std::move(status)));
         },
         [](http_parser* parser) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto statusor = conn_impl->onHeadersComplete();
-          return conn_impl->setAndCheckCallbackStatusOr(std::move(statusor));
+          return statusToInt(conn_impl->setAndCheckCallbackStatusOr(std::move(statusor)));
         },
         [](http_parser* parser, const char* at, size_t length) -> int {
           static_cast<ParserCallbacks*>(parser->data)->bufferBody(at, length);
@@ -78,7 +99,7 @@ class LegacyHttpParserImpl::Impl {
         [](http_parser* parser) -> int {
           auto* conn_impl = static_cast<ParserCallbacks*>(parser->data);
           auto status = conn_impl->onMessageComplete();
-          return conn_impl->setAndCheckCallbackStatusOr(std::move(status));
+          return statusToInt(conn_impl->setAndCheckCallbackStatusOr(std::move(status)));
         },
         [](http_parser* parser) -> int {
           // A 0-byte chunk header is used to signal the end of the chunked body.
@@ -184,25 +205,6 @@ absl::string_view LegacyHttpParserImpl::errorMessage() const {
 
 int LegacyHttpParserImpl::hasTransferEncoding() const { return impl_->hasTransferEncoding(); }
 
-int LegacyHttpParserImpl::statusToInt(const ParserStatus code) const {
-  // See
-  // https://github.com/nodejs/http-parser/blob/5c5b3ac62662736de9e71640a8dc16da45b32503/http_parser.h#L72.
-  switch (code) {
-  case ParserStatus::Error:
-    return -1;
-  case ParserStatus::Success:
-    return 0;
-  case ParserStatus::NoBody:
-    return 1;
-  case ParserStatus::NoBodyData:
-    return 2;
-  case ParserStatus::Paused:
-    return 31;
-  default:
-    PANIC("not implemented");
-  }
-}
-
 } // namespace Http1
 } // namespace Http
 } // namespace Envoy
diff --git a/source/common/http/http1/legacy_parser_impl.h b/source/common/http/http1/legacy_parser_impl.h
@@ -26,7 +26,6 @@ class LegacyHttpParserImpl : public Parser {
   absl::string_view methodName() const override;
   absl::string_view errorMessage() const override;
   int hasTransferEncoding() const override;
-  int statusToInt(const ParserStatus code) const override;
 
 private:
   class Impl;

diff --git a/source/common/http/http1/parser.h b/source/common/http/http1/parser.h
@@ -106,8 +106,8 @@ class ParserCallbacks {
    */
   virtual void onChunkHeader(bool) PURE;
 
-  virtual int setAndCheckCallbackStatus(Status&& status) PURE;
-  virtual int setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) PURE;
+  virtual ParserStatus setAndCheckCallbackStatus(Status&& status) PURE;
+  virtual ParserStatus setAndCheckCallbackStatusOr(Envoy::StatusOr<ParserStatus>&& statusor) PURE;
 };
 
 class Parser {
@@ -152,9 +152,6 @@ class Parser {
 
   // Returns whether the Transfer-Encoding header is present.
   virtual int hasTransferEncoding() const PURE;
-
-  // Converts a ParserStatus code to the parsers' integer return code value.
-  virtual int statusToInt(const ParserStatus code) const PURE;
 };
 
 using ParserPtr = std::unique_ptr<Parser>;