[PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[Jakub Kicinski]

I got some feedback from users trying to integrate the SFP JSON
output to Meta's monitoring systems. The loss / fault signals
are currently a bit awkward to parse. This patch set changes
the format, is it still okay to merge it (as a fix?)
I think it's a worthwhile improvement, not sure how many people
depend on the current JSON format after 1 release..

Jakub Kicinski (2):
  module_common: always print per-lane status in JSON
  module_common: print loss / fault signals as bool

 module-common.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH ethtool 1/2] module_common: always print per-lane status in JSON

[Jakub Kicinski]

The JSON output type changes when loss of signal / fault is
detected. When there is no problem we print single bool, eg:

  "rx_loss_of_signal": false,

but when there's a problem we print an array:

  "rx_loss_of_signal": ["No", "Yes", "No", "No"],

This appears to be a mirror of the human-readable output,
but it's a pain to parse / unmarshall for user space.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 module-common.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/module-common.c b/module-common.c
index 9c21c2a55a18..ae500c62af89 100644
--- a/module-common.c
+++ b/module-common.c
@@ -308,11 +308,8 @@ void module_show_lane_status(const char *name, unsigned int lane_cnt,
 
 	convert_json_field_name(name, json_fn);
 
-	if (!value) {
-		if (is_json_context())
-			print_bool(PRINT_JSON, json_fn, NULL, false);
-		else
-			printf("\t%-41s : None\n", name);
+	if (!value && !is_json_context()) {
+		printf("\t%-41s : None\n", name);
 		return;
 	}
 
-- 
2.49.0

[PATCH ethtool 2/2] module_common: print loss / fault signals as bool

[Jakub Kicinski]

JSON output is supposed to be easy to parse. We currently
output "Yes" / "No" for the per-lane signal loss / fault.
This forces user space to do string matching. Print bool.

Before:
  "rx_loss_of_signal": ["No", "No", "No", "No"],

After:
  "rx_loss_of_signal": [false, false, false, false],

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 module-common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/module-common.c b/module-common.c
index ae500c62af89..11b71bdb6281 100644
--- a/module-common.c
+++ b/module-common.c
@@ -317,8 +317,7 @@ void module_show_lane_status(const char *name, unsigned int lane_cnt,
 		open_json_array(json_fn, "");
 
 		while (lane_cnt--) {
-			print_string(PRINT_JSON, NULL, "%s",
-				     value & 1 ? yes : no);
+			print_bool(PRINT_JSON, NULL, NULL, value & 1);
 			value >>= 1;
 		}
 		close_json_array("");
-- 
2.49.0

Re: [PATCH ethtool 1/2] module_common: always print per-lane status in JSON

[Ido Schimmel]

On Thu, May 29, 2025 at 07:20:32AM -0700, Jakub Kicinski wrote:
> The JSON output type changes when loss of signal / fault is
> detected. When there is no problem we print single bool, eg:
> 
>   "rx_loss_of_signal": false,
> 
> but when there's a problem we print an array:
> 
>   "rx_loss_of_signal": ["No", "Yes", "No", "No"],
> 
> This appears to be a mirror of the human-readable output,
> but it's a pain to parse / unmarshall for user space.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH ethtool 2/2] module_common: print loss / fault signals as bool

[Ido Schimmel]

On Thu, May 29, 2025 at 07:20:33AM -0700, Jakub Kicinski wrote:
> JSON output is supposed to be easy to parse. We currently
> output "Yes" / "No" for the per-lane signal loss / fault.
> This forces user space to do string matching. Print bool.
> 
> Before:
>   "rx_loss_of_signal": ["No", "No", "No", "No"],
> 
> After:
>   "rx_loss_of_signal": [false, false, false, false],
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[Joe Damato]

On Thu, May 29, 2025 at 07:20:31AM -0700, Jakub Kicinski wrote:
> I got some feedback from users trying to integrate the SFP JSON
> output to Meta's monitoring systems. The loss / fault signals
> are currently a bit awkward to parse. This patch set changes
> the format, is it still okay to merge it (as a fix?)
> I think it's a worthwhile improvement, not sure how many people
> depend on the current JSON format after 1 release..
> 
> Jakub Kicinski (2):
>   module_common: always print per-lane status in JSON
>   module_common: print loss / fault signals as bool
> 
>  module-common.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Code seems reasonable to me; no idea on whether its still okay to
merge as a fix, but I agree it seems like a worthwhile improvement.

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Thu, May 29, 2025 at 07:20:31AM GMT, Jakub Kicinski wrote:
> I got some feedback from users trying to integrate the SFP JSON
> output to Meta's monitoring systems. The loss / fault signals
> are currently a bit awkward to parse. This patch set changes
> the format, is it still okay to merge it (as a fix?)
> I think it's a worthwhile improvement, not sure how many people
> depend on the current JSON format after 1 release..

It's unfortunate that the format already got into 6.14 but thankfully
it's been only about six weeks since so hopefully there won't be many
(or perhaps none if we are lucky).

I wonder if it would make sense to also release 6.14.1 with the format
change to make it more apparent for those using 6.14 that the change
should be backported. SLE16 (and Leap 16.0) is going to be one of the
distributions with ethtool 6.14 but there I can add the patch myself.

Michal

> 
> Jakub Kicinski (2):
>   module_common: always print per-lane status in JSON
>   module_common: print loss / fault signals as bool
> 
>  module-common.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> -- 
> 2.49.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[Jakub Kicinski]

On Fri, 30 May 2025 13:35:25 +0200 Michal Kubecek wrote:
> On Thu, May 29, 2025 at 07:20:31AM GMT, Jakub Kicinski wrote:
> > I got some feedback from users trying to integrate the SFP JSON
> > output to Meta's monitoring systems. The loss / fault signals
> > are currently a bit awkward to parse. This patch set changes
> > the format, is it still okay to merge it (as a fix?)
> > I think it's a worthwhile improvement, not sure how many people
> > depend on the current JSON format after 1 release..  
> 
> It's unfortunate that the format already got into 6.14 but thankfully
> it's been only about six weeks since so hopefully there won't be many
> (or perhaps none if we are lucky).
> 
> I wonder if it would make sense to also release 6.14.1 with the format
> change to make it more apparent for those using 6.14 that the change
> should be backported. SLE16 (and Leap 16.0) is going to be one of the
> distributions with ethtool 6.14 but there I can add the patch myself.

FWIW I'll try to get it backported to Fedora / CentOS too. 
So cutting 6.14.1 may be preferable.

Re: [PATCH ethtool 0/2] module_common: adjust the JSON output for per-lane signals

[patchwork-bot+netdevbpf]

Hello:

This series was applied to ethtool/ethtool.git (master)
by Michal Kubecek <mkubecek@suse.cz>:

On Thu, 29 May 2025 07:20:31 -0700 you wrote:
> I got some feedback from users trying to integrate the SFP JSON
> output to Meta's monitoring systems. The loss / fault signals
> are currently a bit awkward to parse. This patch set changes
> the format, is it still okay to merge it (as a fix?)
> I think it's a worthwhile improvement, not sure how many people
> depend on the current JSON format after 1 release..
> 
> [...]

Here is the summary with links:
  - [ethtool,1/2] module_common: always print per-lane status in JSON
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=7ca78b77af74
  - [ethtool,2/2] module_common: print loss / fault signals as bool
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=35eb71b00968

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html