Re: [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 20 Feb 2026 19:54:00 +0100
"Matthieu Baerts (NGI0)" <matttbe@kernel.org> wrote:

> With this series, the command 'ip [-t[s]] mptcp monitor' produces an
> output with the JSON format.
> 
> - Patch 1: a small refactoring to uniform the code.
> 
> - Patch 2: add JSON support in print_timestamp() helper.
> 
> - Patch 3: add JSON support to 'ip mptcp monitor' as requested by
>   Stephen.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (3):
>       mptcp: uniform stream closure
>       utils: timestamp: add JSON support
>       mptcp: monitor: add JSON support
> 
>  ip/ipmptcp.c | 96 +++++++++++++++++++++++++++++++++++++++++-------------------
>  lib/utils.c  | 19 ++++++++----
>  2 files changed, 80 insertions(+), 35 deletions(-)
> ---
> base-commit: f120cdb35da76ca99932d0428a0917bd01a843da
> change-id: 20260220-ipr-monitor-json-1573371524c1
> 
> Best regards,

I used recent review-prompt to do a more deep dive AI analysis.
And it found:

Here's the deep dive review. The series is solid work from Matthieu —
clean, well-structured, and follows iproute2 conventions. The main
items worth raising:

token printed via print_uint() with %08x format — text will show hex
but JSON will emit decimal. Probably fine (JSON consumers prefer raw
numbers), but worth a conscious decision.

Signal handler calls delete_json_obj() and rtnl_close() — these aren't
async-signal-safe. Pragmatically OK for a Ctrl-C'd monitor tool, but
rtnl_close() could be dropped since exit() cleans up the socket anyway.

Missing delete_json_obj() on the normal/error exit path after
rtnl_listen() returns — if it fails rather than being signaled, the
JSON ] never gets written.

Patch 1 quietly fixes a real bug (fflush before close_json_object) —
the commit message could call that out more explicitly.

--------
# Review: [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support

**Author**: Matthieu Baerts (NGI0) \<matttbe@kernel.org\>
**Date**: Fri, 20 Feb 2026
**Series**: 3 patches

## Overview

This series adds JSON output support to `ip mptcp monitor`, converting raw `printf()` calls to the standard `print_XXX()` helpers. It's well-structured as three incremental patches: a cleanup, a utility enhancement, and the main feature. Overall this is solid work — the conversion is thorough and follows iproute2 conventions well. I have a few comments below, mostly minor.

---

## Patch 1/3: mptcp: uniform stream closure

**Verdict: Looks good.**

This is a clean refactoring. Two changes:

1. Replaces `print_string(PRINT_FP, NULL, "\n", NULL)` and `print_string(PRINT_FP, NULL, "%s", "\n")` with `print_nl()` — correct and cleaner.

2. Reorders `close_json_object()` to come *after* `print_nl()` but *before* `fflush()`. This is the right ordering: newline belongs inside the object's text representation, and flushing should happen after the JSON object is fully closed so the complete JSON is written out.

In `print_mptcp_limit()`, the original code called `fflush(stdout)` before `close_json_object()` — that's a bug in the existing code (would flush incomplete JSON). This patch fixes it. Worth noting in the commit message that this is a bugfix, not just cosmetic.

No issues.

---

## Patch 2/3: utils: timestamp: add JSON support

**Verdict: Acceptable, with comments.**

Adds JSON-aware output to `print_timestamp()` by using `print_string()` when `fp == stdout`, falling back to `fprintf()` otherwise.

### Comments

**1. The `fp == stdout` conditional pattern is unusual but justified here.**

The commit message explains this: the `print_XXX()` helpers only write to stdout, so when `fp` is something else (e.g. stderr), it must fall back to `fprintf()`. This is a reasonable pragmatic choice given the existing API. A brief inline comment in the code would help future readers understand *why* the check exists:

```c
/* print_string() only outputs to stdout; fall back to fprintf() for other streams */
if (fp == stdout)
```

**2. JSON key naming: `timestamp_short` vs `timestamp`.**

The short timestamp gets key `"timestamp_short"` while the long one gets `"timestamp"`. This means JSON consumers need to know which format was requested to know which key to look for. Consider using a single key name (e.g. `"timestamp"`) for both, since only one can ever be present at a time and consumers shouldn't need to care about the format variant. Up to the maintainer though — there may be value in distinguishing them.

**3. The `snprintf` composition is clean.**

Building the timestamp string first with `strftime` + `snprintf` for the microseconds, then passing the complete string to `print_string()`, is the right approach. It avoids needing two separate JSON fields for the time and microsecond components.

**4. Minor: `strftime()` return value.**

`strftime()` returns 0 on failure (buffer too small). The code uses the return value in `ts + len` without checking for 0. The buffer is 40 bytes and the format `"%Y-%m-%dT%H:%M:%S"` is ~19 chars, so this is safe in practice, but a pedantic reviewer might want a check. Not a blocker.

---

## Patch 3/3: mptcp: monitor: add JSON support

**Verdict: Good overall, a few points to discuss.**

This is the main patch. It converts all `printf()` / `color_fprintf()` calls in `mptcp_monitor_msg()` to `print_XXX()` helpers and adds JSON object framing and signal handling.

### What's done well

