public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support
@ 2026-02-20 18:54 Matthieu Baerts (NGI0)
  2026-02-20 18:54 ` [PATCH iproute2-next 1/3] mptcp: uniform stream closure Matthieu Baerts (NGI0)
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Matthieu Baerts (NGI0) @ 2026-02-20 18:54 UTC (permalink / raw)
  To: Stephen Hemminger, David Ahern; +Cc: netdev, mptcp, Matthieu Baerts (NGI0)

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,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH iproute2-next 1/3] mptcp: uniform stream closure
  2026-02-20 18:54 [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Matthieu Baerts (NGI0)
@ 2026-02-20 18:54 ` Matthieu Baerts (NGI0)
  2026-02-20 18:54 ` [PATCH iproute2-next 2/3] utils: timestamp: add JSON support Matthieu Baerts (NGI0)
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Matthieu Baerts (NGI0) @ 2026-02-20 18:54 UTC (permalink / raw)
  To: Stephen Hemminger, David Ahern; +Cc: netdev, mptcp, Matthieu Baerts (NGI0)

Use print_nl() instead of print_string(), close the json object, then flush
the stream to flush the end of the JSON object ASAP.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 ip/ipmptcp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index ec9ef62a..18ec05a8 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -273,8 +273,8 @@ static int print_mptcp_addrinfo(struct rtattr *addrinfo)
 		}
 	}
 
+	print_nl();
 	close_json_object();
-	print_string(PRINT_FP, NULL, "\n", NULL);
 	fflush(stdout);
 
 	return 0;
@@ -426,9 +426,11 @@ static int print_mptcp_limit(struct nlmsghdr *n, void *arg)
 
 		print_uint(PRINT_ANY, "subflows", "subflows %d ", val);
 	}
-	print_string(PRINT_FP, NULL, "%s", "\n");
-	fflush(stdout);
+
+	print_nl();
 	close_json_object();
+	fflush(stdout);
+
 	return 0;
 }
 

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iproute2-next 2/3] utils: timestamp: add JSON support
  2026-02-20 18:54 [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Matthieu Baerts (NGI0)
  2026-02-20 18:54 ` [PATCH iproute2-next 1/3] mptcp: uniform stream closure Matthieu Baerts (NGI0)
@ 2026-02-20 18:54 ` Matthieu Baerts (NGI0)
  2026-02-21  5:39   ` Stephen Hemminger
  2026-02-20 18:54 ` [PATCH iproute2-next 3/3] mptcp: monitor: " Matthieu Baerts (NGI0)
  2026-02-21  5:35 ` [PATCH iproute2-next 0/3] ip mptcp " Stephen Hemminger
  3 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts (NGI0) @ 2026-02-20 18:54 UTC (permalink / raw)
  To: Stephen Hemminger, David Ahern; +Cc: netdev, mptcp, Matthieu Baerts (NGI0)

Only if the output stream is 'stdout', because all JSON helpers like
print_string() only write on 'stdout'.

Supporting JSON is easy with the helpers. The biggest modification is to
extract the end value.

