Extract frame header and frame length parsing into separate helpers

Summary:
The `MoQControlCodec::onIngress()` state machine had the logic for two of its three parse states (`FRAME_HEADER_TYPE` and `FRAME_LENGTH`) inlined directly in the `switch` body, making the function lengthy and harder to follow.

This diff extracts those two cases into the private helpers that were already declared in the header: parseFrameHeaderType and parseFrameLength.

Reviewed By: afrind

Differential Revision: D96370652

fbshipit-source-id: 28b5d852cab8a77576e5230b387c0cef4e5ea73f

Author: Aman Sharma

moxygen/MoQCodec.cpp

@@ -16,6 +16,76 @@ void MoQCodec::onIngressStart(std::unique_ptr<folly::IOBuf> data) {
   ingress_.append(std::move(data));
 }
 
+folly::Expected<folly::Unit, ErrorCode> MoQControlCodec::parseFrameHeaderType(
+    folly::io::Cursor& cursor,
+    size_t& remainingLength) {
+  auto type = quic::follyutils::decodeQuicInteger(cursor);
+  if (!type) {
+    XLOG(DBG6) << __func__ << " underflow";
+    return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
+  }
+  curFrameType_ = FrameType(type->first);
+  remainingLength -= type->second;
+  if (!checkFrameAllowed(curFrameType_)) {
+    XLOG(DBG4) << "Frame not allowed: 0x" << std::setfill('0')
+               << std::setw(sizeof(uint64_t) * 2) << std::hex
+               << (uint64_t)curFrameType_ << " on streamID=" << streamId_;
+    return folly::makeUnexpected(ErrorCode::PROTOCOL_VIOLATION);
+  }
+  if (callback_) {
+    callback_->onFrame(curFrameType_);
+  }
+  parseState_ = ParseState::FRAME_LENGTH;
+  return folly::unit;
+}
+
+folly::Expected<folly::Unit, ErrorCode> MoQControlCodec::parseFrameLength(
+    folly::io::Cursor& cursor,
+    size_t& remainingLength) {
+  uint64_t length = 0;
+  size_t bytesParsed = 0;
+
+  bool parseFrameLengthAs16bit = false;
+  if (curFrameType_ == FrameType::CLIENT_SETUP ||
+      curFrameType_ == FrameType::SERVER_SETUP) {
+    parseFrameLengthAs16bit = true;
+  } else if (
+      curFrameType_ == FrameType::LEGACY_CLIENT_SETUP ||
+      curFrameType_ == FrameType::LEGACY_SERVER_SETUP) {
+    parseFrameLengthAs16bit = false;
+  } else if (!moqFrameParser_.getVersion().has_value()) {
+    XLOG(DBG4)
+        << "Received a non-setup frame before knowing the negotiated version";
+    return folly::makeUnexpected(ErrorCode::PROTOCOL_VIOLATION);
+  } else {
+    parseFrameLengthAs16bit =
+        (getDraftMajorVersion(*moqFrameParser_.getVersion()) >= 11);
+  }
+
+  if (parseFrameLengthAs16bit) {
+    if (remainingLength < 2) {
+      XLOG(DBG6) << __func__ << " underflow";
+      return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
+    }
+    // Parse the length as a 16 bit integer
+    length = cursor.readBE<uint16_t>();
+    bytesParsed = 2;
+  } else {
+    // Parse the length as a varint
+    auto decodeResult = quic::follyutils::decodeQuicInteger(cursor);
+    if (!decodeResult) {
+      XLOG(DBG6) << __func__ << " underflow";
+      return folly::makeUnexpected(ErrorCode::PARSE_UNDERFLOW);
+    }
+    length = decodeResult->first;
+    bytesParsed = decodeResult->second;
+  }
+  curFrameLength_ = length;
+  remainingLength -= bytesParsed;
+  parseState_ = ParseState::FRAME_PAYLOAD;
+  return folly::unit;
+}
+
 MoQCodec::ParseResult MoQControlCodec::onIngress(
     std::unique_ptr<folly::IOBuf> data,
     bool eom) {
@@ -25,73 +95,19 @@ MoQCodec::ParseResult MoQControlCodec::onIngress(
   while (!connError_ && remainingLength > 0) {
     switch (parseState_) {
       case ParseState::FRAME_HEADER_TYPE: {
-        auto type = quic::follyutils::decodeQuicInteger(cursor);
-        if (!type) {
-          XLOG(DBG6) << __func__ << " underflow";
-          connError_ = ErrorCode::PARSE_UNDERFLOW;
-          break;
-        }
-        curFrameType_ = FrameType(type->first);
-        remainingLength -= type->second;
-        auto res = checkFrameAllowed(curFrameType_);
-        if (!res) {
-          XLOG(DBG4) << "Frame not allowed: 0x" << std::setfill('0')
-                     << std::setw(sizeof(uint64_t) * 2) << std::hex
-                     << (uint64_t)curFrameType_ << " on streamID=" << streamId_;
-          connError_.emplace(ErrorCode::PROTOCOL_VIOLATION);
+        auto res = parseFrameHeaderType(cursor, remainingLength);
+        if (res.hasError()) {
+          connError_ = res.error();
           break;
         }
-        if (callback_) {
-          callback_->onFrame(curFrameType_);
-        }
-        parseState_ = ParseState::FRAME_LENGTH;
         [[fallthrough]];
       }
       case ParseState::FRAME_LENGTH: {
-        uint64_t length = 0;
-        size_t bytesParsed = 0;
-
-        bool parseFrameLengthAs16bit = false;
-        if (curFrameType_ == FrameType::CLIENT_SETUP ||
-            curFrameType_ == FrameType::SERVER_SETUP) {
-          parseFrameLengthAs16bit = true;
-        } else if (
-            curFrameType_ == FrameType::LEGACY_CLIENT_SETUP ||
-            curFrameType_ == FrameType::LEGACY_SERVER_SETUP) {
-          parseFrameLengthAs16bit = false;
-        } else if (!moqFrameParser_.getVersion().has_value()) {
-          XLOG(DBG4)
-              << "Received a non-setup frame before knowing the negotiated version";
-          connError_.emplace(ErrorCode::PROTOCOL_VIOLATION);
+        auto res = parseFrameLength(cursor, remainingLength);
+        if (res.hasError()) {
+          connError_ = res.error();
           break;
-        } else {
-          parseFrameLengthAs16bit =
-              (getDraftMajorVersion(*moqFrameParser_.getVersion()) >= 11);
-        }
-
-        if (parseFrameLengthAs16bit) {
-          if (remainingLength < 2) {
-            XLOG(DBG6) << __func__ << " underflow";
-            connError_ = ErrorCode::PARSE_UNDERFLOW;
-            break;
-          }
-          // Parse the length as a 16 bit integer
-          length = cursor.readBE<uint16_t>();
-          bytesParsed = 2;
-        } else {
-          // Parse the length as a varint
-          auto decodeResult = quic::follyutils::decodeQuicInteger(cursor);
-          if (!decodeResult) {
-            XLOG(DBG6) << __func__ << " underflow";
-            connError_ = ErrorCode::PARSE_UNDERFLOW;
-            break;
-          }
-          length = decodeResult->first;
-          bytesParsed = decodeResult->second;
         }
-        curFrameLength_ = length;
-        remainingLength -= bytesParsed;
-        parseState_ = ParseState::FRAME_PAYLOAD;
         [[fallthrough]];
       }
       case ParseState::FRAME_PAYLOAD: {

moxygen/MoQCodec.h

@@ -170,6 +170,15 @@ class MoQControlCodec : public MoQCodec {
   };
   ParseState parseState_{ParseState::FRAME_HEADER_TYPE};
   bool seenSetup_{false};
+
+ private:
+  // State-machine helpers called from onIngress().
+  folly::Expected<folly::Unit, ErrorCode> parseFrameHeaderType(
+      folly::io::Cursor& cursor,
+      size_t& remainingLength);
+  folly::Expected<folly::Unit, ErrorCode> parseFrameLength(
+      folly::io::Cursor& cursor,
+      size_t& remainingLength);
 };
 
 class MoQSubNsReceiverCodec : public MoQControlCodec {