Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't prepend header in BDX messages #8234

Merged
merged 5 commits into from
Jul 16, 2021
Merged

Don't prepend header in BDX messages #8234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58c22c2
Don't prepend header in BDX messages
holbrookt Jul 8, 2021
c227eae
use Protocols::Id instead of uint32_t
holbrookt Jul 9, 2021
25c88e7
Merge branch 'master' into bdx-payload-header-8098
holbrookt Jul 12, 2021
792b9c7
Merge branch 'master' into bdx-payload-header-8098
holbrookt Jul 13, 2021
f0b3899
add PayloadHeader argument to message handler methods
holbrookt Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 33 additions & 35 deletions src/protocols/bdx/BdxTransferSession.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <protocols/secure_channel/StatusReport.h>
#include <support/BufferReader.h>
#include <support/CodeUtils.h>
#include <support/logging/CHIPLogging.h>
#include <system/SystemPacketBuffer.h>
#include <transport/SecureSessionMgr.h>

Expand Down Expand Up @@ -41,22 +42,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().ToFullyQualifiedSpecForm();
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 @@ -97,8 +93,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 @@ -163,12 +158,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 @@ -191,6 +185,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 @@ -218,8 +214,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 @@ -231,12 +226,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 @@ -245,11 +237,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 @@ -260,13 +256,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 @@ -288,9 +282,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 @@ -300,6 +291,8 @@ CHIP_ERROR TransferSession::PrepareBlock(const BlockData & inData)
mAwaitingResponse = true;
mLastBlockNum = mNextBlockNum++;

PrepareOutgoingMessageEvent(msgType, mPendingOutput, mMsgTypeData);

return CHIP_NO_ERROR;
}

Expand All @@ -316,8 +309,6 @@ CHIP_ERROR TransferSession::PrepareBlockAck()

ReturnErrorOnFailure(WriteToPacketBuffer(ackMsg, mPendingMsgHandle));

ReturnErrorOnFailure(AttachHeader(msgType, mPendingMsgHandle));

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

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

return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -768,13 +759,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 @@ -836,5 +826,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
22 changes: 18 additions & 4 deletions src/protocols/bdx/BdxTransferSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,22 @@ class DLL_EXPORT TransferSession
bool IsEof = false;
};

struct MessageTypeData
{
uint32_t ProtocolId = 0; // Should only be SecureChannel or BDX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store this as an actual protocol id, as opposed to uint32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I originally wrote this header, I ran into compilation errors when I tried using non-trivial types in the structs included in the anonymous union.

That being said, I tried doing this change today and got it to compile locally, so I'm not sure what I did wrong last time. I agree with you that it's much simpler and cleaner to use Protocols::Id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New commit has these changes

uint8_t 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 +107,7 @@ class DLL_EXPORT TransferSession
TransferAcceptData transferAcceptData;
BlockData blockdata;
StatusReportData statusData;
MessageTypeData msgTypeData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used in practice anywhere outside the unit test? I'm not obviously seeing it....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is - see PrepareOutgoingMessageEvent() in BdxTransferSession.cpp

Also this struct will be crucial to any application that uses TransferSession because it will need to pass the ProtocolId and MessageType to ExchangeContext::SendMessage()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's actually used as an arg to AttachHeaderAndSend looks like. OK, that's the part I was missing.

};

OutputEvent() : EventType(OutputEventType::kNone) { statusData = { StatusCode::kNone }; }
Expand All @@ -107,6 +118,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 +131,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 @@ -308,11 +320,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