[PATCH iproute2 0/3] Devlink health reporter's issues

[PATCH iproute2 0/3] Devlink health reporter's issues

[Tariq Toukan]

Hi,

This patchset by Aya fixes two issues: wrong time-stamp on a dump in
devlink health reporter and messy display of non JSON output in devlink
health diagnostics and dump.

1) Wrong time-stamp on a dump in devlink health reporter: 
This bug fix consist of 2 patches. First patch refactors the current
implementation of helper function which displays the reporter's dump
time-stamp and add the actual print to the function's body.
The second patch introduces a new attribute which is the time-stamp in
current time in nsec instead of jiffies. When the new attribute is
present try and translate the time-stamp according to 'current time'.

2) Messy display of non-JSON output in devlink health diagnostics and dump:
This patch mainly deals with dynamic object and array opening. The
label is stored and only when the proceeding value arrives the name is
printed with regards to the context. 

Series generated against the shared master/net-next head:
24bee3bf9752 tipc: add new commands to set TIPC AEAD key

Regards,
Tariq

Aya Levin (3):
  devlink: Print health reporter's dump time-stamp in a helper function
  devlink: Add a new time-stamp format for health reporter's dump
  devlink: Fix fmsg nesting in non JSON output

 devlink/devlink.c | 153 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 119 insertions(+), 34 deletions(-)

-- 
2.21.0

[PATCH iproute2 1/3] devlink: Print health reporter's dump time-stamp in a helper function

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

Add pr_out_dump_reporter prefix to the helper function's name and
encapsulate the print in it.

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 | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 0b8985f32636..aee6c87cbce7 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6654,8 +6654,11 @@ static const char *health_state_name(uint8_t state)
 	}
 }
 
-static void format_logtime(uint64_t time_ms, char *ts_date, char *ts_time)
+static void pr_out_dump_reporter_format_logtime(struct dl *dl, const struct nlattr *attr)
 {
+	char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	uint64_t time_ms = mnl_attr_get_u64(attr);
 	struct sysinfo s_info;
 	struct tm *info;
 	time_t now, sec;
@@ -6673,16 +6676,16 @@ static void format_logtime(uint64_t time_ms, char *ts_date, char *ts_time)
 	sec = now - s_info.uptime + time_ms / 1000;
 	info = localtime(&sec);
 out:
-	strftime(ts_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", info);
-	strftime(ts_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", info);
+	strftime(dump_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", info);
+	strftime(dump_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", info);
+	pr_out_str(dl, "last_dump_date", dump_date);
+	pr_out_str(dl, "last_dump_time", dump_time);
 }
 
 static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
 	enum devlink_health_reporter_state state;
-	const struct nlattr *attr;
-	uint64_t time_ms;
 	int err;
 
 	err = mnl_attr_parse_nested(tb_health[DEVLINK_ATTR_HEALTH_REPORTER],
@@ -6710,17 +6713,8 @@ static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT]));
 	pr_out_u64(dl, "recover",
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT]));
-	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]) {
-		char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
-		char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
-
-		attr = tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS];
-		time_ms = mnl_attr_get_u64(attr);
-		format_logtime(time_ms, dump_date, dump_time);
-
-		pr_out_str(dl, "last_dump_date", dump_date);
-		pr_out_str(dl, "last_dump_time", dump_time);
-	}
+	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
+		pr_out_dump_reporter_format_logtime(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]);
 	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
 		pr_out_u64(dl, "grace_period",
 			   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD]));
-- 
2.21.0

[PATCH iproute2 2/3] devlink: Add a new time-stamp format for health reporter's dump

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

Introduce a new attribute representing a new time-stamp format: current
time in ns (to comply with y2038) instead of jiffies. If the new
attribute was received, translate the time-stamp accordingly (ns).

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 | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index aee6c87cbce7..f0181e41faa4 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6682,6 +6682,25 @@ out:
 	pr_out_str(dl, "last_dump_time", dump_time);
 }
 
