Skip to content

Commit

Permalink
Resolve comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@kghost
kghost committed Jun 10, 2022
1 parent 01d544b commit 124d909
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 47 deletions.
9 changes: 1 addition & 8 deletions src/messaging/EphemeralExchangeDispatch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,6 @@
* limitations under the License.
*/

/**
* @file
* This file defines Application Channel class. The object of this
* class can be used by CHIP data model cluster applications to send
* and receive messages. The messages are encrypted using session keys.
*/

#pragma once

#include <messaging/ExchangeMessageDispatch.h>
Expand All @@ -46,7 +39,7 @@ class EphemeralExchangeDispatch : public ExchangeMessageDispatch
{
// Only permit StandaloneAck
return (protocol == Protocols::SecureChannel::Id &&
type == static_cast<uint8_t>(Protocols::SecureChannel::MsgType::StandaloneAck));
type == to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck));
}

bool IsEncryptionRequired() const override
Expand Down
68 changes: 31 additions & 37 deletions src/messaging/ExchangeMgr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,11 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err));
SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf));
return;
}

// If rcvd msg is from initiator then this exchange is created as not Initiator.
// If rcvd msg is not from initiator then this exchange is created as Initiator.
// Note that if matchingUMH is not null then rcvd msg if from initiator.
// TODO: Figure out which channel to use for the received message
ExchangeContext * ec =
mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), delegate);
ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, false, delegate);

if (ec == nullptr)
{
Expand All @@ -300,6 +296,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const

// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY));
// No resource for creating new exchange, SendStandaloneAckIfNeeded probably also fails, so do not try it here
return;
}

Expand All @@ -310,7 +307,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
{
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE));
ec->Close();
ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf));
SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf));
return;
}

Expand All @@ -323,47 +320,44 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
return;
}

ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf));
SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf));
return;
}

void ExchangeManager::ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf)
void ExchangeManager::SendStandaloneAckIfNeeded(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
const SessionHandle & session, MessageFlags msgFlags,
System::PacketBufferHandle && msgBuf)
{
// If we need to send a StandaloneAck, create a EphemeralExchange for the purpose to send the StandaloneAck
if (payloadHeader.NeedsAck())
{
// If rcvd msg is from initiator then this exchange is created as not Initiator.
// If rcvd msg is not from initiator then this exchange is created as Initiator.
// Create a EphemeralExchange to generate a StandaloneAck
ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(),
nullptr, true /* IsEphemeralExchange */);

if (ec == nullptr)
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY));
return;
}
if (!payloadHeader.NeedsAck())
return;

ChipLogDetail(ExchangeManager, "Generating StandaloneAck via exchange: " ChipLogFormatExchange, ChipLogValueExchange(ec));
// If rcvd msg is from initiator then this exchange is created as not Initiator.
// If rcvd msg is not from initiator then this exchange is created as Initiator.
// Create a EphemeralExchange to generate a StandaloneAck
ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(),
nullptr, true /* IsEphemeralExchange */);

// Make sure the exchange stays alive through the code below even if we
// close it before calling HandleMessage.
ExchangeHandle ref(*ec);
if (ec == nullptr)
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY));
return;
}

// No need to verify packet encryption type, the EphemeralExchange can handle both secure and insecure messages.
ChipLogDetail(ExchangeManager, "Generating StandaloneAck via exchange: " ChipLogFormatExchange, ChipLogValueExchange(ec));

CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf));
if (err != CHIP_NO_ERROR)
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err));
}
// No need to verify packet encryption type, the EphemeralExchange can handle both secure and insecure messages.

// The exchange should be closed inside HandleMessage function. So don't bother close it here.
return;
CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf));
if (err != CHIP_NO_ERROR)
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err));
}

// The exchange should be closed inside HandleMessage function. So don't bother close it here.
return;
}

void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * delegate)
Expand Down
4 changes: 2 additions & 2 deletions src/messaging/ExchangeMgr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate

void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session,
DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf) override;
void ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session,
MessageFlags msgFlags, System::PacketBufferHandle && msgBuf);
void SendStandaloneAckIfNeeded(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf);
};

} // namespace Messaging
Expand Down

0 comments on commit 124d909

Please sign in to comment.