Skip to content

Commit

Permalink
Revise PacketBufferWriter interface (#4874)
Browse files Browse the repository at this point in the history
* Revise PacketBufferWriter interface

#### Problem

The `PacketBufferWriter` constructor that takes a size and allocates the
`PacketBuffer` internally is awkward for some cases, and useless for the
case of an existing `PacketBufferHandle`.

#### Summary of Changes

- `PacketBufferWriter()` takes ownership of an existing `PacketBufferHandle`
  (same as `PacketBufferTLVWriter`).
- Added a `Finalize()` overload that matches that of `PacketBufferTLVWriter`.

* * fix doxygen

* * review

* * review 2
  • Loading branch information
kpschoedel authored and pull[bot] committed Mar 2, 2021
1 parent 6c3548a commit 1081180
Show file tree
Hide file tree
Showing 10 changed files with 30 additions and 35 deletions.
2 changes: 1 addition & 1 deletion src/app/encoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
#define COMMAND_HEADER(name, clusterId) \
const char * kName = name; \
\
PacketBufferWriter buf(kMaxBufferSize); \
PacketBufferWriter buf(System::PacketBufferHandle::New(kMaxBufferSize)); \
if (buf.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
Expand Down
2 changes: 1 addition & 1 deletion src/app/zap-templates/templates/chip/encoder-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
#define COMMAND_HEADER(name, clusterId) \
const char * kName = name; \
\
PacketBufferWriter buf(kMaxBufferSize); \
PacketBufferWriter buf(System::PacketBufferHandle::New(kMaxBufferSize)); \
if (buf.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/bdx/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ static_library("bdx") {
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
"${chip_root}/src/system",
"${chip_root}/src/transport/raw",
"${chip_root}/src/transport",
]
}
6 changes: 3 additions & 3 deletions src/protocols/bdx/BdxTransferSession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <support/CodeUtils.h>
#include <support/ReturnMacros.h>
#include <system/SystemPacketBuffer.h>
#include <transport/SecureSessionMgr.h>

namespace {
constexpr uint8_t kBdxVersion = 0; ///< The version of this implementation of the BDX spec
Expand All @@ -25,7 +26,7 @@ constexpr size_t kStatusReportMinSize = 2 + 4 + 2; ///< 16 bits for GeneralCode,
CHIP_ERROR WriteToPacketBuffer(const ::chip::bdx::BdxMessage & msgStruct, ::chip::System::PacketBufferHandle & msgBuf)
{
size_t msgDataSize = msgStruct.MessageSize();
::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(msgDataSize);
::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(chip::MessagePacketBuffer::New(msgDataSize), msgDataSize);
if (bbuf.IsNull())
{
return CHIP_ERROR_NO_MEMORY;
Expand All @@ -36,7 +37,6 @@ CHIP_ERROR WriteToPacketBuffer(const ::chip::bdx::BdxMessage & msgStruct, ::chip
{
return CHIP_ERROR_NO_MEMORY;
}

return CHIP_NO_ERROR;
}

Expand Down Expand Up @@ -848,7 +848,7 @@ void TransferSession::PrepareStatusReport(StatusCode code)
{
mStatusReportData.StatusCode = code;

Encoding::LittleEndian::PacketBufferWriter bbuf(kStatusReportMinSize);
Encoding::LittleEndian::PacketBufferWriter bbuf(chip::MessagePacketBuffer::New(kStatusReportMinSize), kStatusReportMinSize);
VerifyOrReturn(!bbuf.IsNull());

bbuf.Put16(static_cast<uint16_t>(Protocols::Common::StatusCode::Failure));
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/bdx/tests/TestBdxMessages.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ void TestHelperWrittenAndParsedMatch(nlTestSuite * inSuite, void * inContext, Ms
CHIP_ERROR err = CHIP_NO_ERROR;

size_t msgSize = testMsg.MessageSize();
Encoding::LittleEndian::PacketBufferWriter bbuf(msgSize);
Encoding::LittleEndian::PacketBufferWriter bbuf(System::PacketBufferHandle::New(msgSize));
NL_TEST_ASSERT(inSuite, !bbuf.IsNull());

testMsg.WriteToBuffer(bbuf);
Expand Down
10 changes: 0 additions & 10 deletions src/system/SystemPacketBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -619,16 +619,6 @@ PacketBufferHandle PacketBufferHandle::CloneData(uint16_t aAdditionalSize, uint1

namespace Encoding {

void PacketBufferWriterUtil::Initialize(BufferWriter & aBufferWriter, System::PacketBufferHandle & aPacket, size_t aAvailableSize,
uint16_t aReservedSize)
{
aPacket = System::PacketBufferHandle::New(aAvailableSize, aReservedSize);
if (!aPacket.IsNull())
{
aBufferWriter = Encoding::BufferWriter(aPacket->Start(), aAvailableSize);
}
}

System::PacketBufferHandle PacketBufferWriterUtil::Finalize(BufferWriter & aBufferWriter, System::PacketBufferHandle & aPacket)
{
if (!aPacket.IsNull() && aBufferWriter.Fit())
Expand Down
29 changes: 17 additions & 12 deletions src/system/SystemPacketBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include <system/SystemError.h>

#include <stddef.h>
#include <utility>

#if CHIP_SYSTEM_CONFIG_USE_LWIP
#include <lwip/mem.h>
Expand All @@ -60,7 +61,6 @@ class PacketBufferTest;

#undef CHIP_SYSTEM_PACKETBUFFER_HAS_RIGHT_SIZE // True if RightSize() has a nontrivial implementation
#undef CHIP_SYSTEM_PACKETBUFFER_HAS_CHECK // True if Check() has a nontrivial implementation

#if CHIP_SYSTEM_CONFIG_USE_LWIP
#if LWIP_PBUF_FROM_CUSTOM_POOLS
#define CHIP_SYSTEM_PACKETBUFFER_STORE CHIP_SYSTEM_PACKETBUFFER_STORE_LWIP_CUSTOM
Expand Down Expand Up @@ -697,8 +697,6 @@ class PacketBufferWriterUtil
private:
template <typename>
friend class PacketBufferWriterBase;
static void Initialize(BufferWriter & aBufferWriter, System::PacketBufferHandle & aPacket, size_t aAvailableSize,
uint16_t aReservedSize);
static System::PacketBufferHandle Finalize(BufferWriter & aBufferWriter, System::PacketBufferHandle & aPacket);
};

Expand All @@ -721,19 +719,26 @@ class PacketBufferWriterBase : public Writer
{
public:
/**
* Constructs a BufferWriter that writes into a newly allocated packet buffer.
* Constructs a BufferWriter that writes into a packet buffer, using all avaiable space.
*
* If no memory is available, or if the size requested is too large, then \c IsNull() will be true.
* Otherwise, it is guaranteed that the BufferWriter length is \a aAvailableSize. (The underlying packet
* buffer may be larger.)
* @param[in] aPacket A handle to PacketBuffer, to be used as backing store for the BufferWriter.
*/
PacketBufferWriterBase(System::PacketBufferHandle && aPacket) :
Writer(aPacket->Start() + aPacket->DataLength(), aPacket->AvailableDataLength())
{
mPacket = std::move(aPacket);
}

/**
* Constructs a BufferWriter that writes into a packet buffer, using no more than the requested space.
*
* @param[in] aAvailableSize Length bound of the BufferWriter.
* @param[in] aReservedSize Reserved packet buffer space for protocol headers; see \c PacketBufferHandle::New().
* @param[in] aPacket A handle to PacketBuffer, to be used as backing store for the BufferWriter.
* @param[in] aSize Maximum number of octects to write into the packet buffer.
*/
PacketBufferWriterBase(size_t aAvailableSize, uint16_t aReservedSize = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE) :
Writer(nullptr, 0)
PacketBufferWriterBase(System::PacketBufferHandle && aPacket, size_t aSize) :
Writer(aPacket->Start() + aPacket->DataLength(), chip::min(aSize, static_cast<size_t>(aPacket->AvailableDataLength())))
{
PacketBufferWriterUtil::Initialize(*this, mPacket, aAvailableSize, aReservedSize);
mPacket = std::move(aPacket);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/system/tests/TestSystemPacketBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1715,8 +1715,8 @@ void PacketBufferTest::CheckPacketBufferWriter(nlTestSuite * inSuite, void * inC

const char kPayload[] = "Hello, world!";

PacketBufferWriter yay(sizeof(kPayload));
PacketBufferWriter nay(sizeof(kPayload) - 2);
PacketBufferWriter yay(PacketBufferHandle::New(sizeof(kPayload)));
PacketBufferWriter nay(PacketBufferHandle::New(sizeof(kPayload)), sizeof(kPayload) - 2);
NL_TEST_ASSERT(inSuite, !yay.IsNull());
NL_TEST_ASSERT(inSuite, !nay.IsNull());

Expand Down
4 changes: 2 additions & 2 deletions src/transport/NetworkProvisioning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ CHIP_ERROR NetworkProvisioning::SendNetworkCredentials(const char * ssid, const
const size_t bufferSize = EncodedStringSize(ssid) + EncodedStringSize(passwd);
VerifyOrExit(CanCastTo<uint16_t>(bufferSize), err = CHIP_ERROR_INVALID_ARGUMENT);
{
Encoding::LittleEndian::PacketBufferWriter bbuf(bufferSize + MessagePacketBuffer::kMaxFooterSize);
Encoding::LittleEndian::PacketBufferWriter bbuf(MessagePacketBuffer::New(bufferSize), bufferSize);

ChipLogProgress(NetworkProvisioning, "Sending Network Creds. Delegate %p\n", mDelegate);
VerifyOrExit(mDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
Expand Down Expand Up @@ -264,7 +264,7 @@ CHIP_ERROR NetworkProvisioning::SendThreadCredentials(const DeviceLayer::Interna
4; // threadData.ThreadChannel, threadData.FieldPresent.ThreadExtendedPANId,
// threadData.FieldPresent.ThreadMeshPrefix, threadData.FieldPresent.ThreadPSKc
/* clang-format on */
Encoding::LittleEndian::PacketBufferWriter bbuf(credentialSize + MessagePacketBuffer::kMaxFooterSize);
Encoding::LittleEndian::PacketBufferWriter bbuf(MessagePacketBuffer::New(credentialSize), credentialSize);

ChipLogProgress(NetworkProvisioning, "Sending Thread Credentials");
VerifyOrExit(mDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
Expand Down
4 changes: 2 additions & 2 deletions src/transport/PASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const PacketHeader & header, con
data_len = static_cast<uint16_t>(Y_len + verifier_len);

{
Encoding::PacketBufferWriter bbuf(data_len);
Encoding::PacketBufferWriter bbuf(System::PacketBufferHandle::New(data_len));
VerifyOrExit(!bbuf.IsNull(), err = CHIP_SYSTEM_ERROR_NO_MEMORY);
bbuf.Put(&Y[0], Y_len);
bbuf.Put(verifier, verifier_len);
Expand Down Expand Up @@ -691,7 +691,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const PacketHeader & header, con
}

{
Encoding::PacketBufferWriter bbuf(verifier_len);
Encoding::PacketBufferWriter bbuf(System::PacketBufferHandle::New(verifier_len));
VerifyOrExit(!bbuf.IsNull(), err = CHIP_SYSTEM_ERROR_NO_MEMORY);

bbuf.Put(verifier, verifier_len);
Expand Down

0 comments on commit 1081180

Please sign in to comment.