+static void pr_out_dump_report_timestamp(struct dl *dl, const struct nlattr *attr)
+{
+	char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	time_t tv_sec;
+	struct tm *tm;
+	uint64_t ts;
+
+	ts = mnl_attr_get_u64(attr);
+	tv_sec = ts / 1000000000;
+	tm = localtime(&tv_sec);
+
+	strftime(dump_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", tm);
+	strftime(dump_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", tm);
+
+	pr_out_str(dl, "last_dump_date", dump_date);
+	pr_out_str(dl, "last_dump_time", dump_time);
+}
+
 static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
@@ -6713,7 +6732,9 @@ static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT]));
 	pr_out_u64(dl, "recover",
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT]));
-	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
+	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS_NS])
+		pr_out_dump_report_timestamp(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS_NS]);
+	else if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
 		pr_out_dump_reporter_format_logtime(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]);
 	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
 		pr_out_u64(dl, "grace_period",
-- 
2.21.0

[PATCH iproute2 3/3] devlink: Fix fmsg nesting in non JSON output

[Tariq Toukan]

From: Aya Levin <ayal@mellanox.com>

When an object or an array opening follows a name (label), add a new
line and indentation before printing the label. When name (label) is
followed by a value, print both at the same line.

Prior to this patch nesting was not visible in a non JSON output:
JSON:
{
    "Common config": {
        "SQ": {
            "stride size": 64,
            "size": 1024
        },
        "CQ": {
            "stride size": 64,
            "size": 1024
        } },
    "SQs": [ {
            "channel ix": 0,
            "sqn": 10,
            "HW state": 1,
            "stopped": false,
            "cc": 0,
            "pc": 0,
            "CQ": {
                "cqn": 6,
                "HW status": 0
            }
         },{
            "channel ix": 0,
            "sqn": 14,
            "HW state": 1,
            "stopped": false,
            "cc": 0,
            "pc": 0,
            "CQ": {
                "cqn": 10,
                "HW status": 0
            }
         } ]
}

Before this 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

With this patch:
Common config:
  SQ:
    stride size: 64 size: 1024
    CQ:
      stride size: 64 size: 1024
SQs:
  channel ix: 0 sqn: 10 HW state: 1 stopped: false cc: 0 pc: 0
  CQ:
    cqn: 6 HW status: 0
  channel ix: 1 sqn: 14 HW state: 1 stopped: false cc: 0 pc: 0
  CQ:
    cqn: 10 HW status: 0

Fixes: 7b8baf834d5e ("devlink: Add devlink health diagnose 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 | 106 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 88 insertions(+), 18 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index f0181e41faa4..95f05a0b5223 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6463,12 +6463,23 @@ static int fmsg_value_show(struct dl *dl, int type, struct nlattr *nl_data)
 	return MNL_CB_OK;
 }
 
+static void pr_out_fmsg_name(struct dl *dl, char **name)
+{
+	if (!*name)
+		return;
+
+	pr_out_name(dl, *name);
+	free(*name);
+	*name = NULL;
+}
+
 struct nest_entry {
 	int attr_type;
 	struct list_head list;
 };
 
 struct fmsg_cb_data {
+	char *name;
 	struct dl *dl;
 	uint8_t value_type;
 	struct list_head entry_list;
@@ -6498,6 +6509,56 @@ static int cmd_fmsg_nest_queue(struct fmsg_cb_data *fmsg_data,
 	return MNL_CB_OK;
 }
 
+static void pr_out_fmsg_group_start(struct dl *dl, char **name)
+{
+	__pr_out_newline();
+	pr_out_fmsg_name(dl, name);
+	__pr_out_newline();
+	__pr_out_indent_inc();
+}
+
+static void pr_out_fmsg_group_end(struct dl *dl)
+{
+	__pr_out_newline();
+	__pr_out_indent_dec();
+}
+
+static void pr_out_fmsg_start_object(struct dl *dl, char **name)
+{
+	if (dl->json_output) {
+		pr_out_fmsg_name(dl, name);
+		jsonw_start_object(dl->jw);
+	} else {
+		pr_out_fmsg_group_start(dl, name);
+	}
+}
+
+static void pr_out_fmsg_end_object(struct dl *dl)
+{
+	if (dl->json_output)
+		jsonw_end_object(dl->jw);
+	else
+		pr_out_fmsg_group_end(dl);
+}
+
+static void pr_out_fmsg_start_array(struct dl *dl, char **name)
+{
+	if (dl->json_output) {
+		pr_out_fmsg_name(dl, name);
+		jsonw_start_array(dl->jw);
+	} else {
+		pr_out_fmsg_group_start(dl, name);
+	}
+}
+
+static void pr_out_fmsg_end_array(struct dl *dl)
+{
+	if (dl->json_output)
+		jsonw_end_array(dl->jw);
+	else
+		pr_out_fmsg_group_end(dl);
+}
+
 static int cmd_fmsg_nest(struct fmsg_cb_data *fmsg_data, uint8_t nest_value,
 			 bool start)
 {
@@ -6512,26 +6573,17 @@ static int cmd_fmsg_nest(struct fmsg_cb_data *fmsg_data, uint8_t nest_value,
 	switch (value) {
 	case DEVLINK_ATTR_FMSG_OBJ_NEST_START:
 		if (start)
-			pr_out_entry_start(dl);
+			pr_out_fmsg_start_object(dl, &fmsg_data->name);
 		else
-			pr_out_entry_end(dl);
+			pr_out_fmsg_end_object(dl);
 		break;
 	case DEVLINK_ATTR_FMSG_PAIR_NEST_START:
 		break;
 	case DEVLINK_ATTR_FMSG_ARR_NEST_START:
-		if (dl->json_output) {
-			if (start)
-				jsonw_start_array(dl->jw);
-			else
-				jsonw_end_array(dl->jw);
-		} else {
-			if (start) {
-				__pr_out_newline();
-				__pr_out_indent_inc();
-			} else {
-				__pr_out_indent_dec();
-			}
-		}
+		if (start)
+			pr_out_fmsg_start_array(dl, &fmsg_data->name);
+		else
+			pr_out_fmsg_end_array(dl);
 		break;
 	default:
 		return -EINVAL;
@@ -6569,12 +6621,16 @@ static int cmd_fmsg_object_cb(const struct nlmsghdr *nlh, void *data)
 				return err;
 			break;
 		case DEVLINK_ATTR_FMSG_OBJ_NAME:
-			pr_out_name(dl, mnl_attr_get_str(nla_object));
+			free(fmsg_data->name);
+			fmsg_data->name = strdup(mnl_attr_get_str(nla_object));
+			if (!fmsg_data->name)
+				return -ENOMEM;
 			break;
 		case DEVLINK_ATTR_FMSG_OBJ_VALUE_TYPE:
 			fmsg_data->value_type = mnl_attr_get_u8(nla_object);
 			break;
 		case DEVLINK_ATTR_FMSG_OBJ_VALUE_DATA:
+			pr_out_fmsg_name(dl, &fmsg_data->name);
 			err = fmsg_value_show(dl, fmsg_data->value_type,
 					      nla_object);
 			if (err != MNL_CB_OK)
@@ -6587,6 +6643,20 @@ static int cmd_fmsg_object_cb(const struct nlmsghdr *nlh, void *data)
 	return MNL_CB_OK;
 }
 
+static void cmd_fmsg_init(struct dl *dl, struct fmsg_cb_data *data)
+{
+	/* FMSG is dynamic: opening of an object or array causes a
+	 * newline. JSON starts with an { or [, but plain text should
+	 * not start with a new line. Ensure this by setting
+	 * g_new_line_count to 1: avoiding newline before the first
+	 * print.
+	 */
+	g_new_line_count = 1;
+	data->name = NULL;
+	data->dl = dl;
+	INIT_LIST_HEAD(&data->entry_list);
+}
+
 static int cmd_health_object_common(struct dl *dl, uint8_t cmd, uint16_t flags)
 {
 	struct fmsg_cb_data data;
@@ -6600,9 +6670,9 @@ static int cmd_health_object_common(struct dl *dl, uint8_t cmd, uint16_t flags)
 	if (err)
 		return err;
 
-	data.dl = dl;
-	INIT_LIST_HEAD(&data.entry_list);
+	cmd_fmsg_init(dl, &data);
 	err = _mnlg_socket_sndrcv(dl->nlg, nlh, cmd_fmsg_object_cb, &data);
+	free(data.name);
 	return err;
 }
 
-- 
2.21.0

Re: [PATCH iproute2 0/3] Devlink health reporter's issues

[Stephen Hemminger]

On Wed, 11 Dec 2019 17:45:33 +0200
Tariq Toukan <tariqt@mellanox.com> wrote:

> Hi,
> 
> This patchset by Aya fixes two issues: wrong time-stamp on a dump in
> devlink health reporter and messy display of non JSON output in devlink
> health diagnostics and dump.
> 
> 1) Wrong time-stamp on a dump in devlink health reporter: 
> This bug fix consist of 2 patches. First patch refactors the current
> implementation of helper function which displays the reporter's dump
> time-stamp and add the actual print to the function's body.
> The second patch introduces a new attribute which is the time-stamp in
> current time in nsec instead of jiffies. When the new attribute is
> present try and translate the time-stamp according to 'current time'.
> 
> 2) Messy display of non-JSON output in devlink health diagnostics and dump:
> This patch mainly deals with dynamic object and array opening. The
> label is stored and only when the proceeding value arrives the name is
> printed with regards to the context. 
> 
> Series generated against the shared master/net-next head:
> 24bee3bf9752 tipc: add new commands to set TIPC AEAD key
> 
> Regards,
> Tariq
> 
> Aya Levin (3):
>   devlink: Print health reporter's dump time-stamp in a helper function
>   devlink: Add a new time-stamp format for health reporter's dump
>   devlink: Fix fmsg nesting in non JSON output
> 
>  devlink/devlink.c | 153 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 119 insertions(+), 34 deletions(-)
> 

Applied, devlink really needs to get converted to same json
library as rest of iproute2.

Re: [PATCH iproute2 0/3] Devlink health reporter's issues

[David Ahern]

On 12/16/19 9:53 PM, Stephen Hemminger wrote:
> Applied, devlink really needs to get converted to same json
> library as rest of iproute2.

We have been saying that for too many releases. rdma has fixed its json
support; devlink needs to the same.

Maybe it is time to stop taking patches.

Re: [PATCH iproute2 0/3] Devlink health reporter's issues

[Stephen Hemminger]

On Tue, 17 Dec 2019 08:08:31 -0700
David Ahern <dsahern@gmail.com> wrote:

> On 12/16/19 9:53 PM, Stephen Hemminger wrote:
> > Applied, devlink really needs to get converted to same json
> > library as rest of iproute2.  
> 
> We have been saying that for too many releases. rdma has fixed its json
> support; devlink needs to the same.
> 
> Maybe it is time to stop taking patches.

Or add nagware patch :-)

Re: [PATCH iproute2 0/3] Devlink health reporter's issues

[Jiri Pirko]

Tue, Dec 17, 2019 at 04:08:31PM CET, dsahern@gmail.com wrote:
>On 12/16/19 9:53 PM, Stephen Hemminger wrote:
>> Applied, devlink really needs to get converted to same json
>> library as rest of iproute2.
>
>We have been saying that for too many releases. rdma has fixed its json
>support; devlink needs to the same.

It's under work now, patches are going to be sent soon, hopefully.


>
>Maybe it is time to stop taking patches.