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

Jump to bottom

Final PacketBufferHandle cleanup. #4364

Merged
merged 6 commits into from
Jan 26, 2021
Merged

Final PacketBufferHandle cleanup. #4364

merged 6 commits into from
Jan 26, 2021

Conversation

kpschoedel
Copy link
Contributor

@kpschoedel kpschoedel commented Jan 14, 2021 •
edited
Loading

Problem

Code should use PacketBufferHandle rather than PacketBuffer *.
A few references and transitional interfaces remain.

Summary of Changes

  • Promote Release_ForNow() to UnsafeRelease(). It's impractical
    to remove entirely since it's used conjunction with platform event
    queues.
  • Replace GetLwIPpbuf() with LwIPPacketBufferView::UnsafeGetLwIPpbuf(),
    restricted to specific friend classes.
  • Privatize AddRef() and raw SetDataLength().
  • Remove AddToEnd_ForNow().
  • Remove various using PacketBuffer declarations.
  • Remove obsolete comments about PacketBuffer * ownership, and
    rephrase comments mentioning the PacketBuffer class by name.

Fixes #2707 - Figure out a way to express PacketBuffer ownership in the type system

@kpschoedel
Final PacketBufferHandle cleanup.
5342400 
#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
A few references and transitional interfaces remain.

#### Summary of Changes

- Promote `Release_ForNow()` to `UnsafeRelease()`. It's impractical
  to remove entirely since it's used conjunction with platform event
  queues.
- Rename `GetLwIPpbuf()` to `UnsafeGetLwIPpbuf()` for consistency.
- Privatize `AddRef()` and raw `SetDataLength()`.
- Remove `AddToEnd_ForNow()`.
- Remove various `using PacketBuffer` declarations.
- Remove obsolete comments about `PacketBuffer *` ownership, and
  rephrase comments mentioning the `PacketBuffer` class by name.

Fixes project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system
kpschoedel
kpschoedel commented Jan 14, 2021
View reviewed changes
src/system/SystemPacketBuffer.h
* The ref-qualifier `&&` requires the caller to use `std::move` to emphasize that ownership is
* moved out of this handle.
*/
CHECK_RETURN_VALUE PacketBuffer * UnsafeRelease() &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is currently only used to reference a PacketBuffer in events. In the case of CHIPoBLEWriteReceived it's as a PacketBuffer*, but in ChipSystemLayerEvent it's as a uintptr_t. An option would be to only Release as an opaque uintptr_t to discourage any use of packet buffers through a PacketBuffer*; it might then also be possible to make PacketBuffer itself a private type inside PacketBufferHandle.

rwalker-apple
rwalker-apple reviewed Jan 14, 2021
View reviewed changes
src/system/SystemPacketBuffer.h Outdated
@@ -592,9 +575,25 @@ class DLL_EXPORT PacketBufferHandle
*
* @note This should be used ONLY by low-level code interfacing with LwIP.
*/
struct pbuf * GetLwIPpbuf() { return static_cast<struct pbuf *>(mBuffer); }
struct pbuf * UnsafeGetLwIPpbuf() { return static_cast<struct pbuf *>(mBuffer); }
Copy link
Contributor

Choose a reason for hiding this comment

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

can we protect this method and make a list of friend classes allowed to call it? is that worth the work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the only uses are in Inet:…EndPoint, so that shouldn't be difficult. (The same isn't true of UnsafeRelease(), unfortunately, since it's mostly used under src/platform/, so hiding it would be a problem for out-of-tree platforms.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with a task-specific subclass to set a pattern for disjoint exposures. No increase in code size.

rwalker-apple
rwalker-apple approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@rwalker-apple rwalker-apple left a comment

Choose a reason for hiding this comment

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

nice work

bzbarsky-apple
bzbarsky-apple approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

Nicely done!

src/inet/RawEndPoint.cpp Outdated
#else // LWIP_VERSION_MAJOR <= 1 && LWIP_VERSION_MINOR < 5
if (PCB_ISIPV6(mRaw))
{
ip6_addr_t ipv6Addr = addr.ToIPv6();

lwipErr = raw_sendto_ip6(mRaw, msg.GetLwIPpbuf(), &ipv6Addr);
lwipErr = raw_sendto_ip6(mRaw, msg.UnsafeGetLwIPpbuf(), &ipv6Addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

src/inet/RawEndPoint.cpp Outdated
}
#if INET_CONFIG_ENABLE_IPV4
else
{
ip4_addr_t ipv4Addr = addr.ToIPv4();

lwipErr = raw_sendto(mRaw, msg.GetLwIPpbuf(), &ipv4Addr);
lwipErr = raw_sendto(mRaw, msg.UnsafeGetLwIPpbuf(), &ipv4Addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

src/inet/UDPEndPoint.cpp Outdated
@@ -600,9 +598,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
}

if (intfId != INET_NULL_INTERFACEID)
lwipErr = udp_sendto_if_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
lwipErr = udp_sendto_if_ip6(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

src/inet/UDPEndPoint.cpp Outdated
else
lwipErr = udp_sendto_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
lwipErr = udp_sendto_ip6(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

src/inet/UDPEndPoint.cpp Outdated
@@ -619,9 +617,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
}

if (intfId != INET_NULL_INTERFACEID)
lwipErr = udp_sendto_if(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
lwipErr = udp_sendto_if(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

src/inet/UDPEndPoint.cpp Outdated
else
lwipErr = udp_sendto(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
lwipErr = udp_sendto(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and thanks for catching that. Looks like I rushed that change and stopped when the active builds worked.

@kpschoedel
Copy link
Contributor Author

@andy31415 @saurabhst @BroderickCarlin @hawk248 @jelderton ? Need a couple more partner reviews.

@github-actions
Copy link

Size increase report for "esp32-example-build" from fad6d05

File Section File VM
chip-all-clusters-app.elf .flash.text -8 -8
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_abbrev,0,2074
.debug_str,0,115
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,1
.debug_loc,0,-3
.flash.text,-8,-8
.debug_ranges,0,-72
.debug_info,0,-143
.debug_line,0,-1064

@bzbarsky-apple bzbarsky-apple merged commit bd11f98 into project-chip:master Jan 26, 2021
@kpschoedel kpschoedel deleted the x2707-19-cleanup branch January 27, 2021 14:21
austinh0 pushed a commit to austinh0/connectedhomeip that referenced this pull request Jan 28, 2021
@kpschoedel @austinh0
Final PacketBufferHandle cleanup. (project-chip#4364)
6e106fb 
* Final PacketBufferHandle cleanup.

#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
A few references and transitional interfaces remain.

#### Summary of Changes

- Promote `Release_ForNow()` to `UnsafeRelease()`. It's impractical
  to remove entirely since it's used conjunction with platform event
  queues.
- Rename `GetLwIPpbuf()` to `UnsafeGetLwIPpbuf()` for consistency.
- Privatize `AddRef()` and raw `SetDataLength()`.
- Remove `AddToEnd_ForNow()`.
- Remove various `using PacketBuffer` declarations.
- Remove obsolete comments about `PacketBuffer *` ownership, and
  rephrase comments mentioning the `PacketBuffer` class by name.

Fixes project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system
austinh0 pushed a commit to austinh0/connectedhomeip that referenced this pull request Jan 28, 2021
@kpschoedel @austinh0
Final PacketBufferHandle cleanup. (project-chip#4364)
2fce597 
* Final PacketBufferHandle cleanup.

#### Problem

Code should use `PacketBufferHandle` rather than `PacketBuffer *`.
A few references and transitional interfaces remain.

#### Summary of Changes

- Promote `Release_ForNow()` to `UnsafeRelease()`. It's impractical
  to remove entirely since it's used conjunction with platform event
  queues.
- Rename `GetLwIPpbuf()` to `UnsafeGetLwIPpbuf()` for consistency.
- Privatize `AddRef()` and raw `SetDataLength()`.
- Remove `AddToEnd_ForNow()`.
- Remove various `using PacketBuffer` declarations.
- Remove obsolete comments about `PacketBuffer *` ownership, and
  rephrase comments mentioning the `PacketBuffer` class by name.

Fixes project-chip#2707 - Figure out a way to express PacketBuffer ownership in the type system
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@jelderton jelderton jelderton approved these changes

@rwalker-apple rwalker-apple rwalker-apple approved these changes

@mspang mspang mspang approved these changes

@andy31415 andy31415 Awaiting requested review from andy31415

@BroderickCarlin BroderickCarlin Awaiting requested review from BroderickCarlin

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@saurabhst saurabhst Awaiting requested review from saurabhst

@shana-apple shana-apple Awaiting requested review from shana-apple

Assignees
No one assigned
Labels
app ble controller darwin efr32 esp32 examples inet linux platform zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Figure out a way to express PacketBuffer ownership in the type system
5 participants
@kpschoedel @mspang @rwalker-apple @jelderton @bzbarsky-apple