Skip to content

Commit

Permalink
Don't prepend header in BDX messages (project-chip#8234)
Browse files Browse the repository at this point in the history 
* Don't prepend header in BDX messages

- remove code in BdxTransferSession that prepended PayloadHeaders to
outgoing messages
- add MessageTypeData to convey message type and protocol

* use Protocols::Id instead of uint32_t

* add PayloadHeader argument to message handler methods

- ExchangeDelegate already consumes PayloadHeader from the message, so
this needs to be removed from HandleMessageReceived()
  • Loading branch information
@holbrookt
holbrookt authored and Nikita committed Sep 23, 2021
1 parent 0933aae commit 26f95a9
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 86 deletions.
78 changes: 37 additions & 41 deletions src/protocols/bdx/BdxTransferSession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <support/BufferReader.h>
#include <support/CodeUtils.h>
#include <support/TypeTraits.h>
#include <support/logging/CHIPLogging.h>
#include <system/SystemPacketBuffer.h>
#include <transport/SecureSessionMgr.h>

Expand Down Expand Up @@ -42,22 +43,17 @@ CHIP_ERROR WriteToPacketBuffer(const ::chip::bdx::BdxMessage & msgStruct, ::chip
return CHIP_NO_ERROR;
}

// We could make this whole method a template, but it's probably smaller code to
// share the implementation across all message types.
CHIP_ERROR AttachHeader(chip::Protocols::Id protocolId, uint8_t msgType, ::chip::System::PacketBufferHandle & msgBuf)
template <typename MessageType>
void PrepareOutgoingMessageEvent(MessageType messageType, chip::bdx::TransferSession::OutputEventType & pendingOutput,
chip::bdx::TransferSession::MessageTypeData & outputMsgType)
{
::chip::PayloadHeader payloadHeader;

payloadHeader.SetMessageType(protocolId, msgType);
static_assert(std::is_same<std::underlying_type_t<decltype(messageType)>, uint8_t>::value, "Cast is not safe");

return payloadHeader.EncodeBeforeData(msgBuf);
pendingOutput = chip::bdx::TransferSession::OutputEventType::kMsgToSend;
outputMsgType.ProtocolId = chip::Protocols::MessageTypeTraits<MessageType>::ProtocolId();
outputMsgType.MessageType = static_cast<uint8_t>(messageType);
}

template <typename MessageType>
inline CHIP_ERROR AttachHeader(MessageType msgType, ::chip::System::PacketBufferHandle & msgBuf)
{
return AttachHeader(chip::Protocols::MessageTypeTraits<MessageType>::ProtocolId(), static_cast<uint8_t>(msgType), msgBuf);
}
} // anonymous namespace

namespace chip {
Expand Down Expand Up @@ -98,8 +94,7 @@ void TransferSession::PollOutput(OutputEvent & event, uint64_t curTimeMs)
event = OutputEvent::StatusReportEvent(OutputEventType::kStatusReceived, mStatusReportData);
break;
case OutputEventType::kMsgToSend:
event = OutputEvent(OutputEventType::kMsgToSend);
event.MsgData = std::move(mPendingMsgHandle);
event = OutputEvent::MsgToSendEvent(mMsgTypeData, std::move(mPendingMsgHandle));
mTimeoutStartTimeMs = curTimeMs;
break;
case OutputEventType::kInitReceived:
Expand Down Expand Up @@ -164,12 +159,11 @@ CHIP_ERROR TransferSession::StartTransfer(TransferRole role, const TransferInitD
ReturnErrorOnFailure(WriteToPacketBuffer(initMsg, mPendingMsgHandle));

const MessageType msgType = (mRole == TransferRole::kSender) ? MessageType::SendInit : MessageType::ReceiveInit;
ReturnErrorOnFailure(AttachHeader(msgType, mPendingMsgHandle));

mState = TransferState::kAwaitingAccept;
mAwaitingResponse = true;

mPendingOutput = OutputEventType::kMsgToSend;
PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}
Expand All @@ -192,6 +186,8 @@ CHIP_ERROR TransferSession::WaitForTransfer(TransferRole role, BitFlags<Transfer

CHIP_ERROR TransferSession::AcceptTransfer(const TransferAcceptData & acceptData)
{
MessageType msgType;

const BitFlags<TransferControlFlags> proposedControlOpts(mTransferRequestData.TransferCtlFlags);

VerifyOrReturnError(mState == TransferState::kNegotiateTransferParams, CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -219,8 +215,7 @@ CHIP_ERROR TransferSession::AcceptTransfer(const TransferAcceptData & acceptData
acceptMsg.MetadataLength = acceptData.MetadataLength;

ReturnErrorOnFailure(WriteToPacketBuffer(acceptMsg, mPendingMsgHandle));

ReturnErrorOnFailure(AttachHeader(MessageType::ReceiveAccept, mPendingMsgHandle));
msgType = MessageType::ReceiveAccept;
}
else
{
Expand All @@ -232,12 +227,9 @@ CHIP_ERROR TransferSession::AcceptTransfer(const TransferAcceptData & acceptData
acceptMsg.MetadataLength = acceptData.MetadataLength;

ReturnErrorOnFailure(WriteToPacketBuffer(acceptMsg, mPendingMsgHandle));

ReturnErrorOnFailure(AttachHeader(MessageType::SendAccept, mPendingMsgHandle));
msgType = MessageType::SendAccept;
}

mPendingOutput = OutputEventType::kMsgToSend;

mState = TransferState::kTransferInProgress;

if ((mRole == TransferRole::kReceiver && mControlMode == TransferControlFlags::kSenderDrive) ||
Expand All @@ -246,11 +238,15 @@ CHIP_ERROR TransferSession::AcceptTransfer(const TransferAcceptData & acceptData
mAwaitingResponse = true;
}

PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}

CHIP_ERROR TransferSession::PrepareBlockQuery()
{
const MessageType msgType = MessageType::BlockQuery;

VerifyOrReturnError(mState == TransferState::kTransferInProgress, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mRole == TransferRole::kReceiver, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mPendingOutput == OutputEventType::kNone, CHIP_ERROR_INCORRECT_STATE);
Expand All @@ -261,13 +257,11 @@ CHIP_ERROR TransferSession::PrepareBlockQuery()

ReturnErrorOnFailure(WriteToPacketBuffer(queryMsg, mPendingMsgHandle));

ReturnErrorOnFailure(AttachHeader(MessageType::BlockQuery, mPendingMsgHandle));

mPendingOutput = OutputEventType::kMsgToSend;

mAwaitingResponse = true;
mLastQueryNum = mNextQueryNum++;

PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}

Expand All @@ -289,9 +283,6 @@ CHIP_ERROR TransferSession::PrepareBlock(const BlockData & inData)
ReturnErrorOnFailure(WriteToPacketBuffer(blockMsg, mPendingMsgHandle));

const MessageType msgType = inData.IsEof ? MessageType::BlockEOF : MessageType::Block;
ReturnErrorOnFailure(AttachHeader(msgType, mPendingMsgHandle));

mPendingOutput = OutputEventType::kMsgToSend;

if (msgType == MessageType::BlockEOF)
{
Expand All @@ -301,6 +292,8 @@ CHIP_ERROR TransferSession::PrepareBlock(const BlockData & inData)
mAwaitingResponse = true;
mLastBlockNum = mNextBlockNum++;

PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}

Expand All @@ -317,8 +310,6 @@ CHIP_ERROR TransferSession::PrepareBlockAck()

ReturnErrorOnFailure(WriteToPacketBuffer(ackMsg, mPendingMsgHandle));

ReturnErrorOnFailure(AttachHeader(msgType, mPendingMsgHandle));

if (mState == TransferState::kTransferInProgress)
{
if (mControlMode == TransferControlFlags::kSenderDrive)
Expand All @@ -335,7 +326,7 @@ CHIP_ERROR TransferSession::PrepareBlockAck()
mAwaitingResponse = false;
}

mPendingOutput = OutputEventType::kMsgToSend;
PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -376,13 +367,11 @@ void TransferSession::Reset()
mAwaitingResponse = false;
}

CHIP_ERROR TransferSession::HandleMessageReceived(System::PacketBufferHandle msg, uint64_t curTimeMs)
CHIP_ERROR TransferSession::HandleMessageReceived(const PayloadHeader & payloadHeader, System::PacketBufferHandle msg,
uint64_t curTimeMs)
{
VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);

PayloadHeader payloadHeader;
ReturnErrorOnFailure(payloadHeader.DecodeAndConsume(msg));

if (payloadHeader.HasProtocol(Protocols::BDX::Id))
{
ReturnErrorOnFailure(HandleBdxMessage(payloadHeader, std::move(msg)));
Expand All @@ -402,7 +391,7 @@ CHIP_ERROR TransferSession::HandleMessageReceived(System::PacketBufferHandle msg
}

// Return CHIP_ERROR only if there was a problem decoding the message. Otherwise, call PrepareStatusReport().
CHIP_ERROR TransferSession::HandleBdxMessage(PayloadHeader & header, System::PacketBufferHandle msg)
CHIP_ERROR TransferSession::HandleBdxMessage(const PayloadHeader & header, System::PacketBufferHandle msg)
{
VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(mPendingOutput == OutputEventType::kNone, CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -450,7 +439,7 @@ CHIP_ERROR TransferSession::HandleBdxMessage(PayloadHeader & header, System::Pac
* NOTE: BDX does not currently expect to ever use a "Success" general code, so it will be treated as an error along with any
* other code.
*/
CHIP_ERROR TransferSession::HandleStatusReportMessage(PayloadHeader & header, System::PacketBufferHandle msg)
CHIP_ERROR TransferSession::HandleStatusReportMessage(const PayloadHeader & header, System::PacketBufferHandle msg)
{
VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);

Expand Down Expand Up @@ -767,13 +756,12 @@ void TransferSession::PrepareStatusReport(StatusCode code)
mPendingMsgHandle = bbuf.Finalize();
if (mPendingMsgHandle.IsNull())
{
ChipLogError(BDX, "%s: error preparing message: %s", __FUNCTION__, ErrorStr(CHIP_ERROR_NO_MEMORY));
mPendingOutput = OutputEventType::kInternalError;
}
else
{
CHIP_ERROR err = AttachHeader(Protocols::SecureChannel::MsgType::StatusReport, mPendingMsgHandle);
VerifyOrExit(err == CHIP_NO_ERROR, mPendingOutput = OutputEventType::kInternalError);
mPendingOutput = OutputEventType::kMsgToSend;
PrepareOutgoingMessageEvent(Protocols::SecureChannel::MsgType::StatusReport, mPendingOutput, mMsgTypeData);
}

exit:
Expand Down Expand Up @@ -835,5 +823,13 @@ TransferSession::OutputEvent TransferSession::OutputEvent::StatusReportEvent(Out
return event;
}

TransferSession::OutputEvent TransferSession::OutputEvent::MsgToSendEvent(MessageTypeData typeData, System::PacketBufferHandle msg)
{
OutputEvent event(OutputEventType::kMsgToSend);
event.MsgData = std::move(msg);
event.msgTypeData = typeData;
return event;
}

} // namespace bdx
} // namespace chip
36 changes: 27 additions & 9 deletions src/protocols/bdx/BdxTransferSession.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,24 @@ class DLL_EXPORT TransferSession
bool IsEof = false;
};

struct MessageTypeData
{
Protocols::Id ProtocolId; // Should only ever be SecureChannel or BDX
uint8_t MessageType;

MessageTypeData() : ProtocolId(Protocols::NotSpecified), MessageType(0) {}
};

/**
* @brief
* All output data processed by the TransferSession object will be passed to the caller using this struct via PollOutput().
*
* NOTE: Some sub-structs may contain pointers to data in a PacketBuffer. In this case, the MsgData field MUST be populated
* with a PacketBufferHandle that encapsulates the respective PacketBuffer, in order to ensure valid memory access.
* NOTE: Some sub-structs may contain pointers to data in a PacketBuffer (see Blockdata). In this case, the MsgData field MUST
* be populated with a PacketBufferHandle that encapsulates the respective PacketBuffer, in order to ensure valid memory
* access.
*
* NOTE: MsgData can contain messages that have been received or messages that should be sent by the caller. The underlying
* buffer will always start at the data, never at the payload header. Outgoing messages do not have a header prepended.
*/
struct OutputEvent
{
Expand All @@ -97,6 +109,7 @@ class DLL_EXPORT TransferSession
TransferAcceptData transferAcceptData;
BlockData blockdata;
StatusReportData statusData;
MessageTypeData msgTypeData;
};

OutputEvent() : EventType(OutputEventType::kNone) { statusData = { StatusCode::kNone }; }
Expand All @@ -107,6 +120,7 @@ class DLL_EXPORT TransferSession
static OutputEvent TransferAcceptEvent(TransferAcceptData data, System::PacketBufferHandle msg);
static OutputEvent BlockDataEvent(BlockData data, System::PacketBufferHandle msg);
static OutputEvent StatusReportEvent(OutputEventType type, StatusReportData data);
static OutputEvent MsgToSendEvent(MessageTypeData typeData, System::PacketBufferHandle msg);
};

/**
Expand All @@ -119,8 +133,8 @@ class DLL_EXPORT TransferSession
* It is possible that consecutive calls to this method may emit different outputs depending on the state of the
* TransferSession object.
*
* Note that if the type outputted is kMsgToSend, it is assumed that the message will be send immediately, and the
* session timeout timer will begin at curTimeMs.
* Note that if the type outputted is kMsgToSend, the caller is expected to send the message immediately, and the session
* timeout timer will begin at curTimeMs.
*
* See OutputEventType for all possible output event types.
*
Expand Down Expand Up @@ -236,13 +250,15 @@ class DLL_EXPORT TransferSession
* @brief
* Process a message intended for this TransferSession object.
*
* @param msg A PacketBufferHandle pointing to the message buffer to process. May be BDX or StatusReport protocol.
* @param curTimeMs Current time indicated by the number of milliseconds since some epoch defined by the platform
* @param payloadHeader A PayloadHeader containing the Protocol type and Message Type
* @param msg A PacketBufferHandle pointing to the message buffer to process. May be BDX or StatusReport protocol.
* Buffer is expected to start at data (not header).
* @param curTimeMs Current time indicated by the number of milliseconds since some epoch defined by the platform
*
* @return CHIP_ERROR Indicates any problems in decoding the message, or if the message is not of the BDX or StatusReport
* protocols.
*/
CHIP_ERROR HandleMessageReceived(System::PacketBufferHandle msg, uint64_t curTimeMs);
CHIP_ERROR HandleMessageReceived(const PayloadHeader & payloadHeader, System::PacketBufferHandle msg, uint64_t curTimeMs);

TransferControlFlags GetControlMode() const { return mControlMode; }
uint64_t GetStartOffset() const { return mStartOffset; }
Expand All @@ -266,8 +282,8 @@ class DLL_EXPORT TransferSession
};

// Incoming message handlers
CHIP_ERROR HandleBdxMessage(PayloadHeader & header, System::PacketBufferHandle msg);
CHIP_ERROR HandleStatusReportMessage(PayloadHeader & header, System::PacketBufferHandle msg);
CHIP_ERROR HandleBdxMessage(const PayloadHeader & header, System::PacketBufferHandle msg);
CHIP_ERROR HandleStatusReportMessage(const PayloadHeader & header, System::PacketBufferHandle msg);
void HandleTransferInit(MessageType msgType, System::PacketBufferHandle msgData);
void HandleReceiveAccept(System::PacketBufferHandle msgData);
void HandleSendAccept(System::PacketBufferHandle msgData);
Expand Down Expand Up @@ -308,11 +324,13 @@ class DLL_EXPORT TransferSession
uint64_t mTransferLength = 0; ///< 0 represents indefinite length
uint16_t mTransferMaxBlockSize = 0;

// Used to store event data before it is emitted via PollOutput()
System::PacketBufferHandle mPendingMsgHandle;
StatusReportData mStatusReportData;
TransferInitData mTransferRequestData;
TransferAcceptData mTransferAcceptData;
BlockData mBlockEventData;
MessageTypeData mMsgTypeData;

uint32_t mNumBytesProcessed = 0;

Expand Down
Loading

0 comments on commit 26f95a9

Please sign in to comment.