From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f169.google.com (mail-qk1-f169.google.com [209.85.222.169]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1E9DC24A069 for ; Sat, 21 Feb 2026 05:35:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771652138; cv=none; b=AnWhp1NLZcvR3ooKYP4JGSEzWjQrgd6RwlcLEGpHvlbMm566xiFsktN5lg/HPBaZ7fRmdpeGzFx5aryLQB/NoXMcSBAKPgpwM1pOBeLcLKCMjI6o/xSzOffpZHk0qUc+r2MVZ9i5RuiJqVa0KPC5ofd+S35eqiatWG8ZveC8JrU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1771652138; c=relaxed/simple; bh=5qH1LrLrLDiVb2kaKGK5Vwbm30x4/G5Xg+w/R9Za6Rs=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=meUKCclBwLq4nWNWDKDJlL9vIWFxdkGT/wc0iJ4dZRejAtHf8bxhnCvhjIslqQhjFrlK4A4sPRtOuxlaLlVpMpCQ/SMgVkxF4Ao+ME8+s7AjK5nYJLgU/YRbdPdUr4wFOi2DfllXkPpLYM+oQNgaO9hMfpMpbCC2+mcGc3fxA+Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=networkplumber.org; spf=pass smtp.mailfrom=networkplumber.org; dkim=pass (2048-bit key) header.d=networkplumber-org.20230601.gappssmtp.com header.i=@networkplumber-org.20230601.gappssmtp.com header.b=2krzVSDi; arc=none smtp.client-ip=209.85.222.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=networkplumber.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=networkplumber.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=networkplumber-org.20230601.gappssmtp.com header.i=@networkplumber-org.20230601.gappssmtp.com header.b="2krzVSDi" Received: by mail-qk1-f169.google.com with SMTP id af79cd13be357-8cb7edbcde6so249537285a.3 for ; Fri, 20 Feb 2026 21:35:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1771652135; x=1772256935; darn=lists.linux.dev; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=2kf50zJ95esjyGsfgOuMwdxzW3Z/K3g4Iqb5Y6hn44Q=; b=2krzVSDi/wzH1wuRaUMot0zETppwcf7ao8j7m0Xe0TRsNxZljct0KSRNj5Aw5pXlBj HYInXb0wuu2DSFGNtotwKwhMk1u5frrk2fVaEHytEC2KsYA3YpIm0qt43cM3EpMIHsYC LXjSBZye0Scy0iT4c9Rl8D6FtmlZDlYNL6v0GfP31m5gd0bZek3HSEvPJRQ48eEAdjKi cd4wjupzkvtqedEPd9kLTt7UxNrtGEctC7LVdad2+MxHQYVSNHsg8lsedRiqMp7/sSSR MtKnnixTtyHJr8jlCxd+aq52oH5K7AlpSQCl0wOYvh+/phtjXUAuEERkTTSs2raBaMaX 6G7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1771652135; x=1772256935; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=2kf50zJ95esjyGsfgOuMwdxzW3Z/K3g4Iqb5Y6hn44Q=; b=h2t6Ss3AQW3GwwmEVKqdh6tMlCFZmi0s/KHOE/QerTTN4Jk6zRZaz65RgVBMmr/oOH sGuL7W6Er0Shy8L3IKVeKpPSKXrT8hFAidLiLrccVmLQhFxAT5GRH277W957iJL1DjZF 81uR9Wa8vaJt3wcDOPxCxg6wcDKVuD54qXRz+Oh5WQU/D/jjXGDEY/+CRHLoJFWi0hJ6 v9XxQf+MV5qNJnTICoyy9hROIrIJQBuzk9jXuSevbrXsWbcH43XICwX3elpIs9TVtXKn YLF3NCp5WVO4F/x6YYB1+UH/r5HM0owRMA6yqTCrp3l2w+SkfZ56BCFcyFoU+VQwgdie SdeQ== X-Forwarded-Encrypted: i=1; AJvYcCU0WRh8Aa1tfxwifiUWuHieZWgQabpYfTfm6Ra1qeOd71C34/wYY8/2nMWM6GJXh/j60K7cLw==@lists.linux.dev X-Gm-Message-State: AOJu0YzBwD1oh4CFKqqRxFik+kEPvVzVm5Mi2U6Kng9UaBgnVuGaNUio clM7/TKNWKB25ezI/PqzI3qE62EZehZutMox6UI9nCYEcSeQ4WVF8OjqlPOv6MUgkCE= X-Gm-Gg: AZuq6aIzh10Dh+4B8tR63LfgOSygiH8UmiU9yDmx4O2pu11oPRRNqCM4zNm8NZRD/xB pSV1dqPvFtzI4eIBaCD44KKCAE/tLwCATH9Koam7fNHRfTd/evN0VQsOg6P9+gWbA8PG8dMMuAw qdYELgr2mqjTKxi9SJqO6RiEjRLkMPDMCe+1lGQJ60y2MyG4atI763GWM6DzfoNzU3RaSR0CqFy kAVq6Y/rfOnd9AaE+sc4N/MPtAShmjFjUplyF8ldyBGYz97YRNTUAsjYU43KdK0kOB0U6Lrcn+w 4J0upzDgkDcYznYjopaUm35B44NtgJZRcJTXnulTrYgipemCoCI74XNfm+W5AX4R3KVwGFKhzsc 4yVRPR+1SX7QtbIRTBi/HYfxCTmrQ6Dv9cnoEjP5ppUdn1nFqmSoR/ml0eDsPQqofNNywZkPP9O WKqeDBGBMQCviopjjCJr4Z2kan3SxVz/Chbfen7nYAVcodhlOxdhCQJACUTTXkBHPF X-Received: by 2002:a05:620a:170f:b0:8ca:363a:cd73 with SMTP id af79cd13be357-8cb8ca834d2mr252625985a.68.1771652134876; Fri, 20 Feb 2026 21:35:34 -0800 (PST) Received: from phoenix.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-8997e6341c4sm10841026d6.35.2026.02.20.21.35.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 Feb 2026 21:35:34 -0800 (PST) Date: Fri, 20 Feb 2026 21:35:31 -0800 From: Stephen Hemminger To: "Matthieu Baerts (NGI0)" Cc: David Ahern , netdev@vger.kernel.org, mptcp@lists.linux.dev Subject: Re: [PATCH iproute2-next 0/3] ip mptcp monitor: add JSON support Message-ID: <20260220213531.2d930e5d@phoenix.local> In-Reply-To: <20260220-ipr-monitor-json-v1-0-eb4b0d5f7820@kernel.org> References: <20260220-ipr-monitor-json-v1-0-eb4b0d5f7820@kernel.org> Precedence: bulk X-Mailing-List: mptcp@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Fri, 20 Feb 2026 19:54:00 +0100 "Matthieu Baerts (NGI0)" wrote: > With this series, the command 'ip [-t[s]] mptcp monitor' produces an > output with the JSON format. >=20 > - Patch 1: a small refactoring to uniform the code. >=20 > - Patch 2: add JSON support in print_timestamp() helper. >=20 > - Patch 3: add JSON support to 'ip mptcp monitor' as requested by > Stephen. >=20 > Signed-off-by: Matthieu Baerts (NGI0) > --- > Matthieu Baerts (NGI0) (3): > mptcp: uniform stream closure > utils: timestamp: add JSON support > mptcp: monitor: add JSON support >=20 > 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 >=20 > 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 =E2=80= =94 clean, well-structured, and follows iproute2 conventions. The main items worth raising: token printed via print_uint() with %08x format =E2=80=94 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() =E2=80=94 these are= n'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 =E2=80=94 if it fails rather than being signaled, the JSON ] never gets written. Patch 1 quietly fixes a real bug (fflush before close_json_object) =E2=80=94 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) \ **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-structure= d as three incremental patches: a cleanup, a utility enhancement, and the m= ain feature. Overall this is solid work =E2=80=94 the conversion is thoroug= h and follows iproute2 conventions well. I have a few comments below, mostl= y 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(PR= INT_FP, NULL, "%s", "\n")` with `print_nl()` =E2=80=94 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()` =E2=80=94 that's a bug in the existing code (would fl= ush incomplete JSON). This patch fixes it. Worth noting in the commit messa= ge 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()` whe= n `fp =3D=3D stdout`, falling back to `fprintf()` otherwise. ### Comments **1. The `fp =3D=3D stdout` conditional pattern is unusual but justified he= re.** The commit message explains this: the `print_XXX()` helpers only write to s= tdout, 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 =3D=3D 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 reques= ted 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 co= nsumers shouldn't need to care about the format variant. Up to the maintain= er though =E2=80=94 there may be value in distinguishing them. **3. The `snprintf` composition is clean.** Building the timestamp string first with `strftime` + `snprintf` for the mi= croseconds, then passing the complete string to `print_string()`, is the ri= ght approach. It avoids needing two separate JSON fields for the time and m= icrosecond components. **4. Minor: `strftime()` return value.** `strftime()` returns 0 on failure (buffer too small). The code uses the ret= urn 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()` call= s in `mptcp_monitor_msg()` to `print_XXX()` helpers and adds JSON object fr= aming and signal handling. ### What's done well - Consistent use of `print_uint(PRINT_ANY, ...)`, `print_string(PRINT_ANY, = ...)`, `print_0xhex(PRINT_ANY, ...)` throughout =E2=80=94 no fprintf-to-std= out for display output. - Error output isn't affected (no stdout corruption risk). - `open_json_object(NULL)` / `close_json_object()` properly paired in `mptc= p_monitor_msg()`. - The boolean flags (`server_side`, `deny_join_id0`) correctly use separate= `print_string(PRINT_FP, ...)` / `print_bool(PRINT_JSON, ...)` paths =E2=80= =94 this is one of the legitimate cases for separate JSON/text output, sinc= e 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 ni= ce cleanup. ### Comments **1. `token` format: `%08x` in `print_uint()` =E2=80=94 JSON will emit deci= mal, not hex.** ```c print_uint(PRINT_ANY, "token"," token=3D%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 =E2=80=94 which will be decimal.= This changes the JSON representation from what a user might expect for a t= oken (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 consum= ers 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=3D%08x", ``` There's a missing space after `"token",` =E2=80=94 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 f= unctions.** ```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 clos= ing `]`, and `rtnl_close()` calls `close()`. Strictly speaking, `fwrite`/`f= flush` are not async-signal-safe (POSIX). If the signal arrives while insid= e `printf()` or another stdio call in `mptcp_monitor_msg()`, this could dea= dlock 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 approa= ch would be to set a volatile flag in the signal handler and check it in th= e 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 ac= ceptable as-is. Worth noting for awareness. **4. `rtnl_close(&genl_rth)` in signal handler =E2=80=94 is this necessary?= ** Other monitor commands (e.g. `ip monitor`) don't close the rtnl handle in s= ignal handlers. The process is about to `exit(0)` anyway, so the kernel wil= l 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 =3D { .sa_handler =3D sig_exit_json }; sigaction(SIGINT, &sa, NULL); sigaction(SIGTERM, &sa, NULL); } ``` This looks correct =E2=80=94 `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 pa= th (after `rtnl_listen()` returns). If `rtnl_listen()` returns due to an er= ror rather than a signal, the JSON output won't be properly closed. Conside= r 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 =3D { .sa_handler =3D 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` =E2=80=94 but the signal handler calls `ex= it(0)` so it never actually returns. Acceptable. --- ## Commit Message Quality All three patches have proper `Signed-off-by`, correct `[PATCH iproute2-nex= t]` prefix, and reasonable descriptions. Patch 2's message is the most deta= iled, which is appropriate since it has the most subtle design decision (th= e `fp =3D=3D stdout` check). One suggestion: Patch 1's message could mention that the reordering of `clo= se_json_object()` vs `fflush()` in `print_mptcp_limit()` fixes incorrect fl= ush ordering, not just "uniform" the code. --- ## Summary | Check | Status | |---|---| | `strcmp()` not `matches()` | N/A (no arg parsing changes) | | `print_XXX()` helpers used | =E2=9C=85 All printf=E2=86=92print_XXX conve= rsions correct | | Errors to stderr | =E2=9C=85 No error output changes | | JSON open/close paired | =E2=9C=85 Balanced in mptcp_monitor_msg | | `PRINT_ANY` preferred | =E2=9C=85 Used throughout; separate paths only wh= ere justified | | Signed-off-by present | =E2=9C=85 All three patches | | Subject format | =E2=9C=85 `[PATCH iproute2-next N/3]` | | No string breaking | =E2=9C=85 | | Coding style | =E2=9C=85 Tabs, braces, naming all correct | **Key items for author attention:** 1. **`token` hex format** =E2=80=94 confirm whether decimal in JSON is inte= ntional (probably fine, but worth a note) 2. **Signal handler safety** =E2=80=94 pragmatically OK, but consider remov= ing `rtnl_close()` from the handler 3. **Missing `delete_json_obj()`** on `rtnl_listen()` error path 4. Consider single `"timestamp"` key name in patch 2