From 7defbc95bf055345fdf528ba42ac6410dd2c4041 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Fri, 3 Feb 2023 21:37:55 -0800
Subject: [PATCH] [link-metrics] simplify `HandleManagementRequest()` (#8714)

---
 src/core/thread/link_metrics.cpp | 80 +++++++++++++++++---------------
 src/core/thread/link_metrics.hpp |  2 +-
 2 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/src/core/thread/link_metrics.cpp b/src/core/thread/link_metrics.cpp
index 7fd4e081d..2547f1bf3 100644
--- a/src/core/thread/link_metrics.cpp
+++ b/src/core/thread/link_metrics.cpp
@@ -280,66 +280,70 @@ exit:
 
 Error LinkMetrics::HandleManagementRequest(const Message &aMessage, Neighbor &aNeighbor, Status &aStatus)
 {
-    Error       error = kErrorNone;
-    Tlv         tlv;
-    uint8_t     seriesId;
-    uint8_t     seriesFlagsMask;
-    EnhAckFlags enhAckFlags;
-    Metrics     metrics;
-    bool        hasForwardProbingRegistrationTlv = false;
-    bool        hasEnhAckProbingTlv              = false;
-    uint16_t    offset;
-    uint16_t    length;
-    uint16_t    index = 0;
+    Error               error = kErrorNone;
+    uint16_t            offset;
+    uint16_t            endOffset;
+    uint16_t            tlvEndOffset;
+    uint16_t            length;
+    FwdProbingRegSubTlv fwdProbingSubTlv;
+    EnhAckConfigSubTlv  enhAckConfigSubTlv;
+    Metrics             metrics;
 
     SuccessOrExit(error = Tlv::FindTlvValueOffset(aMessage, Mle::Tlv::Type::kLinkMetricsManagement, offset, length));
+    endOffset = offset + length;
 
-    while (index < length)
+    // Set sub-TLV lengths to zero to indicate that we have
+    // not yet seen them in the message.
+    fwdProbingSubTlv.SetLength(0);
+    enhAckConfigSubTlv.SetLength(0);
+
+    for (; offset < endOffset; offset = tlvEndOffset)
     {
-        uint16_t pos = offset + index;
+        Tlv      tlv;
+        uint16_t minTlvSize;
+        Tlv     *subTlv;
 
-        SuccessOrExit(aMessage.Read(pos, tlv));
+        SuccessOrExit(error = aMessage.Read(offset, tlv));
 
-        pos += sizeof(tlv);
+        VerifyOrExit(offset + tlv.GetSize() <= endOffset, error = kErrorParse);
+        tlvEndOffset = static_cast<uint16_t>(offset + tlv.GetSize());
 
         switch (tlv.GetType())
         {
         case SubTlv::kFwdProbingReg:
-            VerifyOrExit(!hasForwardProbingRegistrationTlv && !hasEnhAckProbingTlv, error = kErrorParse);
-            VerifyOrExit(tlv.GetLength() >= sizeof(seriesId) + sizeof(seriesFlagsMask), error = kErrorParse);
-            SuccessOrExit(aMessage.Read(pos, seriesId));
-            pos += sizeof(seriesId);
-            SuccessOrExit(aMessage.Read(pos, seriesFlagsMask));
-            pos += sizeof(seriesFlagsMask);
-            SuccessOrExit(error = ReadTypeIdsFromMessage(
-                              aMessage, pos, static_cast<uint16_t>(offset + index + tlv.GetSize()), metrics));
-            hasForwardProbingRegistrationTlv = true;
+            subTlv     = &fwdProbingSubTlv;
+            minTlvSize = sizeof(Tlv) + FwdProbingRegSubTlv::kMinLength;
             break;
 
         case SubTlv::kEnhAckConfig:
-            VerifyOrExit(!hasForwardProbingRegistrationTlv && !hasEnhAckProbingTlv, error = kErrorParse);
-            VerifyOrExit(tlv.GetLength() >= sizeof(EnhAckFlags), error = kErrorParse);
-            SuccessOrExit(aMessage.Read(pos, enhAckFlags));
-            pos += sizeof(enhAckFlags);
-            SuccessOrExit(error = ReadTypeIdsFromMessage(
-                              aMessage, pos, static_cast<uint16_t>(offset + index + tlv.GetSize()), metrics));
-            hasEnhAckProbingTlv = true;
+            subTlv     = &enhAckConfigSubTlv;
+            minTlvSize = sizeof(Tlv) + EnhAckConfigSubTlv::kMinLength;
             break;
 
         default:
-            break;
+            continue;
         }
 
-        index += static_cast<uint16_t>(tlv.GetSize());
+        // Ensure message contains only one sub-TLV.
+        VerifyOrExit(fwdProbingSubTlv.GetLength() == 0, error = kErrorParse);
+        VerifyOrExit(enhAckConfigSubTlv.GetLength() == 0, error = kErrorParse);
+
+        VerifyOrExit(tlv.GetSize() >= minTlvSize, error = kErrorParse);
+
+        // Read `subTlv` with its `minTlvSize`, followed by the Type IDs.
+        SuccessOrExit(error = aMessage.Read(offset, subTlv, minTlvSize));
+        SuccessOrExit(error = ReadTypeIdsFromMessage(aMessage, offset + minTlvSize, tlvEndOffset, metrics));
     }
 
-    if (hasForwardProbingRegistrationTlv)
+    if (fwdProbingSubTlv.GetLength() != 0)
     {
-        aStatus = ConfigureForwardTrackingSeries(seriesId, seriesFlagsMask, metrics, aNeighbor);
+        aStatus = ConfigureForwardTrackingSeries(fwdProbingSubTlv.GetSeriesId(), fwdProbingSubTlv.GetSeriesFlagsMask(),
+                                                 metrics, aNeighbor);
     }
-    else if (hasEnhAckProbingTlv)
+
+    if (enhAckConfigSubTlv.GetLength() != 0)
     {
-        aStatus = ConfigureEnhAckProbing(enhAckFlags, metrics, aNeighbor);
+        aStatus = ConfigureEnhAckProbing(enhAckConfigSubTlv.GetEnhAckFlags(), metrics, aNeighbor);
     }
 
 exit:
@@ -606,7 +610,7 @@ exit:
 }
 
 #if OPENTHREAD_CONFIG_MLE_LINK_METRICS_SUBJECT_ENABLE
-Status LinkMetrics::ConfigureEnhAckProbing(EnhAckFlags aEnhAckFlags, const Metrics &aMetrics, Neighbor &aNeighbor)
+Status LinkMetrics::ConfigureEnhAckProbing(uint8_t aEnhAckFlags, const Metrics &aMetrics, Neighbor &aNeighbor)
 {
     Status status = kStatusSuccess;
     Error  error  = kErrorNone;
diff --git a/src/core/thread/link_metrics.hpp b/src/core/thread/link_metrics.hpp
index 028a1cb4b..aef44320c 100644
--- a/src/core/thread/link_metrics.hpp
+++ b/src/core/thread/link_metrics.hpp
@@ -312,7 +312,7 @@ private:
                                           const Metrics &aMetrics,
                                           Neighbor      &aNeighbor);
 
-    Status ConfigureEnhAckProbing(EnhAckFlags aEnhAckFlags, const Metrics &aMetrics, Neighbor &aNeighbor);
+    Status ConfigureEnhAckProbing(uint8_t aEnhAckFlags, const Metrics &aMetrics, Neighbor &aNeighbor);
 
     Error FindNeighbor(const Ip6::Address &aDestination, Neighbor *&aNeighbor) const;