Resolve comments

src/messaging/ApplicationExchangeDispatch.cpp

@@ -30,13 +30,13 @@ CHIP_ERROR ApplicationExchangeDispatch::PrepareMessage(SecureSessionHandle sessi
                                                        System::PacketBufferHandle && message,
                                                        EncryptedPacketBufferHandle & preparedMessage)
 {
-    return mSessionMgr->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage);
+    return mSessionMgr->BuildEncryptedMessagePayload(session, payloadHeader, std::move(message), preparedMessage);
 }
 
 CHIP_ERROR ApplicationExchangeDispatch::SendPreparedMessage(SecureSessionHandle session,
-                                                            const EncryptedPacketBufferHandle & message) const
+                                                            const EncryptedPacketBufferHandle & preparedMessage) const
 {
-    return mSessionMgr->SendPreparedMessage(session, message);
+    return mSessionMgr->SendPreparedMessage(session, preparedMessage);
 }
 
 bool ApplicationExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type)

src/messaging/ExchangeMessageDispatch.cpp

@@ -29,6 +29,7 @@
 #endif
 
 #include <inttypes.h>
+#include <memory>
 
 #include <messaging/ExchangeMessageDispatch.h>
 #include <messaging/ReliableMessageContext.h>
@@ -76,18 +77,14 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SecureSessionHandle session, uin
 
         // Add to Table for subsequent sending
         ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry));
-
-        CHIP_ERROR err = PrepareMessage(session, payloadHeader, std::move(message), entry->retainedBuf);
-        if (err != CHIP_NO_ERROR)
-        {
-            // Remove from table
-            ChipLogError(ExchangeManager, "Failed to send message with err %s", ::chip::ErrorStr(err));
-            reliableMessageMgr->ClearRetransTable(*entry);
-            return err;
-        }
-
-        ReturnErrorOnFailure(SendPreparedMessage(session, entry->retainedBuf));
-        reliableMessageMgr->StartRetransmision(entry);
+        auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) {
+            reliableMessageMgr->ClearRetransTable(*e);
+        };
+        std::unique_ptr<ReliableMessageMgr::RetransTableEntry, decltype(deleter)> entryOwner(entry, deleter);
+
+        ReturnErrorOnFailure(PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf));
+        ReturnErrorOnFailure(SendPreparedMessage(session, entryOwner->retainedBuf));
+        reliableMessageMgr->StartRetransmision(entryOwner.release());
     }
     else
     {

src/messaging/ExchangeMessageDispatch.h

@@ -44,9 +44,19 @@ class ExchangeMessageDispatch : public ReferenceCounted<ExchangeMessageDispatch>
                            ReliableMessageContext * reliableMessageContext, bool isReliableTransmission, Protocols::Id protocol,
                            uint8_t type, System::PacketBufferHandle && message);
 
+    /**
+     * @brief
+     *   This interface takes the payload and returns the prepared message which can be send multiple times.
+     *
+     * @param session         Peer node to which the payload to be sent
+     * @param payloadHeader   The payloadHeader to be encoded into the packet
+     * @param message         The payload to be sent
+     * @param preparedMessage The handle to hold the prepared message
+     */
     virtual CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader,
                                       System::PacketBufferHandle && message, EncryptedPacketBufferHandle & preparedMessage) = 0;
-    virtual CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const  = 0;
+    virtual CHIP_ERROR SendPreparedMessage(SecureSessionHandle session,
+                                           const EncryptedPacketBufferHandle & preparedMessage) const                       = 0;
 
     virtual CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId,
                                          const Transport::PeerAddress & peerAddress,

src/messaging/tests/TestReliableMessageProtocol.cpp

@@ -129,9 +129,9 @@ class MockSessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessa
         return CHIP_NO_ERROR;
     }
 
-    CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const override
+    CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & preparedMessage) const override
     {
-        return gTransportMgr.SendMessage(Transport::PeerAddress(), message.Retain());
+        return gTransportMgr.SendMessage(Transport::PeerAddress(), preparedMessage.CastToWritable());
     }
 
     bool IsReliableTransmissionAllowed() const override { return mRetainMessageOnSend; }

src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp

@@ -41,10 +41,10 @@ CHIP_ERROR SessionEstablishmentExchangeDispatch::PrepareMessage(SecureSessionHan
 }
 
 CHIP_ERROR SessionEstablishmentExchangeDispatch::SendPreparedMessage(SecureSessionHandle session,
-                                                                     const EncryptedPacketBufferHandle & message) const
+                                                                     const EncryptedPacketBufferHandle & preparedMessage) const
 {
     ReturnErrorCodeIf(mTransportMgr == nullptr, CHIP_ERROR_INCORRECT_STATE);
-    return mTransportMgr->SendMessage(mPeerAddress, message.Retain());
+    return mTransportMgr->SendMessage(mPeerAddress, preparedMessage.CastToWritable());
 }
 
 CHIP_ERROR SessionEstablishmentExchangeDispatch::OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId,

src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h

@@ -45,7 +45,7 @@ class SessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessageDi
 
     CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && message,
                               EncryptedPacketBufferHandle & out) override;
-    CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const override;
+    CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & preparedMessage) const override;
 
     CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId,
                                  const Transport::PeerAddress & peerAddress,

src/transport/SecureSessionMgr.cpp

@@ -122,8 +122,9 @@ Transport::Type SecureSessionMgr::GetTransportType(NodeId peerNodeId)
     return Transport::Type::kUndefined;
 }
 
-CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader,
-                                            System::PacketBufferHandle && msgBuf, EncryptedPacketBufferHandle & preparedMessage)
+CHIP_ERROR SecureSessionMgr::BuildEncryptedMessagePayload(SecureSessionHandle session, PayloadHeader & payloadHeader,
+                                                          System::PacketBufferHandle && msgBuf,
+                                                          EncryptedPacketBufferHandle & encryptedMessage)
 {
     PacketHeader packetHeader;
     if (IsControlMessage(payloadHeader))
@@ -135,7 +136,7 @@ CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, Payload
     if (state == nullptr)
     {
         return CHIP_ERROR_NOT_CONNECTED;
-    };
+    }
 
     Transport::AdminPairingInfo * admin = mAdmins->FindAdminWithId(state->GetAdminId());
     if (admin == nullptr)
@@ -149,8 +150,8 @@ CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, Payload
 
     ReturnErrorOnFailure(packetHeader.EncodeBeforeData(msgBuf));
 
-    preparedMessage = EncryptedPacketBufferHandle::MarkEncrypted(std::move(msgBuf));
-    ChipLogProgress(Inet, "Prepared msg %p from 0x" ChipLogFormatX64 " to 0x" ChipLogFormatX64 ".", &preparedMessage,
+    encryptedMessage = EncryptedPacketBufferHandle::MarkEncrypted(std::move(msgBuf));
+    ChipLogProgress(Inet, "Prepared msg %p from 0x" ChipLogFormatX64 " to 0x" ChipLogFormatX64 ".", &encryptedMessage,
                     ChipLogValueX64(localNodeId), ChipLogValueX64(state->GetPeerNodeId()));
 
     return CHIP_NO_ERROR;
@@ -164,7 +165,7 @@ CHIP_ERROR SecureSessionMgr::SendPreparedMessage(SecureSessionHandle session, co
 
     VerifyOrExit(mState == State::kInitialized, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(!preparedMessage.IsNull(), err = CHIP_ERROR_INVALID_ARGUMENT);
-    msgBuf = preparedMessage.Retain();
+    msgBuf = preparedMessage.CastToWritable();
     VerifyOrExit(!msgBuf.IsNull(), err = CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrExit(!msgBuf->HasChainedBuffer(), err = CHIP_ERROR_INVALID_MESSAGE_LENGTH);
 

src/transport/SecureSessionMgr.h

@@ -68,8 +68,6 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle
 
     uint32_t GetMsgId() const;
 
-    PacketBufferHandle Retain() const { return PacketBufferHandle::Retain(); }
-
     /**
      * Creates a copy of the data in this packet.
      *
@@ -108,11 +106,8 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle
      * Get a handle to the data that allows mutating the bytes.  This should
      * only be used if absolutely necessary, because EncryptedPacketBufferHandle
      * represents a buffer that we want to resend as-is.
-     *
-     * We only allow doing this with an rvalue reference, so the fact that we
-     * are moving out of the EncryptedPacketBufferHandle is clear.
      */
-    PacketBufferHandle CastToWritable() && { return PacketBufferHandle(std::move(*this)); }
+    PacketBufferHandle CastToWritable() const { return PacketBufferHandle::Retain(); }
 
 private:
     EncryptedPacketBufferHandle(PacketBufferHandle && aBuffer) : PacketBufferHandle(std::move(aBuffer)) {}
@@ -184,17 +179,17 @@ class DLL_EXPORT SecureSessionMgr : public TransportMgrDelegate
 
     /**
      * @brief
-     *   This function takes the payload and returns the final message which can be send multiple times.
+     *   This function takes the payload and returns encrypted message which can be sent multiple times.
      *
      * @details
      *   It contains following preparation:
      *    1. Encrypt the msgBuf
      *    2. construct the packet header
      *    3. Encode the packet header and prepend it to message.
-     *   Returns a prepared message in preparedMessage.
+     *   Returns a encrypted message in encryptedMessage.
      */
-    CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && msgBuf,
-                              EncryptedPacketBufferHandle & preparedMessage);
+    CHIP_ERROR BuildEncryptedMessagePayload(SecureSessionHandle session, PayloadHeader & payloadHeader,
+                                            System::PacketBufferHandle && msgBuf, EncryptedPacketBufferHandle & encryptedMessage);
 
     /**
      * @brief

src/transport/tests/TestSecureSessionMgr.cpp

@@ -221,7 +221,7 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext)
     payloadHeader.SetMessageType(chip::Protocols::Echo::MsgType::EchoRequest);
 
     EncryptedPacketBufferHandle preparedMessage;
-    err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
+    err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage);
@@ -235,7 +235,8 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext)
 
     callback.LargeMessageSent = true;
 
-    err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(large_buffer), preparedMessage);
+    err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(large_buffer),
+                                                        preparedMessage);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage);
@@ -251,7 +252,8 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext)
 
     callback.LargeMessageSent = true;
 
-    err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(extra_large_buffer), preparedMessage);
+    err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(extra_large_buffer),
+                                                        preparedMessage);
     NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_MESSAGE_TOO_LONG);
 }
 
@@ -319,7 +321,7 @@ void SendEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
 
     payloadHeader.SetInitiator(true);
 
-    err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
+    err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage);
@@ -401,7 +403,7 @@ void SendBadEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
 
     payloadHeader.SetInitiator(true);
 
-    err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
+    err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage);