Skip to content

Commit

Permalink
SystemPacketBuffer.h cleanup
Browse files Browse the repository at this point in the history
#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
Some existing methods should be removed or made private so that code
does not have unnecessary access to the raw pointer.

#### Summary of Changes

- convert public use of `PacketBufferHandle::Adopt()`.
- convert internal use of `Adopt()` to private `Free()`.
- renamed `PacketBuffer::Create()` factory to `Adopt()`.
- rename private `PacketBuffer::Next()` to `ChainedBuffer()`.
- explicity note `PacketBuffer::Next_ForNow()` remaining use in TLV.
- make `PacketBuffer::Free()` private.
- make `PacketBuffer::Consume()` private.
- make `PacketBuffer::AddToEnd_ForNow()` private (still used in tests).
- remove `PacketBuffer::FreeHead_ForNow()`.
- remove `operator*`.

Part of issue project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system
  • Loading branch information
kpschoedel committed Jan 8, 2021
1 parent f77bebf commit cbd36d6
Show file tree
Hide file tree
Showing 16 changed files with 62 additions and 69 deletions.
32 changes: 16 additions & 16 deletions src/app/tests/TestMessageDef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ void AttributePathTest(nlTestSuite * apSuite, void * apContext)

ParseAttributePath(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void AttributePathListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -719,7 +719,7 @@ void AttributePathListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseAttributePathList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void EventPathTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -746,7 +746,7 @@ void EventPathTest(nlTestSuite * apSuite, void * apContext)
eventPathParser.Init(reader);
ParseEventPath(apSuite, eventPathParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void EventPathListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -768,7 +768,7 @@ void EventPathListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseEventPathList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void CommandPathTest(nlTestSuite * apSuite, void * apContext)
Expand Down Expand Up @@ -796,7 +796,7 @@ void CommandPathTest(nlTestSuite * apSuite, void * apContext)

ParseCommandPath(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void EventDataElementTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -823,7 +823,7 @@ void EventDataElementTest(nlTestSuite * apSuite, void * apContext)
eventDataElementParser.Init(reader);
ParseEventDataElement(apSuite, eventDataElementParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void EventListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -847,7 +847,7 @@ void EventListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseEventList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void StatusElementTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -874,7 +874,7 @@ void StatusElementTest(nlTestSuite * apSuite, void * apContext)
statusElementParser.Init(reader);
ParseStatusElement(apSuite, statusElementParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void AttributeStatusElementTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -901,7 +901,7 @@ void AttributeStatusElementTest(nlTestSuite * apSuite, void * apContext)
attributeStatusElementParser.Init(reader);
ParseAttributeStatusElement(apSuite, attributeStatusElementParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void AttributeStatusListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -926,7 +926,7 @@ void AttributeStatusListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseAttributeStatusList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void AttributeDataElementTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -953,7 +953,7 @@ void AttributeDataElementTest(nlTestSuite * apSuite, void * apContext)
attributeDataElementParser.Init(reader);
ParseAttributeDataElement(apSuite, attributeDataElementParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void AttributeDataListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -977,7 +977,7 @@ void AttributeDataListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseAttributeDataList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void CommandDataElementTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -1004,7 +1004,7 @@ void CommandDataElementTest(nlTestSuite * apSuite, void * apContext)
commandDataElementParser.Init(reader);
ParseCommandDataElement(apSuite, commandDataElementParser);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void CommandListTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -1028,7 +1028,7 @@ void CommandListTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseCommandList(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void ReportDataTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -1050,7 +1050,7 @@ void ReportDataTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseReportData(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

void InvokeCommandTest(nlTestSuite * apSuite, void * apContext)
Expand All @@ -1072,7 +1072,7 @@ void InvokeCommandTest(nlTestSuite * apSuite, void * apContext)
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
ParseInvokeCommand(apSuite, reader);

bufHandle.Adopt(nullptr);
bufHandle = nullptr;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/ble/BtpEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ void BtpEngine::LogState() const

ChipLogError(Ble, "mRxFragmentSize: %d", mRxFragmentSize);
ChipLogError(Ble, "mRxState: %d", mRxState);
ChipLogError(Ble, "mRxBuf: %p", mRxBuf.Get_ForNow());
ChipLogError(Ble, "mRxBuf: %d", !mRxBuf.IsNull());
ChipLogError(Ble, "mRxNextSeqNum: %d", mRxNextSeqNum);
ChipLogError(Ble, "mRxNewestUnackedSeqNum: %d", mRxNewestUnackedSeqNum);
ChipLogError(Ble, "mRxOldestUnackedSeqNum: %d", mRxOldestUnackedSeqNum);
Expand All @@ -594,7 +594,7 @@ void BtpEngine::LogState() const

ChipLogError(Ble, "mTxFragmentSize: %d", mTxFragmentSize);
ChipLogError(Ble, "mTxState: %d", mTxState);
ChipLogError(Ble, "mTxBuf: %p", mTxBuf.Get_ForNow());
ChipLogError(Ble, "mTxBuf: %d", !mTxBuf.IsNull());
ChipLogError(Ble, "mTxNextSeqNum: %d", mTxNextSeqNum);
ChipLogError(Ble, "mTxNewestUnackedSeqNum: %d", mTxNewestUnackedSeqNum);
ChipLogError(Ble, "mTxOldestUnackedSeqNum: %d", mTxOldestUnackedSeqNum);
Expand Down
2 changes: 1 addition & 1 deletion src/inet/IPEndPointBasis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ System::Error IPEndPointBasis::PostPacketBufferEvent(chip::System::Layer & aLaye
if (error != INET_NO_ERROR)
{
// If PostEvent() failed, it has not taken ownership of the buffer, so we need to retake it so that it will be freed.
aBuffer.Adopt(buf);
(void) System::PacketBufferHandle::Adopt(buf);
}
return error;
}
Expand Down
6 changes: 3 additions & 3 deletions src/inet/InetLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1068,7 +1068,7 @@ chip::System::Error InetLayer::HandleInetLayerEvent(chip::System::Object & aTarg

case kInetEvent_TCPDataReceived:
static_cast<TCPEndPoint &>(aTarget).HandleDataReceived(
System::PacketBufferHandle::Create(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
System::PacketBufferHandle::Adopt(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
break;

case kInetEvent_TCPDataSent:
Expand All @@ -1083,14 +1083,14 @@ chip::System::Error InetLayer::HandleInetLayerEvent(chip::System::Object & aTarg
#if INET_CONFIG_ENABLE_RAW_ENDPOINT
case kInetEvent_RawDataReceived:
static_cast<RawEndPoint &>(aTarget).HandleDataReceived(
System::PacketBufferHandle::Create(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
System::PacketBufferHandle::Adopt(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
break;
#endif // INET_CONFIG_ENABLE_RAW_ENDPOINT

#if INET_CONFIG_ENABLE_UDP_ENDPOINT
case kInetEvent_UDPDataReceived:
static_cast<UDPEndPoint &>(aTarget).HandleDataReceived(
System::PacketBufferHandle::Create(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
System::PacketBufferHandle::Adopt(reinterpret_cast<chip::System::PacketBuffer *>(aArgument)));
break;
#endif // INET_CONFIG_ENABLE_UDP_ENDPOINT

Expand Down
4 changes: 1 addition & 3 deletions src/inet/RawEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -957,9 +957,7 @@ u8_t RawEndPoint::LwIPReceiveRawMessage(void * arg, struct raw_pcb * pcb, struct
chip::System::Layer & lSystemLayer = ep->SystemLayer();
IPPacketInfo * pktInfo = NULL;
uint8_t enqueue = 1;
System::PacketBufferHandle buf;

buf.Adopt(reinterpret_cast<PacketBuffer *>(static_cast<void *>(p)));
System::PacketBufferHandle buf = System::PacketBufferHandle::Adopt(p);

// Filtering based on the saved ICMP6 types (the only protocol currently supported.)
if ((ep->IPVer == kIPVersion_6) && (ep->IPProto == kIPProtocol_ICMPv6))
Expand Down
4 changes: 1 addition & 3 deletions src/inet/UDPEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -869,9 +869,7 @@ void UDPEndPoint::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb, struct
UDPEndPoint * ep = static_cast<UDPEndPoint *>(arg);
chip::System::Layer & lSystemLayer = ep->SystemLayer();
IPPacketInfo * pktInfo = NULL;
System::PacketBufferHandle buf;

buf.Adopt(reinterpret_cast<PacketBuffer *>(static_cast<void *>(p)));
System::PacketBufferHandle buf = System::PacketBufferHandle::Adopt(p);

pktInfo = GetPacketInfo(buf);
if (pktInfo != NULL)
Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class DLL_EXPORT ExchangeContext : public ReferenceCounted<ExchangeContext, Exch
*
* @param[in] msgType The message type of the corresponding protocol.
*
* @param[in] msgPayload A pointer to the PacketBuffer object holding the CHIP message.
* @param[in] msgPayload A handle to the PacketBuffer object holding the CHIP message.
*
* @param[in] sendFlags Flags set by the application for the CHIP message being sent.
*
Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ExchangeDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class DLL_EXPORT ExchangeDelegate
* @param[in] packetHeader A reference to the PacketHeader object.
* @param[in] protocolId The protocol identifier of the received message.
* @param[in] msgType The message type of the corresponding protocol.
* @param[in] payload A pointer to the PacketBuffer object holding the message payload.
* @param[in] payload A handle to the PacketBuffer object holding the message payload.
*/
virtual void OnMessageReceived(ExchangeContext * ec, const PacketHeader & packetHeader, uint32_t protocolId, uint8_t msgType,
System::PacketBufferHandle payload) = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/platform/EFR32/BLEManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ void BLEManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)
case DeviceEventType::kCHIPoBLEWriteReceived: {
ChipLogProgress(DeviceLayer, "_OnPlatformEvent kCHIPoBLEWriteReceived");
HandleWriteReceived(event->CHIPoBLEWriteReceived.ConId, &CHIP_BLE_SVC_ID, &ChipUUID_CHIPoBLEChar_RX,
PacketBufferHandle::Create(event->CHIPoBLEWriteReceived.Data));
PacketBufferHandle::Adopt(event->CHIPoBLEWriteReceived.Data));
}
break;

Expand Down
2 changes: 1 addition & 1 deletion src/platform/ESP32/bluedroid/BLEManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ void BLEManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)

case DeviceEventType::kCHIPoBLEWriteReceived:
HandleWriteReceived(event->CHIPoBLEWriteReceived.ConId, &CHIP_BLE_SVC_ID, &ChipUUID_CHIPoBLEChar_RX,
PacketBufferHandle::Create(event->CHIPoBLEWriteReceived.Data));
PacketBufferHandle::Adopt(event->CHIPoBLEWriteReceived.Data));
break;

case DeviceEventType::kCHIPoBLEIndicateConfirm:
Expand Down
2 changes: 1 addition & 1 deletion src/platform/ESP32/nimble/BLEManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ void BLEManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)

case DeviceEventType::kCHIPoBLEWriteReceived:
HandleWriteReceived(event->CHIPoBLEWriteReceived.ConId, &CHIP_BLE_SVC_ID, &chipUUID_CHIPoBLEChar_RX,
PacketBufferHandle::Create(event->CHIPoBLEWriteReceived.Data));
PacketBufferHandle::Adopt(event->CHIPoBLEWriteReceived.Data));
break;

case DeviceEventType::kCHIPoBLEIndicateConfirm:
Expand Down
2 changes: 1 addition & 1 deletion src/platform/Linux/BLEManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ void BLEManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event)

case DeviceEventType::kCHIPoBLEWriteReceived:
HandleWriteReceived(event->CHIPoBLEWriteReceived.ConId, &CHIP_BLE_SVC_ID, &ChipUUID_CHIPoBLEChar_RX,
PacketBufferHandle::Create(event->CHIPoBLEWriteReceived.Data));
PacketBufferHandle::Adopt(event->CHIPoBLEWriteReceived.Data));
break;

case DeviceEventType::kCHIPoBLEIndicateConfirm:
Expand Down
2 changes: 1 addition & 1 deletion src/platform/Zephyr/GenericBLEManagerImpl_Zephyr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ CHIP_ERROR GenericBLEManagerImpl_Zephyr<ImplClass>::HandleRXCharWrite(const Chip
bt_conn_index(c1WriteEvent->BtConn));

HandleWriteReceived(c1WriteEvent->BtConn, &CHIP_BLE_SVC_ID, &chipUUID_CHIPoBLEChar_RX,
PacketBufferHandle::Create(c1WriteEvent->Data));
PacketBufferHandle::Adopt(c1WriteEvent->Data));
bt_conn_unref(c1WriteEvent->BtConn);

return CHIP_NO_ERROR;
Expand Down
Loading

0 comments on commit cbd36d6

Please sign in to comment.