icmp6: replace endian-dependent bitfields with flag enums

[rjarry]

The icmp6_router_advert and icmp6_neigh_advert structures used bitfields to represent flag bits. The order of bitfield members in memory is implementation-defined and requires #ifdef BYTE_ORDER guards to ensure correctness across different architectures.

Replace bitfields with uint8_t flag members and define flag constants as typed enums using the GR_BIT8() macro. This approach is portable, explicit, and does not rely on compiler-specific behavior.

Update all code accessing these flags to use bitwise operations instead of direct member access.

Summary by CodeRabbit

• Refactor
  • IPv6 router and neighbor advertisement flag handling consolidated: individual boolean flag fields replaced by a single explicit flags field for clarity and consistency across components.
  • Internal flag sourcing consolidated where diagnostics/trace output report solicited status.
  • No change to observable packet semantics; behavior preserved.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds two public enum types, icmp6_ra_flags_t and icmp6_na_flags_t, and replaces bitfield/boolean flag members in Router Advertisement and Neighbor Advertisement structures with single enum-typed flags fields. Code in trace, router_advert.c, ndp_na_input.c, and ndp_na_output.c was updated to read and set flag bits via the new flags fields (e.g., ICMP6_NA_F_SOLICITED, ICMP6_NA_F_OVERRIDE, router bit) instead of individual bitfield members. Public declarations in the ICMPv6 header were updated to expose the new enums and modified structs.

Pre-merge checks

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly and clearly describes the main change: replacing implementation-defined C bitfields with explicit flag enums in ICMP6 structures. This matches the primary objective of removing endian-dependent bitfield layouts and using the GR_BIT8() macro instead. The title is concise, specific, and accurately reflects the core refactoring work across all modified files.

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d22337 and 5eaebc8.

📒 Files selected for processing (5)

• modules/infra/datapath/gr_icmp6.h (3 hunks)
• modules/infra/datapath/trace.c (1 hunks)
• modules/ip6/control/router_advert.c (1 hunks)
• modules/ip6/datapath/ndp_na_input.c (2 hunks)
• modules/ip6/datapath/ndp_na_output.c (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• modules/infra/datapath/trace.c
• modules/infra/datapath/gr_icmp6.h
• modules/ip6/control/router_advert.c
• modules/ip6/datapath/ndp_na_input.c

🧰 Additional context used

📓 Path-based instructions (1)

**/*.{c,h}

⚙️ CodeRabbit configuration file

**/*.{c,h}: - gr_vec_*() functions cannot fail. No need to check their return value.

• gr_vec_free(x) always sets x = NULL. There is no risk of double free.
• ec_node_*() functions consume their ec_node arguments. No leaks on error.
• rte_node->ctx is an uint8_t array of size 16, not a pointer.
• Don't suggest to replace assert() with graceful error checking.
• We compile with -std=gnu2x. Unnamed parameters in function definitions are valid.

Files:

• modules/ip6/datapath/ndp_na_output.c

🧠 Learnings (1)

📓 Common learnings

Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.

🔇 Additional comments (1)

modules/ip6/datapath/ndp_na_output.c (1)

81-83: LGTM!

The flag initialization and conditional SOLICITED logic are correct. The bitwise operations properly replace the previous bitfield access.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[maxime-leroy]

Does it mean we should not allow any bit-fields in grout ?

[christophefontaine]

We're trading a known complexity in 1 header with a more complex code to write, read and debug in all other files.
I'm not sure if I'm a fan of that.
@rjarry Do we have a test that this works on big endian arch ? Otherwise, I'd refrain merging that.

[maxime-leroy]

See: https://bugs.dpdk.org/show_bug.cgi?id=1679

[rjarry]

we can have bitfields, but for stuff that goes in the network, it is "unsafe" to rely on compiler implementation details.

[rjarry]

The C standard only mentions byte ordering. Not bit ordering. Compilers can store them in any order they want.

[christophefontaine]

I prefer the approach in the BZ comment, using __attribute__((scalar_storage_order("big-endian"))).
Wouldn't it be better ? Even on the debugging side, this feels cleaner.

[maxime-leroy]

scalar_storage_order doesn't exist on clang : https://bugs.llvm.org/show_bug.cgi?id=35293

[rjarry]

As Maxime noted, this isn't supported by clang. And not standard at all.

What's wrong with using an enum with flags? Debuggers will display them without issues.

[maxime-leroy]

It's the same problem with vi. You should use emacs ;-) . Why ? Because !