No behavioural changes intended for the moment, this is a preparation for a
future usage of print_timestamp() within a JSON context.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 lib/utils.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index 13e8c098..d7581709 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1277,21 +1277,30 @@ int print_timestamp(FILE *fp)
 {
 	struct timeval tv;
 	struct tm *tm;
+	char ts[40];
 
 	gettimeofday(&tv, NULL);
 	tm = localtime(&tv.tv_sec);
 
 	if (timestamp_short) {
-		char tshort[40];
+		size_t len;
 
-		strftime(tshort, sizeof(tshort), "%Y-%m-%dT%H:%M:%S", tm);
-		fprintf(fp, "[%s.%06ld] ", tshort, tv.tv_usec);
+		len = strftime(ts, sizeof(ts), "%Y-%m-%dT%H:%M:%S", tm);
+		snprintf(ts + len, sizeof(ts) - len, ".%06ld", tv.tv_usec);
+		if (fp == stdout)
+			print_string(PRINT_ANY, "timestamp_short", "[%s] ", ts);
+		else
+			fprintf(fp, "[%s] ", ts);
 	} else {
 		char *tstr = asctime(tm);
 
 		tstr[strlen(tstr)-1] = 0;
-		fprintf(fp, "Timestamp: %s %ld usec\n",
-			tstr, tv.tv_usec);
+		snprintf(ts, sizeof(ts), "%s %ld usec", tstr, tv.tv_usec);
+		if (fp == stdout)
+			print_string(PRINT_ANY, "timestamp",
+				     "Timestamp: %s\n", ts);
+		else
+			fprintf(fp, "Timestamp: %s\n", ts);
 	}
 
 	return 0;

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH iproute2-next 3/3] mptcp: monitor: add JSON support
  2026-02-20 18:54 [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Matthieu Baerts (NGI0)
  2026-02-20 18:54 ` [PATCH iproute2-next 1/3] mptcp: uniform stream closure Matthieu Baerts (NGI0)
  2026-02-20 18:54 ` [PATCH iproute2-next 2/3] utils: timestamp: add JSON support Matthieu Baerts (NGI0)
@ 2026-02-20 18:54 ` Matthieu Baerts (NGI0)
  2026-02-21  5:23   ` Stephen Hemminger
  2026-02-21  5:35 ` [PATCH iproute2-next 0/3] ip mptcp " Stephen Hemminger
  3 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts (NGI0) @ 2026-02-20 18:54 UTC (permalink / raw)
  To: Stephen Hemminger, David Ahern; +Cc: netdev, mptcp, Matthieu Baerts (NGI0)

The command 'ip [-t[s]] mptcp monitor' now produces an output with the JSON
format.

The previous text output has been preserved.

The JSON support has been added thanks to the print_*() helpers. Note that
it is also needed to add a signal handler to close the JSON and print the
final ']' when stopping 'ip mptcp monitor' which can only be done via a
SIGINT or a SIGTERM.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 ip/ipmptcp.c | 88 +++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 27 deletions(-)

diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index 18ec05a8..87557998 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -2,6 +2,7 @@
 
 #include <arpa/inet.h>
 #include <netinet/in.h>
+#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -476,8 +477,9 @@ static void print_addr(const char *key, int af, struct rtattr *value)
 	char str[INET6_ADDRSTRLEN];
 
 	if (inet_ntop(af, data, str, sizeof(str))) {
-		printf(" %s=", key);
-		color_fprintf(stdout, ifa_family_color(af), "%s", str);
+		print_string(PRINT_FP, NULL, " %s=", key);
+		print_color_string(PRINT_ANY, ifa_family_color(af), key, "%s",
+				   str);
 	}
 }
 
@@ -485,12 +487,13 @@ static void print_iface(int index)
 {
 	const char *ifname;
 
-	printf(" ifindex=%d", index);
+	print_int(PRINT_ANY, "ifindex", " ifindex=%d", index);
 
 	ifname = index ? ll_index_to_name(index) : NULL;
 	if (ifname) {
-		printf(" dev=");
-		color_fprintf(stdout, COLOR_IFNAME, "%s", ifname);
+		print_string(PRINT_FP, NULL, " dev=", NULL);
+		print_color_string(PRINT_ANY, COLOR_IFNAME, "dev", "%s",
+				   ifname);
 	}
 }
 
@@ -509,30 +512,34 @@ static int mptcp_monitor_msg(struct rtnl_ctrl_data *ctrl,
 	if (n->nlmsg_type != genl_family)
 		return 0;
 
+	open_json_object(NULL);
+
 	if (timestamp)
 		print_timestamp(stdout);
 
-	if (ghdr->cmd >= ARRAY_SIZE(event_to_str)) {
-		printf("[UNKNOWN %u]\n", ghdr->cmd);
+	if (ghdr->cmd >= ARRAY_SIZE(event_to_str) ||
+	    event_to_str[ghdr->cmd] == NULL) {
+		char event[40];
+
+		snprintf(event, sizeof(event), "UNKNOWN %u", ghdr->cmd);
+		print_string(PRINT_ANY, "event", "[%s]", event);
 		goto out;
 	}
 
-	if (event_to_str[ghdr->cmd] == NULL) {
-		printf("[UNKNOWN %u]\n", ghdr->cmd);
-		goto out;
-	}
-
-	printf("[%16s]", event_to_str[ghdr->cmd]);
+	print_string(PRINT_ANY, "event", "[%16s]", event_to_str[ghdr->cmd]);
 
 	parse_rtattr(tb, MPTCP_ATTR_MAX, (void *) ghdr + GENL_HDRLEN, len);
 
 	if (tb[MPTCP_ATTR_TOKEN])
-		printf(" token=%08x", rta_getattr_u32(tb[MPTCP_ATTR_TOKEN]));
+		print_uint(PRINT_ANY, "token"," token=%08x",
+			   rta_getattr_u32(tb[MPTCP_ATTR_TOKEN]));
 
 	if (tb[MPTCP_ATTR_REM_ID])
-		printf(" remid=%u", rta_getattr_u8(tb[MPTCP_ATTR_REM_ID]));
+		print_uint(PRINT_ANY, "remid", " remid=%u",
+			   rta_getattr_u8(tb[MPTCP_ATTR_REM_ID]));
 	if (tb[MPTCP_ATTR_LOC_ID])
-		printf(" locid=%u", rta_getattr_u8(tb[MPTCP_ATTR_LOC_ID]));
+		print_uint(PRINT_ANY, "locid", " locid=%u",
+			   rta_getattr_u8(tb[MPTCP_ATTR_LOC_ID]));
 
 	if (tb[MPTCP_ATTR_SADDR4])
 		print_addr("saddr4", AF_INET, tb[MPTCP_ATTR_SADDR4]);
@@ -543,42 +550,60 @@ static int mptcp_monitor_msg(struct rtnl_ctrl_data *ctrl,
 	if (tb[MPTCP_ATTR_DADDR6])
 		print_addr("daddr6", AF_INET6, tb[MPTCP_ATTR_DADDR6]);
 	if (tb[MPTCP_ATTR_SPORT])
-		printf(" sport=%u", rta_getattr_be16(tb[MPTCP_ATTR_SPORT]));
+		print_uint(PRINT_ANY, "sport", " sport=%u",
+			   rta_getattr_be16(tb[MPTCP_ATTR_SPORT]));
 	if (tb[MPTCP_ATTR_DPORT])
-		printf(" dport=%u", rta_getattr_be16(tb[MPTCP_ATTR_DPORT]));
+		print_uint(PRINT_ANY, "dport", " dport=%u",
+			   rta_getattr_be16(tb[MPTCP_ATTR_DPORT]));
 	if (tb[MPTCP_ATTR_BACKUP])
-		printf(" backup=%u", rta_getattr_u8(tb[MPTCP_ATTR_BACKUP]));
+		print_uint(PRINT_ANY, "backup", " backup=%u",
+			   rta_getattr_u8(tb[MPTCP_ATTR_BACKUP]));
 	if (tb[MPTCP_ATTR_ERROR])
-		printf(" error=%u", rta_getattr_u8(tb[MPTCP_ATTR_ERROR]));
+		print_uint(PRINT_ANY, "error", " error=%u",
+			   rta_getattr_u8(tb[MPTCP_ATTR_ERROR]));
 	if (tb[MPTCP_ATTR_TIMEOUT])
-		printf(" timeout=%u", rta_getattr_u32(tb[MPTCP_ATTR_TIMEOUT]));
+		print_uint(PRINT_ANY, "timeout", " timeout=%u",
+			   rta_getattr_u32(tb[MPTCP_ATTR_TIMEOUT]));
 	if (tb[MPTCP_ATTR_IF_IDX])
 		print_iface(rta_getattr_s32(tb[MPTCP_ATTR_IF_IDX]));
 	if (tb[MPTCP_ATTR_RESET_REASON])
-		printf(" reset_reason=%u", rta_getattr_u32(tb[MPTCP_ATTR_RESET_REASON]));
+		print_uint(PRINT_ANY, "reset_reason", " reset_reason=%u",
+			   rta_getattr_u32(tb[MPTCP_ATTR_RESET_REASON]));
 	if (tb[MPTCP_ATTR_RESET_FLAGS])
-		printf(" reset_flags=0x%x", rta_getattr_u32(tb[MPTCP_ATTR_RESET_FLAGS]));
+		print_0xhex(PRINT_ANY, "reset_flags", " reset_flags=%#x",
+			    rta_getattr_u32(tb[MPTCP_ATTR_RESET_FLAGS]));
 
 	if (tb[MPTCP_ATTR_FLAGS])
 		flags = rta_getattr_u16(tb[MPTCP_ATTR_FLAGS]);
 	if ((flags & MPTCP_PM_EV_FLAG_SERVER_SIDE) ||
 	    (tb[MPTCP_ATTR_SERVER_SIDE] && rta_getattr_u8(tb[MPTCP_ATTR_SERVER_SIDE]))) {
 		flags &= ~MPTCP_PM_EV_FLAG_SERVER_SIDE;
-		printf(" server_side");
+		print_string(PRINT_FP, NULL, " server_side", NULL);
+		print_bool(PRINT_JSON, "server_side", NULL, true);
 	}
 	if (flags & MPTCP_PM_EV_FLAG_DENY_JOIN_ID0) {
 		flags &= ~MPTCP_PM_EV_FLAG_DENY_JOIN_ID0;
-		printf(" deny_join_id0");
+		print_string(PRINT_FP, NULL, " deny_join_id0", NULL);
+		print_bool(PRINT_JSON, "deny_join_id0", NULL, true);
 	}
 	if (flags) /* remaining bits */
-		printf(" flags=0x%x", flags);
+		print_0xhex(PRINT_ANY, "flags", " flags=%#x", flags);
 
-	puts("");
 out:
+	print_nl();
+	close_json_object();
 	fflush(stdout);
+
 	return 0;
 }
 
+static void sig_exit_json(int signo)
+{
+	delete_json_obj();
+	rtnl_close(&genl_rth);
+	exit(0);
+}
+
 static int mptcp_monitor(void)
 {
 	if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) {
@@ -586,6 +611,15 @@ static int mptcp_monitor(void)
 		return 1;
 	}
 
+	new_json_obj(json);
+
+	if (is_json_context()) {
+		struct sigaction sa = { .sa_handler = sig_exit_json };
+
+		sigaction(SIGINT, &sa, NULL);
+		sigaction(SIGTERM, &sa, NULL);
+	}
+
 	if (rtnl_listen(&genl_rth, mptcp_monitor_msg, stdout) < 0)
 		return 2;
 

-- 
2.51.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH iproute2-next 3/3] mptcp: monitor: add JSON support
  2026-02-20 18:54 ` [PATCH iproute2-next 3/3] mptcp: monitor: " Matthieu Baerts (NGI0)
@ 2026-02-21  5:23   ` Stephen Hemminger
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2026-02-21  5:23 UTC (permalink / raw)
  To: Matthieu Baerts (NGI0); +Cc: David Ahern, netdev, mptcp

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

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

Technically, none of these functions are signal safe.

Maybe an atexit cleanup function, or on_exit, or destructor.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support
  2026-02-20 18:54 [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Matthieu Baerts (NGI0)
                   ` (2 preceding siblings ...)
  2026-02-20 18:54 ` [PATCH iproute2-next 3/3] mptcp: monitor: " Matthieu Baerts (NGI0)
@ 2026-02-21  5:35 ` Stephen Hemminger
  3 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2026-02-21  5:35 UTC (permalink / raw)
  To: Matthieu Baerts (NGI0); +Cc: David Ahern, netdev, mptcp

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iproute2-next 2/3] utils: timestamp: add JSON support
  2026-02-20 18:54 ` [PATCH iproute2-next 2/3] utils: timestamp: add JSON support Matthieu Baerts (NGI0)
@ 2026-02-21  5:39   ` Stephen Hemminger
  2026-02-23 13:20     ` Matthieu Baerts
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2026-02-21  5:39 UTC (permalink / raw)
  To: Matthieu Baerts (NGI0); +Cc: David Ahern, netdev, mptcp

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

> Only if the output stream is 'stdout', because all JSON helpers like
> print_string() only write on 'stdout'.
> 
> Supporting JSON is easy with the helpers. The biggest modification is to
> extract the end value.
> 
> No behavioural changes intended for the moment, this is a preparation for a
> future usage of print_timestamp() within a JSON context.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

I would go farther, fp is always stdout. Drop the argument to the function.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iproute2-next 2/3] utils: timestamp: add JSON support
  2026-02-21  5:39   ` Stephen Hemminger
@ 2026-02-23 13:20     ` Matthieu Baerts
  2026-02-23 15:31       ` Matthieu Baerts
  0 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts @ 2026-02-23 13:20 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Ahern, netdev, mptcp

Hi Stephen,

On 21/02/2026 06:39, Stephen Hemminger wrote:
> On Fri, 20 Feb 2026 19:54:02 +0100
> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> wrote:
> 
>> Only if the output stream is 'stdout', because all JSON helpers like
>> print_string() only write on 'stdout'.
>>
>> Supporting JSON is easy with the helpers. The biggest modification is to
>> extract the end value.
>>
>> No behavioural changes intended for the moment, this is a preparation for a
>> future usage of print_timestamp() within a JSON context.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> I would go farther, fp is always stdout. Drop the argument to the function.

Good idea!

To be coherent with the rest, I started to look at removing the same
argument from the caller functions, and similar ones. I did a very quick
draft with a few sed, etc. but I'm not sure whether I should take this
direction:

  https://github.com/matttbe/iproute2/commit/585b0109

WDYT? Or should I only drop fp from print_timestamp()? Or simply not use
it like it is done in many other helpers supporting JSON?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH iproute2-next 2/3] utils: timestamp: add JSON support
  2026-02-23 13:20     ` Matthieu Baerts
@ 2026-02-23 15:31       ` Matthieu Baerts
  0 siblings, 0 replies; 9+ messages in thread
From: Matthieu Baerts @ 2026-02-23 15:31 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David Ahern, netdev, mptcp

Hi Stephen,

On 23/02/2026 14:20, Matthieu Baerts wrote:
> Hi Stephen,
> 
> On 21/02/2026 06:39, Stephen Hemminger wrote:
>> On Fri, 20 Feb 2026 19:54:02 +0100
>> "Matthieu Baerts (NGI0)" <matttbe@kernel.org> wrote:
>>
>>> Only if the output stream is 'stdout', because all JSON helpers like
>>> print_string() only write on 'stdout'.
>>>
>>> Supporting JSON is easy with the helpers. The biggest modification is to
>>> extract the end value.
>>>
>>> No behavioural changes intended for the moment, this is a preparation for a
>>> future usage of print_timestamp() within a JSON context.
>>>
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>
>> I would go farther, fp is always stdout. Drop the argument to the function.
> 
> Good idea!
> 
> To be coherent with the rest, I started to look at removing the same
> argument from the caller functions, and similar ones. I did a very quick
> draft with a few sed, etc. but I'm not sure whether I should take this
> direction:
> 
>   https://github.com/matttbe/iproute2/commit/585b0109
> 
> WDYT? Or should I only drop fp from print_timestamp()? Or simply not use
> it like it is done in many other helpers supporting JSON?

I just sent a v2 with the minimal modifications around print_timestamp:
the argument is now simply ignored like it is the case in other helpers
supporting JSON. If there is a need to continue the cleanup, probably
better to do that in a different series I guess.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-02-23 15:31 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-20 18:54 [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Matthieu Baerts (NGI0)
2026-02-20 18:54 ` [PATCH iproute2-next 1/3] mptcp: uniform stream closure Matthieu Baerts (NGI0)
2026-02-20 18:54 ` [PATCH iproute2-next 2/3] utils: timestamp: add JSON support Matthieu Baerts (NGI0)
2026-02-21  5:39   ` Stephen Hemminger
2026-02-23 13:20     ` Matthieu Baerts
2026-02-23 15:31       ` Matthieu Baerts
2026-02-20 18:54 ` [PATCH iproute2-next 3/3] mptcp: monitor: " Matthieu Baerts (NGI0)
2026-02-21  5:23   ` Stephen Hemminger
2026-02-21  5:35 ` [PATCH iproute2-next 0/3] ip mptcp " Stephen Hemminger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox