Skip to content

Refactor message marshalling - parent path max length#90

Open
werwurm wants to merge 6 commits intomainfrom
werwurm/variable_pathlength2
Open

Refactor message marshalling - parent path max length#90
werwurm wants to merge 6 commits intomainfrom
werwurm/variable_pathlength2

Conversation

@werwurm
Copy link
Contributor

@werwurm werwurm commented Mar 13, 2026

The maximum path length is no longer hard coded/set at compile time. Instead, parsing the parent path is deferred, so that the number of compressed input slices need not be known and allocated in advance.

The maximum path length is no longer hard coded/set at compile time.
Instead, parsing the parent path is deferred, so that the number of
compressed input slices need not be known and allocated in advance.
@github-actions
Copy link

github-actions bot commented Mar 13, 2026

LCOV of commit 9ed4bc7 during lcov-test-coverage-report #116

Summary coverage rate:
  lines......: 95.4% (2733 of 2864 lines)
  functions..: 99.1% (209 of 211 functions)
  branches...: 86.3% (1495 of 1733 branches)

Files changed coverage rate: n/a

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors NAT20 service message marshalling for the “parent path” field to remove the compile-time maximum path length and defer decoding until iteration, reducing up-front allocation requirements.

Changes:

  • Introduces n20_parent_path_t (encoded/decoded representation) plus n20_msg_parent_path_iterate() for on-demand traversal.
  • Updates message read/write logic to preserve an encoded CBOR slice for parent paths instead of decoding into a fixed-size array.
  • Refactors/extends unit tests to cover round-trips and iterator behavior for both encoded and decoded parent-path inputs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
include/nat20/service/messages.h Adds n20_parent_path_t API + iterator and updates request payload structs to use it.
src/service/messages.c Implements deferred parent-path parsing, iterator, and updates request read/write paths accordingly.
src/service/test/messages.cpp Updates round-trip tests and adds iterator success/error-path tests for parent paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@werwurm werwurm requested a review from Copilot March 13, 2026 22:22
@werwurm werwurm marked this pull request as ready for review March 13, 2026 22:22
@werwurm werwurm requested review from mjain02 and seidelrj March 13, 2026 22:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors NAT20 service message marshalling to remove the compile-time maximum parent-path length by storing parent paths as either an encoded CBOR slice or a decoded slice array, and deferring per-element parsing until iteration.

Changes:

  • Replace fixed-size parent_path arrays + parent_path_length with n20_parent_path_t across request payloads.
  • Update message read/write logic to preserve an encoded parent-path slice and add n20_msg_parent_path_iterate() for on-demand decoding.
  • Refactor/extend tests to validate round-trips and parent-path iteration behavior (including error propagation).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

File Description
src/service/messages.c Implements encoded/decoded parent-path read/write, adds iterator API, and updates request parsing/serialization.
include/nat20/service/messages.h Introduces the n20_parent_path_t public type + iterator API and updates request payload structs to use it.
src/service/test/messages.cpp Updates round-trip tests to use n20_parent_path_t and adds iterator success/error tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


static n20_error_t n20_msg_compressed_context_array_read(n20_istream_t* istream,
n20_parent_path_t* compressed_context) {

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra newline.

Suggested change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants