ospfd: Implement rfc4222 Recommendation 1 and 2

[dfedyk]

Implement RFC 4222 Recommendation 1 and Recommendation 2 includes documentation and topotests for each. See documentation for more details.

[greptile-apps]

Greptile Summary

This PR implements RFC 4222 Recommendation 1 (per-packet-type DSCP marking via ip ospf dscp all|low-control) and Recommendation 2 (dead-timer reset on any control packet via ip ospf dead-timer-reset any-control), including documentation, VTY commands, operational display, and topotests for both features.

• P1 – flaky test: test_ospf_dead_timer queries R2's reset counter immediately after flooding 49 loopbacks on R1, with no sleep in between; the LSUs have not been processed yet and resets will be 0, causing a spurious failure.
• P2 – default branch inconsistency: in ospf_tos_for_type, the default case reads dscp_low_control directly without the configured-check / dscp_ospf_all fallback that the DB-Desc/LS-Req/LS-Upd case applies.

Confidence Score: 4/5

Safe to merge after fixing the missing sleep in test_ospf_dead_timer; core C implementation is correct.

One P1 finding remains: the inactive-timer topotest will be flaky in CI because it checks the reset counter without sleeping after flooding routes. The C implementation itself (DSCP marking, inactivity timer restart, byte-order handling) is correct. The P2 default-branch inconsistency in ospf_tos_for_type is a minor architectural nit that will not affect currently deployed packet types.

tests/topotests/ospf_rfc4222_inactive_tmr/test_ospf_rfc4222_inactive_tmr.py — missing sleep before counter assertion

Important Files Changed

Filename	Overview
ospfd/ospf_packet.c	Adds ospf_maybe_restart_inactivity() (RFC4222 Rec2) and ospf_tos_for_type() (RFC4222 Rec1); default branch in ospf_tos_for_type skips the low-control configured-check fallback
ospfd/ospf_vty.c	Adds DEFPY commands for DSCP and dead-timer-reset; config-write guards are now independent (idempotency fix applied); DSCP operational display and neighbor reset counter added
ospfd/ospf_interface.c	Initialises new dead_timer_any, dscp_ospf_all, dscp_low_control params (UNSET + default value) and adds them to ospf_free_if_params guard
ospfd/ospf_nsm.c	Adds ospf_nsm_restart_inactivity_timer() helper that cancels and re-arms the inactivity timer; straightforward
ospfd/ospf_neighbor.h	Adds dead_timer_resets uint64_t counter to ospf_neighbor struct
tests/topotests/ospf_rfc4222_inactive_tmr/test_ospf_rfc4222_inactive_tmr.py	Tests dead-timer reset counter; missing sleep between LSU flood and counter check makes test_ospf_dead_timer flaky
tests/topotests/ospf_rfc4222_dscp/test_ospf_rfc4222_dscp.py	Tests on-wire DSCP markings and operational display; ack_ok/hello_ok/low_ctrl_ok initialised correctly; duplicate pytest import is harmless

Comments Outside Diff (1)

1. tests/topotests/ospf_rfc4222_inactive_tmr/test_ospf_rfc4222_inactive_tmr.py, line 1354-1367 (link)

   No sleep before querying the reset counter — test will be flaky

   Routes are added to R1 and then R2's neighbor-detail output is captured immediately. OSPF must still detect the new prefixes, generate LSAs, transmit LSUs over the link, and have R2 process them before dead_timer_resets > 0 can be true. Without any sleep the assertion will race and the test will sporadically fail in CI.

   Add a sleep after the loopback additions, similar to how the DSCP test uses topotest.sleep(3, "Gathering Packets") before reading results:

   topotest.sleep(3, "Wait for LSU flood")
   out = r2.vtysh_cmd("show ip ospf neighbor detail")

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: tests/topotests/ospf_rfc4222_inactive_tmr/test_ospf_rfc4222_inactive_tmr.py
   Line: 1354-1367
   
   Comment:
   **No sleep before querying the reset counter — test will be flaky**
   
   Routes are added to R1 and then R2's neighbor-detail output is captured immediately. OSPF must still detect the new prefixes, generate LSAs, transmit LSUs over the link, and have R2 process them before `dead_timer_resets > 0` can be true. Without any sleep the assertion will race and the test will sporadically fail in CI.
   
   Add a sleep after the loopback additions, similar to how the DSCP test uses `topotest.sleep(3, "Gathering Packets")` before reading results:
   
   ```python
   topotest.sleep(3, "Wait for LSU flood")
   out = r2.vtysh_cmd("show ip ospf neighbor detail")
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: tests/topotests/ospf_rfc4222_inactive_tmr/test_ospf_rfc4222_inactive_tmr.py
Line: 1354-1367

Comment:
**No sleep before querying the reset counter — test will be flaky**

Routes are added to R1 and then R2's neighbor-detail output is captured immediately. OSPF must still detect the new prefixes, generate LSAs, transmit LSUs over the link, and have R2 process them before `dead_timer_resets > 0` can be true. Without any sleep the assertion will race and the test will sporadically fail in CI.

Add a sleep after the loopback additions, similar to how the DSCP test uses `topotest.sleep(3, "Gathering Packets")` before reading results:

```python
topotest.sleep(3, "Wait for LSU flood")
out = r2.vtysh_cmd("show ip ospf neighbor detail")
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: ospfd/ospf_packet.c
Line: 441-443

Comment:
**`default` branch skips the configured-check fallback**

The `OSPF_MSG_DB_DESC`/`OSPF_MSG_LS_REQ`/`OSPF_MSG_LS_UPD` case correctly falls back to `dscp_ospf_all` when `dscp_low_control` is not explicitly configured. The `default` branch reads `dscp_low_control` unconditionally, so any unknown future packet type gets the raw (possibly un-configured) low-control value instead of inheriting `all`. The same fallback guard used above should be applied here for consistency.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (5): Last reviewed commit: "doc: add OSPF DSCP control packet markin..." | Re-trigger Greptile}

[greptile-apps]

