Skip to content

[nat64] use OwningList to manage active mappings - #11878

Merged
jwhui merged 1 commit into
openthread:mainfrom
abtink:nat64/mapping-init-free
Aug 30, 2025
Merged

[nat64] use OwningList to manage active mappings#11878
jwhui merged 1 commit into
openthread:mainfrom
abtink:nat64/mapping-init-free

Conversation

@abtink

@abtink abtink commented Aug 28, 2025

Copy link
Copy Markdown
Member

This change updates the management of active NAT64 mappings to use an OwningList.

To support this, the Mapping struct now inherits from InstanceLocatorInit and includes a new Free() method. This method encapsulates the logic for releasing the associated IPv4 address and the Mapping entry back to their corresponding pools.

This new design simplifies the Translator class by removing the redundant ReleaseMapping(), ReleaseMappings(), and ReleaseExpiredMappings() methods. All mapping cleanup operations are now handled by the OwningList class (which invokes Mapping::Free() on entries as they are removed).

@github-actions

github-actions Bot commented Aug 28, 2025

Copy link
Copy Markdown

Merging #11878 into main

name branch text data bss total
ot-cli-ftd 853bbd1 479608 860 66548 547016
5f95614 479608 860 66548 547016
+/- +0 +0 +0 +0
ot-ncp-ftd 853bbd1 445324 764 61792 507880
5f95614 445324 764 61792 507880
+/- +0 +0 +0 +0
ot-cli-mtd 853bbd1 372352 764 51020 424136
5f95614 372352 764 51020 424136
+/- +0 +0 +0 +0
ot-ncp-mtd 853bbd1 353452 764 46288 400504
5f95614 353452 764 46288 400504
+/- +0 +0 +0 +0
ot-cli-ftd-br 853bbd1 581376 868 135332 717576
5f95614 581312 868 135332 717512
+/- -64 +0 +0 -64
ot-rcp 853bbd1 63136 568 20804 84508
5f95614 63136 568 20804 84508
+/- +0 +0 +0 +0
Library files
name branch text data bss total
libopenthread-ftd.a 853bbd1 246826 95 40326 287247
5f95614 246826 95 40326 287247
+/- +0 +0 +0 +0
libopenthread-cli-ftd.a 853bbd1 60843 0 8083 68926
5f95614 60843 0 8083 68926
+/- +0 +0 +0 +0
libopenthread-ncp-ftd.a 853bbd1 33257 0 5948 39205
5f95614 33257 0 5948 39205
+/- +0 +0 +0 +0
libopenthread-mtd.a 853bbd1 165183 0 24822 190005
5f95614 165183 0 24822 190005
+/- +0 +0 +0 +0
libopenthread-cli-mtd.a 853bbd1 41134 0 8059 49193
5f95614 41134 0 8059 49193
+/- +0 +0 +0 +0
libopenthread-ncp-mtd.a 853bbd1 25785 0 5948 31733
5f95614 25785 0 5948 31733
+/- +0 +0 +0 +0
libopenthread-ftd-br.a 853bbd1 350343 100 109078 459521
5f95614 350272 100 109078 459450
+/- -71 +0 +0 -71
libopenthread-cli-ftd-br.a 853bbd1 78409 0 8115 86524
5f95614 78409 0 8115 86524
+/- +0 +0 +0 +0
libopenthread-rcp.a 853bbd1 9932 0 5060 14992
5f95614 9932 0 5060 14992
+/- +0 +0 +0 +0
libopenthread-radio.a 853bbd1 19423 0 238 19661
5f95614 19423 0 238 19661
+/- +0 +0 +0 +0

@abtink

abtink commented Aug 28, 2025

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the management of active NAT64 mappings to use an OwningList, which simplifies the code by encapsulating mapping cleanup logic within the Mapping::Free method. This is a great improvement that removes several manual mapping release methods from the Translator class. The changes are well-implemented and align with the goal of simplification. I have one minor suggestion to improve code readability by refactoring a conditional statement to avoid an empty if block.

@codecov

codecov Bot commented Aug 28, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 38.46154% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.44%. Comparing base (853bbd1) to head (b7bcde6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/core/net/nat64_translator.cpp 27.27% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11878      +/-   ##
==========================================
- Coverage   75.25%   69.44%   -5.81%     
==========================================
  Files         644      644              
  Lines       98754   102434    +3680     
==========================================
- Hits        74313    71134    -3179     
- Misses      24441    31300    +6859     
Files with missing lines Coverage Δ
src/core/net/nat64_translator.hpp 36.36% <100.00%> (-51.14%) ⬇️
src/core/net/nat64_translator.cpp 23.20% <27.27%> (-51.06%) ⬇️

... and 190 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This change refactors the NAT64 translator to use OwningList for managing active mappings, replacing manual memory management with automatic cleanup. The Mapping struct is enhanced to handle its own resource deallocation through a new Free() method, eliminating the need for multiple release methods in the Translator class.

  • Replaces LinkedList<Mapping> with OwningList<Mapping> for automatic memory management
  • Adds Free() method to Mapping struct to encapsulate cleanup logic
  • Removes redundant release methods (ReleaseMapping, ReleaseMappings, ReleaseExpiredMappings)

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/core/net/nat64_translator.hpp Updates class structure to use OwningList, adds Free() method to Mapping, removes manual release methods
src/core/net/nat64_translator.cpp Implements Mapping::Free() method, updates mapping pool initialization, replaces manual cleanup calls with OwningList methods

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/core/net/nat64_translator.hpp
Comment thread src/core/net/nat64_translator.cpp
Comment thread src/core/net/nat64_translator.cpp
Comment thread src/core/net/nat64_translator.cpp
This change updates the management of active NAT64 mappings to use an
`OwningList`.

To support this, the `Mapping` struct now inherits from
`InstanceLocatorInit` and includes a new `Free()` method. This method
encapsulates the logic for releasing the associated IPv4 address and
the `Mapping` entry back to their corresponding pools.

This new design simplifies the `Translator` class by removing the
redundant `ReleaseMapping()`, `ReleaseMappings()`, and
`ReleaseExpiredMappings()` methods. All mapping cleanup operations
are now handled by the `OwningList` class (which invokes
`Mapping::Free()` on entries as they are removed).
@abtink
abtink force-pushed the nat64/mapping-init-free branch from cec9823 to b7bcde6 Compare August 29, 2025 20:19
@abtink
abtink marked this pull request as ready for review August 29, 2025 20:19
@jwhui
jwhui merged commit 83272e7 into openthread:main Aug 30, 2025
137 of 139 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants