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

[PATCH iproute2 0/4] 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.
Patch 4 fixes ill setting of auto_recovery, prior to this patch setting
  auto_recovery zeroed grace_period.

Series generated against master commit:
84b9168328bf ip nexthop: Allow flush|list operations to specify a specific protocol

Thanks,
Tariq

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

 devlink/devlink.c | 66 +++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[PATCH iproute2 1/4] 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 2f084c020765..f1b9b2da39d7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -355,6 +355,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,
@@ -1799,14 +1805,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)
@@ -1819,29 +1822,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)
@@ -5835,14 +5832,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 ? "[" : "");
 	}
 }
 
-- 
1.8.3.1

[PATCH iproute2 2/4] 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 f1b9b2da39d7..1bfc3283a832 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1843,26 +1843,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)
@@ -1889,18 +1892,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)
-- 
1.8.3.1

[PATCH iproute2 3/4] 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

Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Reported-by: Jiri Pirko <jiri@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 1bfc3283a832..722b6a101673 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6377,7 +6377,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();
-- 
1.8.3.1

[PATCH iproute2 4/4] devlink: Fix devlink health set command

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

Prior to this patch both the reporter's name and the grace period
attributes shared the same bit. This caused zeroing grace period when
setting auto recovery. Let each parameter has its own bit.

Fixes: b18d89195b16 ("devlink: Add devlink health set 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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 722b6a101673..306abfb5222b 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -231,11 +231,11 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_FLASH_FILE_NAME	BIT(25)
 #define DL_OPT_FLASH_COMPONENT	BIT(26)
 #define DL_OPT_HEALTH_REPORTER_NAME	BIT(27)
-#define DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD	BIT(27)
-#define DL_OPT_HEALTH_REPORTER_AUTO_RECOVER	BIT(28)
+#define DL_OPT_HEALTH_REPORTER_GRACEFUL_PERIOD	BIT(28)
 #define DL_OPT_TRAP_NAME		BIT(29)
 #define DL_OPT_TRAP_ACTION		BIT(30)
 #define DL_OPT_TRAP_GROUP_NAME		BIT(31)
+#define DL_OPT_HEALTH_REPORTER_AUTO_RECOVER	BIT(32)
 
 struct dl_opts {
 	uint64_t present; /* flags of present items */
-- 
1.8.3.1

Re: [PATCH iproute2 4/4] devlink: Fix devlink health set command

[Stephen Hemminger]

On Thu,  5 Sep 2019 15:43:07 +0300
Tariq Toukan <tariqt@mellanox.com> wrote:

> From: Aya Levin <ayal@mellanox.com>
> 
> Prior to this patch both the reporter's name and the grace period
> attributes shared the same bit. This caused zeroing grace period when
> setting auto recovery. Let each parameter has its own bit.
> 
> Fixes: b18d89195b16 ("devlink: Add devlink health set command")
> Signed-off-by: Aya Levin <ayal@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>

Does not apply to current iproute2.

Re: [PATCH iproute2 4/4] devlink: Fix devlink health set command

[Aya Levin]

On 9/17/2019 6:23 PM, Stephen Hemminger wrote:
> On Thu,  5 Sep 2019 15:43:07 +0300
> Tariq Toukan <tariqt@mellanox.com> wrote:
> 
>> From: Aya Levin <ayal@mellanox.com>
>>
>> Prior to this patch both the reporter's name and the grace period
>> attributes shared the same bit. This caused zeroing grace period when
>> setting auto recovery. Let each parameter has its own bit.
>>
>> Fixes: b18d89195b16 ("devlink: Add devlink health set command")
>> Signed-off-by: Aya Levin <ayal@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> 
> Does not apply to current iproute2.
This patch is redundant due to patch from Andrea Claudi 
4fb98f08956ff4810354d75afa9b04cb6f8011bc
Please reject it.
Other patches that were submitted with, are valid.
>