_{20 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[riw777]

looks good ... it doesn't look like the lint errors are in the changed code (?)

/* 
<stdin>:359: trailing whitespace.
/* 
<stdin>:371: trailing whitespace.
/* 
<stdin>:383: trailing whitespace.
/* 
warning: 4 lines add whitespace errors.
Report for ospf_packet.c | 10 issues
===============================================
< WARNING: Block comments use * on subsequent lines
< #422: FILE: /tmp/f1-1891220/ospf_packet.c:422:
< ERROR: trailing whitespace
< #1301: FILE: /tmp/f1-1891220/ospf_packet.c:1301:
< ERROR: trailing whitespace
< #1552: FILE: /tmp/f1-1891220/ospf_packet.c:1552:
< ERROR: trailing whitespace
< #1800: FILE: /tmp/f1-1891220/ospf_packet.c:1800:
< ERROR: trailing whitespace
< #2194: FILE: /tmp/f1-1891220/ospf_packet.c:2194:\nDetails at [https://ci1.netdef.org/browse/FRR-PULLREQ3-CHECKOUT-14881](https://ci1.netdef.org/browse/FRR-PULLREQ3-CHECKOUT-14881)```

[riw777]

@dfedyk can you fix the lint stuff here?

[dfedyk]

Sorry for delay I had some vacation and STDs meetings back now.

[riw777]

FYI, the linter looks wrong on the one remaining suggested change ...

[Jafaral]

@greptile review

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[Jafaral]

@greptile review

[riw777]

looks good, waiting on @Jafaral 's review

[Jafaral]

@greptile review

[greptile-apps]

dscp_ospf_all never applied to bulk traffic despite documentation claiming it does

The command help text for ip ospf dscp all (line 3399 in ospf_vty.c) explicitly states: "DSCP used for all OSPF control packets (Hellos, Acks, and others when low-control not set)". However, ospf_tos_for_type hardwires DB-Desc, LS-Req, and LS-Upd to always read dscp_low_control, regardless of whether dscp_low_control was explicitly configured.

With the defaults both at IPTOS_PREC_INTERNETCONTROL >> 2 (CS6 = 48), an operator who runs only ip ospf dscp all 46 will get Hello/Ack at DSCP 46 while bulk traffic continues to use DSCP 48 — a higher precedence class, which inverts the QoS priority the RFC is designed to achieve. The bulk-traffic path should fall back to dscp_ospf_all when dscp_low_control is not explicitly configured:

case OSPF_MSG_DB_DESC:
case OSPF_MSG_LS_REQ:
case OSPF_MSG_LS_UPD:
default:
    /* Fall back to dscp_ospf_all when low-control is not explicitly set */
    if (OSPF_IF_PARAM_CONFIGURED(oi->params, dscp_low_control) ||
        OSPF_IF_PARAM_CONFIGURED(IF_DEF_PARAMS(oi->ifp), dscp_low_control))
        dscp = OSPF_IF_PARAM(oi, dscp_low_control);
    else
        dscp = OSPF_IF_PARAM(oi, dscp_ospf_all);
    break;

The existing test_ospf_dscp_all_and_low_control test always configures both parameters together (all=46 and low-control=40), so it does not exercise or catch this gap.

Prompt To Fix With AI

This is a comment left during a code review.
Path: ospfd/ospf_packet.c
Line: 419-444

Comment:
**`dscp_ospf_all` never applied to bulk traffic despite documentation claiming it does**

The command help text for `ip ospf dscp all` (line 3399 in `ospf_vty.c`) explicitly states: *"DSCP used for all OSPF control packets (Hellos, Acks, **and others when low-control not set**)"*. However, `ospf_tos_for_type` hardwires DB-Desc, LS-Req, and LS-Upd to always read `dscp_low_control`, regardless of whether `dscp_low_control` was explicitly configured.

With the defaults both at `IPTOS_PREC_INTERNETCONTROL >> 2` (CS6 = 48), an operator who runs only `ip ospf dscp all 46` will get Hello/Ack at DSCP 46 while bulk traffic continues to use DSCP 48 — a **higher** precedence class, which inverts the QoS priority the RFC is designed to achieve. The bulk-traffic path should fall back to `dscp_ospf_all` when `dscp_low_control` is not explicitly configured:

```c
case OSPF_MSG_DB_DESC:
case OSPF_MSG_LS_REQ:
case OSPF_MSG_LS_UPD:
default:
    /* Fall back to dscp_ospf_all when low-control is not explicitly set */
    if (OSPF_IF_PARAM_CONFIGURED(oi->params, dscp_low_control) ||
        OSPF_IF_PARAM_CONFIGURED(IF_DEF_PARAMS(oi->ifp), dscp_low_control))
        dscp = OSPF_IF_PARAM(oi, dscp_low_control);
    else
        dscp = OSPF_IF_PARAM(oi, dscp_ospf_all);
    break;
```

The existing `test_ospf_dscp_all_and_low_control` test always configures **both** parameters together (all=46 *and* low-control=40), so it does not exercise or catch this gap.

How can I resolve this? If you propose a fix, please make it concise.

[dfedyk]

This is currently design is intent- do not couple value to precedence meaning since a higher DSCP value does not always mean a higher precedence. This is basically a user beware configuration and you should know what you are doing. If we think the documentation needs to be more explicit then I would rather update the documentation than make a change where the system forces a value which would not allow flexibility through all values. If we implement this it would prevent users setting to some DSCP values which could have local meaning. There are DSCP values are available for local/experimental use for example (e.g., 1–7, 9, 11, 13, 15, 17, .).

[Jafaral]

@greptile, look at the reply above

[greptile-apps]

Fair point — you're right that coupling the fallback logic to a precedence assumption would be wrong. DSCP is just a 6-bit marking; its meaning is locally defined, and forcing dscp_ospf_all as a fallback would break operators using local/experimental codepoints for bulk traffic.

The documentation wording "and others when low-control not set" is the real issue here. If the help text were updated to say something like "DSCP used for Hello and LS-Ack packets" (without implying a fallback), the behavior and docs would be consistent and no code change is needed.

Tip: You can give me more context with [.greptile/rules](https://www.greptile.com/docs/code-review/custom-standards) so I get this right next time.

[dfedyk]

I will review and update the documentation.

[dfedyk]

I have found a side effect and I am updating the operational display to clear up the confusion.

[Jafaral]

@greptile review