- Consistent use of `print_uint(PRINT_ANY, ...)`, `print_string(PRINT_ANY, ...)`, `print_0xhex(PRINT_ANY, ...)` throughout — no fprintf-to-stdout for display output.
- Error output isn't affected (no stdout corruption risk).
- `open_json_object(NULL)` / `close_json_object()` properly paired in `mptcp_monitor_msg()`.
- The boolean flags (`server_side`, `deny_join_id0`) correctly use separate `print_string(PRINT_FP, ...)` / `print_bool(PRINT_JSON, ...)` paths — this is one of the legitimate cases for separate JSON/text output, since the text format is a bare keyword while JSON needs a proper boolean value.
- Merging the two identical `UNKNOWN` cases into one `||` condition is a nice cleanup.

### Comments

**1. `token` format: `%08x` in `print_uint()` — JSON will emit decimal, not hex.**

```c
print_uint(PRINT_ANY, "token"," token=%08x",
           rta_getattr_u32(tb[MPTCP_ATTR_TOKEN]));
```

The text format string `%08x` will show hex in text mode, but `print_uint()` emits the raw numeric value in JSON mode — which will be decimal. This changes the JSON representation from what a user might expect for a token (usually seen as hex). Two options:

- Use `print_0xhex()` if you want hex in both modes.
- Accept decimal in JSON (it's a valid uint32) and document it. JSON consumers typically prefer raw numeric values anyway.

Either way, this deserves a conscious decision. The same applies to `reset_flags` but that one already uses `print_0xhex()` which is correct.

**2. Missing space before `"token"` format string.**

```c
print_uint(PRINT_ANY, "token"," token=%08x",
```

There's a missing space after `"token",` — purely cosmetic, but the rest of the file has consistent spacing after commas. Very minor.

**3. Signal handler safety: `sig_exit_json()` calls non-async-signal-safe functions.**

```c
static void sig_exit_json(int signo)
{
    delete_json_obj();
    rtnl_close(&genl_rth);
    exit(0);
}
```

`delete_json_obj()` ultimately calls `fwrite()`/`fflush()` to emit the closing `]`, and `rtnl_close()` calls `close()`. Strictly speaking, `fwrite`/`fflush` are not async-signal-safe (POSIX). If the signal arrives while inside `printf()` or another stdio call in `mptcp_monitor_msg()`, this could deadlock or corrupt the stream.

In practice, for a simple monitoring tool that's being Ctrl-C'd by a user, this is unlikely to cause real problems. But the technically correct approach would be to set a volatile flag in the signal handler and check it in the main loop, or use `sigaction` with `SA_RESTART` and handle cleanup after `rtnl_listen()` returns.

This pattern does exist in other iproute2 monitor commands, so it may be acceptable as-is. Worth noting for awareness.

**4. `rtnl_close(&genl_rth)` in signal handler — is this necessary?**

Other monitor commands (e.g. `ip monitor`) don't close the rtnl handle in signal handlers. The process is about to `exit(0)` anyway, so the kernel will clean up the socket. Removing it would reduce the signal-safety concern.

**5. `new_json_obj(json)` placement.**

```c
new_json_obj(json);

if (is_json_context()) {
    struct sigaction sa = { .sa_handler = sig_exit_json };

    sigaction(SIGINT, &sa, NULL);
    sigaction(SIGTERM, &sa, NULL);
}
```

This looks correct — `new_json_obj()` initializes the JSON context, and then the signal handlers are only installed when actually in JSON mode. Good.

However, there's no corresponding `delete_json_obj()` on the normal exit path (after `rtnl_listen()` returns). If `rtnl_listen()` returns due to an error rather than a signal, the JSON output won't be properly closed. Consider adding cleanup after the `rtnl_listen()` call:

```c
if (rtnl_listen(&genl_rth, mptcp_monitor_msg, stdout) < 0) {
    delete_json_obj();
    return 2;
}

delete_json_obj();
return 0;
```

**6. `sigaction` struct initialization.**

```c
struct sigaction sa = { .sa_handler = sig_exit_json };
```

This zero-initializes the rest of the struct via designated initializer, so `sa_mask` is empty and `sa_flags` is 0. That's fine. The `SA_RESTART` flag is intentionally not set, which means `rtnl_listen()` will be interrupted by the signal and return `EINTR` — but the signal handler calls `exit(0)` so it never actually returns. Acceptable.

---

## Commit Message Quality

All three patches have proper `Signed-off-by`, correct `[PATCH iproute2-next]` prefix, and reasonable descriptions. Patch 2's message is the most detailed, which is appropriate since it has the most subtle design decision (the `fp == stdout` check).

One suggestion: Patch 1's message could mention that the reordering of `close_json_object()` vs `fflush()` in `print_mptcp_limit()` fixes incorrect flush ordering, not just "uniform" the code.

---

## Summary

| Check | Status |
|---|---|
| `strcmp()` not `matches()` | N/A (no arg parsing changes) |
| `print_XXX()` helpers used | ✅ All printf→print_XXX conversions correct |
| Errors to stderr | ✅ No error output changes |
| JSON open/close paired | ✅ Balanced in mptcp_monitor_msg |
| `PRINT_ANY` preferred | ✅ Used throughout; separate paths only where justified |
| Signed-off-by present | ✅ All three patches |
| Subject format | ✅ `[PATCH iproute2-next N/3]` |
| No string breaking | ✅ |
| Coding style | ✅ Tabs, braces, naming all correct |

**Key items for author attention:**

1. **`token` hex format** — confirm whether decimal in JSON is intentional (probably fine, but worth a note)
2. **Signal handler safety** — pragmatically OK, but consider removing `rtnl_close()` from the handler
3. **Missing `delete_json_obj()`** on `rtnl_listen()` error path
4. Consider single `"timestamp"` key name in patch 2