Skip to content

sys/net/ipv4: fix internet header length - #22552

Open
basilfx wants to merge 2 commits into
RIOT-OS:masterfrom
basilfx:feature/ipv4_header_fixes
Open

sys/net/ipv4: fix internet header length#22552
basilfx wants to merge 2 commits into
RIOT-OS:masterfrom
basilfx:feature/ipv4_header_fixes

Conversation

@basilfx

@basilfx basilfx commented Aug 10, 2026

Copy link
Copy Markdown
Member

Contribution description

The internet header length field is incorrectly stored in the v_ihl field. According to the documentation, it expects bytes as input.

Also renamed v_ih to v_ihl to be consistent, and added unit tests for this header.

Testing procedure

Run the unit tests: make -C tests/unittests tests-ipv4_hdr term

Issues/PRs references

I want to give gnrc_ipv4 (#12988) another go, so I started with the obvious things.

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

  • Claude Code Sonnet 5 to find the issue, which I then fixed myself
  • Claude Code Sonnet 5 to generate unit tests.

@basilfx
basilfx requested a review from miri64 as a code owner August 10, 2026 13:05
@basilfx basilfx added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Aug 10, 2026
@basilfx
basilfx requested a review from Teufelchen1 as a code owner August 10, 2026 13:05
@basilfx basilfx added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. labels Aug 10, 2026
@github-actions github-actions Bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: sys Area: System labels Aug 10, 2026
@riot-ci

riot-ci commented Aug 10, 2026

Copy link
Copy Markdown

Murdock results

✔️ PASSED

58cdedb tests/unittests: add header tests for sys/net/ipv4

Success Failures Total Runtime
11286 0 11286 14m:06s

Artifacts

Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread tests/unittests/tests-ipv4_hdr/tests-ipv4_hdr.h Outdated
Comment thread tests/unittests/tests-ipv4_hdr/tests-ipv4_hdr.c
@basilfx
basilfx force-pushed the feature/ipv4_header_fixes branch from 888ef24 to d461720 Compare August 10, 2026 16:10
@basilfx
basilfx force-pushed the feature/ipv4_header_fixes branch from affc32e to f7f740e Compare September 5, 2026 22:28
Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread sys/include/net/ipv4/hdr.h Outdated
Comment thread sys/include/net/ipv4/hdr.h Outdated
* * ipv4_hdr_get_ihl()
*/
uint8_t v_ih;
uint8_t v_ihl;

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.

I still wonder if that counts as an API change. Although this is part of the internal struct and there are setters and getters that should be used.

@basilfx basilfx Sep 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The existing getters and setters are incorrect, and there are no uses of v_ih and/or any of the ipv4_hdr_{get/set}_* within RIOT. Furthermore, the header fields already mention the use of the getters and setters.

With that in mind, I think this change is acceptable.

@crasbe
crasbe requested a review from jia200x September 6, 2026 09:49
@crasbe

crasbe commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Ideally I'd like to get feedback from @jia200x too before merging, but if he doesn't respond (also pinged him on Matrix), this is minor enough to merge just like that I guess.

@basilfx

basilfx commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

That is fine. If I can do an intermediate squash already, that would be nice. Then I will continue with next PRs to get GNRC IPv4 ready based on this one :-)

@crasbe

crasbe commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Sure!

The internet header length occupies the four least significant bits,
of the combined version-internet header length field. Its value (
multiplied by four) indicates the length of the IPv4 header.
@basilfx
basilfx force-pushed the feature/ipv4_header_fixes branch from 67876d1 to 58cdedb Compare September 6, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Co-Authored PR/Issue relies on AI. Documentation / Code was partly generated by AI. Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants