[PATCH V2 iproute2 0/3] Devlink health FMSG fixes and enhancements

[PATCH V2 iproute2 0/3] Devlink health FMSG fixes and enhancements

[Tariq Toukan]

Hi,

This patchset by Aya enhances FMSG output and fixes bugs in devlink health.

Patch 1 adds a helper function wrapping repeating code which determines
  whether left-hand-side space separator in needed or not. It's not
  needed after a newline.
Patch 2 fixes left justification of FMSG output. Prior to this patch
  the FMSG output had an extra space on the left-hand-side. This patch
  avoids this by looking at a flag turned on by pr_out_new_line.
Patch 3 fixes inconsistency between input and output in devlink health
  show command.

Series generated against master commit:
8c2093e5d20c ip vrf: Add json support for show command

Thanks,
Tariq

V2:
- Dropped 4th patch, similar one is already accepted:
  4fb98f08956f devlink: fix segfault on health command

Aya Levin (3):
  devlink: Add helper for left justification print
  devlink: Left justification on FMSG output
  devlink: Fix inconsistency between command input and output

 devlink/devlink.c | 62 +++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

-- 
2.21.0

[PATCH V2 iproute2 1/3] devlink: Add helper for left justification print

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

Introduce a helper function which wraps code that adds a left hand side
space separator unless it follows a newline.

Fixes: e3d0f0c0e3d8 ("devlink: add option to generate JSON output")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 devlink/devlink.c | 45 ++++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 5e762e37540c..d654dc7bc637 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -379,6 +379,12 @@ static bool dl_no_arg(struct dl *dl)
 	return dl_argc(dl) == 0;
 }
 
+static void __pr_out_indent_newline(struct dl *dl)
+{
+	if (!g_indent_newline && !dl->json_output)
+		pr_out(" ");
+}
+
 static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_BUS_NAME] = MNL_TYPE_NUL_STRING,
 	[DEVLINK_ATTR_DEV_NAME] = MNL_TYPE_NUL_STRING,
@@ -1828,14 +1834,11 @@ static void pr_out_port_handle_end(struct dl *dl)
 
 static void pr_out_str(struct dl *dl, const char *name, const char *val)
 {
-	if (dl->json_output) {
+	__pr_out_indent_newline(dl);
+	if (dl->json_output)
 		jsonw_string_field(dl->jw, name, val);
-	} else {
-		if (g_indent_newline)
-			pr_out("%s %s", name, val);
-		else
-			pr_out(" %s %s", name, val);
-	}
+	else
+		pr_out("%s %s", name, val);
 }
 
 static void pr_out_bool(struct dl *dl, const char *name, bool val)
@@ -1848,29 +1851,23 @@ static void pr_out_bool(struct dl *dl, const char *name, bool val)
 
 static void pr_out_uint(struct dl *dl, const char *name, unsigned int val)
 {
-	if (dl->json_output) {
+	__pr_out_indent_newline(dl);
+	if (dl->json_output)
 		jsonw_uint_field(dl->jw, name, val);
-	} else {
-		if (g_indent_newline)
-			pr_out("%s %u", name, val);
-		else
-			pr_out(" %s %u", name, val);
-	}
+	else
+		pr_out("%s %u", name, val);
 }
 
 static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
 {
+	__pr_out_indent_newline(dl);
 	if (val == (uint64_t) -1)
 		return pr_out_str(dl, name, "unlimited");
 
-	if (dl->json_output) {
+	if (dl->json_output)
 		jsonw_u64_field(dl->jw, name, val);
-	} else {
-		if (g_indent_newline)
-			pr_out("%s %"PRIu64, name, val);
-		else
-			pr_out(" %s %"PRIu64, name, val);
-	}
+	else
+		pr_out("%s %"PRIu64, name, val);
 }
 
 static void pr_out_bool_value(struct dl *dl, bool value)
@@ -6118,14 +6115,12 @@ static void pr_out_region_handle_end(struct dl *dl)
 
 static void pr_out_region_snapshots_start(struct dl *dl, bool array)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output) {
 		jsonw_name(dl->jw, "snapshot");
 		jsonw_start_array(dl->jw);
 	} else {
-		if (g_indent_newline)
-			pr_out("snapshot %s", array ? "[" : "");
-		else
-			pr_out(" snapshot %s", array ? "[" : "");
+		pr_out("snapshot %s", array ? "[" : "");
 	}
 }
 
-- 
2.21.0

Re: [PATCH V2 iproute2 1/3] devlink: Add helper for left justification print

[Stephen Hemminger]

On Wed,  2 Oct 2019 17:35:14 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:

>  static void pr_out_str(struct dl *dl, const char *name, const char *val)
>  {
> -	if (dl->json_output) {
> +	__pr_out_indent_newline(dl);
> +	if (dl->json_output)
>  		jsonw_string_field(dl->jw, name, val);
> -	} else {
> -		if (g_indent_newline)
> -			pr_out("%s %s", name, val);
> -		else
> -			pr_out(" %s %s", name, val);
> -	}
> +	else
> +		pr_out("%s %s", name, val)

Overall this looks like an improvement.

Why doesn't devlink already use existing json_print infrastructure?

Re: [PATCH V2 iproute2 1/3] devlink: Add helper for left justification print

[Jiri Pirko]

Wed, Oct 02, 2019 at 04:48:04PM CEST, stephen@networkplumber.org wrote:
>On Wed,  2 Oct 2019 17:35:14 +0300
>Tariq Toukan <tariqt@mellanox.com> wrote:
>
>>  static void pr_out_str(struct dl *dl, const char *name, const char *val)
>>  {
>> -	if (dl->json_output) {
>> +	__pr_out_indent_newline(dl);
>> +	if (dl->json_output)
>>  		jsonw_string_field(dl->jw, name, val);
>> -	} else {
>> -		if (g_indent_newline)
>> -			pr_out("%s %s", name, val);
>> -		else
>> -			pr_out(" %s %s", name, val);
>> -	}
>> +	else
>> +		pr_out("%s %s", name, val)
>
>Overall this looks like an improvement.
>
>Why doesn't devlink already use existing json_print infrastructure?

It will happen soon hopefully. I have it on the todo list and hopefully
also a person to do it :)

[PATCH V2 iproute2 2/3] devlink: Left justification on FMSG output

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

FMSG output is dynamic, space separator must be on the left hand side of
the value. Otherwise output has redundant left indentation regardless
the hierarchy.

Before the patch:
 Common config: SQ: stride size: 64 size: 1024
 CQ: stride size: 64 size: 1024
 SQs:
   channel ix: 0 tc: 0 txq ix: 0 sqn: 10 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 6 HW status: 0
   channel ix: 1 tc: 0 txq ix: 1 sqn: 14 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 10 HW status: 0
   channel ix: 2 tc: 0 txq ix: 2 sqn: 18 HW state: 1 stopped: false cc: 5 pc: 5 CQ: cqn: 14 HW status: 0
   channel ix: 3 tc: 0 txq ix: 3 sqn: 22 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 18 HW status: 0

With the patch:
Common config: SQ: stride size: 64 size: 1024
CQ: stride size: 64 size: 1024
SQs:
  channel ix: 0 tc: 0 txq ix: 0 sqn: 10 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 6 HW status: 0
  channel ix: 1 tc: 0 txq ix: 1 sqn: 14 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 10 HW status: 0
  channel ix: 2 tc: 0 txq ix: 2 sqn: 18 HW state: 1 stopped: false cc: 5 pc: 5 CQ: cqn: 14 HW status: 0
  channel ix: 3 tc: 0 txq ix: 3 sqn: 22 HW state: 1 stopped: false cc: 0 pc: 0 CQ: cqn: 18 HW status: 0

Fixes: 844a61764c6f ("devlink: Add helper functions for name and value separately")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 devlink/devlink.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index d654dc7bc637..50baf82af937 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1872,26 +1872,29 @@ static void pr_out_u64(struct dl *dl, const char *name, uint64_t val)
 
 static void pr_out_bool_value(struct dl *dl, bool value)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output)
 		jsonw_bool(dl->jw, value);
 	else
-		pr_out(" %s", value ? "true" : "false");
+		pr_out("%s", value ? "true" : "false");
 }
 
 static void pr_out_uint_value(struct dl *dl, unsigned int value)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output)
 		jsonw_uint(dl->jw, value);
 	else
-		pr_out(" %u", value);
+		pr_out("%u", value);
 }
 
 static void pr_out_uint64_value(struct dl *dl, uint64_t value)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output)
 		jsonw_u64(dl->jw, value);
 	else
-		pr_out(" %"PRIu64, value);
+		pr_out("%"PRIu64, value);
 }
 
 static bool is_binary_eol(int i)
@@ -1918,18 +1921,20 @@ static void pr_out_binary_value(struct dl *dl, uint8_t *data, uint32_t len)
 
 static void pr_out_str_value(struct dl *dl, const char *value)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output)
 		jsonw_string(dl->jw, value);
 	else
-		pr_out(" %s", value);
+		pr_out("%s", value);
 }
 
 static void pr_out_name(struct dl *dl, const char *name)
 {
+	__pr_out_indent_newline(dl);
 	if (dl->json_output)
 		jsonw_name(dl->jw, name);
 	else
-		pr_out(" %s:", name);
+		pr_out("%s:", name);
 }
 
 static void pr_out_region_chunk_start(struct dl *dl, uint64_t addr)
-- 
2.21.0

[PATCH V2 iproute2 3/3] devlink: Fix inconsistency between command input and output

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

In devlink health show command the reporter's name parameter is called
reporter, but in the output the reporter's name is referred to as name

Before this patch:
$ devlink health show pci/0000:04:00.0 reporter tx
pci/0000:04:00.0:
   name tx
     state healthy error 0 recover 0 grace_period 500 auto_recover true

After this patch:
$ devlink health show pci/0000:04:00.0 reporter tx
pci/0000:04:00.0:
   reporter tx
     state healthy error 0 recover 0 grace_period 500 auto_recover true

Reported-by: Jiri Pirko <jiri@mellanox.com>
Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
 devlink/devlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 50baf82af937..5bbe0bddd910 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6660,7 +6660,7 @@ static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 
 	pr_out_handle_start_arr(dl, tb_health);
 
-	pr_out_str(dl, "name",
+	pr_out_str(dl, "reporter",
 		   mnl_attr_get_str(tb[DEVLINK_ATTR_HEALTH_REPORTER_NAME]));
 	if (!dl->json_output) {
 		__pr_out_newline();
-- 
2.21.0

Re: [PATCH V2 iproute2 0/3] Devlink health FMSG fixes and enhancements

[Stephen Hemminger]

On Wed,  2 Oct 2019 17:35:13 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:

> Hi,
> 
> This patchset by Aya enhances FMSG output and fixes bugs in devlink health.
> 
> Patch 1 adds a helper function wrapping repeating code which determines
>   whether left-hand-side space separator in needed or not. It's not
>   needed after a newline.
> Patch 2 fixes left justification of FMSG output. Prior to this patch
>   the FMSG output had an extra space on the left-hand-side. This patch
>   avoids this by looking at a flag turned on by pr_out_new_line.
> Patch 3 fixes inconsistency between input and output in devlink health
>   show command.
> 
> Series generated against master commit:
> 8c2093e5d20c ip vrf: Add json support for show command
> 
> Thanks,
> Tariq
> 
> V2:
> - Dropped 4th patch, similar one is already accepted:
>   4fb98f08956f devlink: fix segfault on health command
> 
> Aya Levin (3):
>   devlink: Add helper for left justification print
>   devlink: Left justification on FMSG output
>   devlink: Fix inconsistency between command input and output
> 
>  devlink/devlink.c | 62 +++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 

Series applied.