Re: [PATCH iproute2-next v2] devlink: Add health command support
From: David Ahern <hidden>
Date: 2019-01-23 03:38:06
On 1/20/19 2:27 AM, Aya Levin wrote:
quoted hunk ↗ jump to hunk
diff --git a/devlink/devlink.c b/devlink/devlink.c index 3651e90c1159..9fc19668ccd0 100644 --- a/devlink/devlink.c +++ b/devlink/devlink.c@@ -1,4 +1,5 @@ /* + *
extra newline
quoted hunk ↗ jump to hunk
* devlink.c Devlink tool * * This program is free software; you can redistribute it and/or@@ -22,7 +23,7 @@ #include <linux/devlink.h> #include <libmnl/libmnl.h> #include <netinet/ether.h> - +#include <sys/sysinfo.h> #include "SNAPSHOT.h" #include "list.h" #include "mnlg.h"@@ -40,6 +41,12 @@ #define PARAM_CMODE_DRIVERINIT_STR "driverinit" #define PARAM_CMODE_PERMANENT_STR "permanent" +#define HEALTH_REPORTER_STATE_HEALTHY_STR "healthy" +#define HEALTH_REPORTER_STATE_ERROR_STR "error" +#define HEALTH_REPORTER_GRACE_PERIOD_STR "grace_period" +#define HEALTH_REPORTER_AUTO_RECOVER_STR "auto_recover" +#define HEALTH_REPORTER_TS_LEN 80 + static int g_new_line_count; #define pr_err(args...) fprintf(stderr, ##args)@@ -199,6 +206,10 @@ static void ifname_map_free(struct ifname_map *ifname_map) #define DL_OPT_REGION_SNAPSHOT_ID BIT(22) #define DL_OPT_REGION_ADDRESS BIT(23) #define DL_OPT_REGION_LENGTH BIT(24) +#define DL_OPT_HANDLE_HEALTH BIT(25) +#define DL_OPT_HEALTH_REPORTER_NAME BIT(26) +#define DL_OPT_HEALTH_REPORTER_DEV BIT(27) +
extra newline
quoted hunk ↗ jump to hunk
struct dl_opts { uint32_t present; /* flags of present items */@@ -230,6 +241,10 @@ struct dl_opts { uint32_t region_snapshot_id; uint64_t region_address; uint64_t region_length; + const char *reporter_name; + const char *reporter_param_name; + const char *reporter_param_value; + }; struct dl {@@ -959,7 +974,7 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required, if (err) return err; o_found |= handle_bit; - } else if (o_required & DL_OPT_HANDLE) { + } else if (DL_OPT_HANDLE) {
typo? Seems like o_required is required.
quoted hunk ↗ jump to hunk
err = dl_argv_handle(dl, &opts->bus_name, &opts->dev_name); if (err) return err;@@ -1178,6 +1193,15 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required, if (err) return err; o_found |= DL_OPT_REGION_LENGTH; + } else if (dl_argv_match(dl, "reporter") && + (o_all & DL_OPT_HEALTH_REPORTER_NAME)) { + dl_arg_inc(dl); + err = dl_argv_str(dl, &opts->reporter_name); + if (err) + return err; + o_found |= DL_OPT_HEALTH_REPORTER_NAME; + o_found |= DL_OPT_HANDLE; + break;
why the break? It is the only option that breaks after a match. If it is required, please add a comment why for future readers.
quoted hunk ↗ jump to hunk
} else { pr_err("Unknown option \"%s\"\n", dl_argv(dl)); return -EINVAL;@@ -1298,6 +1322,12 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required, return -EINVAL; } + if ((o_required & DL_OPT_HEALTH_REPORTER_NAME) && + !(o_found & DL_OPT_HEALTH_REPORTER_NAME)) { + pr_err("Reporter name expected.\n"); + return -EINVAL; + }
I realize your are following suit with this change, but these checks for 'required and not found' are getting really long. There is a better way to do this.
quoted hunk ↗ jump to hunk
+ return 0; }@@ -1382,6 +1412,9 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl) if (opts->present & DL_OPT_REGION_LENGTH) mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN, opts->region_length); + if (opts->present & DL_OPT_HEALTH_REPORTER_NAME) + mnl_attr_put_strz(nlh, DEVLINK_ATTR_HEALTH_REPORTER_NAME, + opts->reporter_name); } static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,@@ -1513,6 +1546,8 @@ static void __pr_out_handle_start(struct dl *dl, struct nlattr **tb, __pr_out_newline(); __pr_out_indent_inc(); arr_last_handle_set(dl, bus_name, dev_name); + } else { + __pr_out_indent_inc(); } } else { pr_out("%s%s", buf, content ? ":" : "");@@ -5557,11 +5592,502 @@ static int cmd_region(struct dl *dl) return -ENOENT; } +static int cmd_health_set_params(struct dl *dl) +{ + struct nlmsghdr *nlh; + uint64_t period; + bool auto_recover; + int err; + + nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_HEALTH_REPORTER_SET, + NLM_F_REQUEST | NLM_F_ACK); + err = dl_argv_parse(dl, DL_OPT_HANDLE | + DL_OPT_HEALTH_REPORTER_NAME, 0); + if (err) + return err; + + err = dl_argv_str(dl, &dl->opts.reporter_param_name); + if (err) + return err; + err = dl_argv_str(dl, &dl->opts.reporter_param_value); + if (err) + return err; + dl_opts_put(nlh, dl); + + if (!strncmp(dl->opts.reporter_param_name, + HEALTH_REPORTER_GRACE_PERIOD_STR, strlen("garce"))) { + err = strtouint64_t(dl->opts.reporter_param_value, &period); + if (err) + goto err_param_value_parse; + mnl_attr_put_u64(nlh, DEVLINK_ATTR_HEALTH_REPORTER_PERIOD, + period); + } else if (!strncmp(dl->opts.reporter_param_name, + HEALTH_REPORTER_AUTO_RECOVER_STR, + strlen("auto"))) { + err = strtobool(dl->opts.reporter_param_value, &auto_recover); + if (err) + goto err_param_value_parse; + mnl_attr_put_u8(nlh, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_REC, + (uint8_t)auto_recover); + } else { + printf("Parameter name: %s is not supported\n", + dl->opts.reporter_param_name); + return -ENOTSUP; + } + + return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL); + +err_param_value_parse: + pr_err("Value \"%s\" is not a number or not within range\n", + dl->opts.param_value); + return err; +} + +static int cmd_health_dump_clear(struct dl *dl) +{ + struct nlmsghdr *nlh; + int err; + + nlh = mnlg_msg_prepare(dl->nlg, + DEVLINK_CMD_HEALTH_REPORTER_DUMP_CLEAR, + NLM_F_REQUEST | NLM_F_ACK); + + err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | + DL_OPT_HEALTH_REPORTER_NAME, 0); + if (err) + return err; + + dl_opts_put(nlh, dl); + return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL); +} + +static int health_value_show(struct dl *dl, int type, struct nlattr *nl_data) +{ + const char *str; + uint8_t *data; + uint32_t len; + uint64_t u64; + uint32_t u32; + uint16_t u16; + uint8_t u8; + int i; + + switch (type) { + case MNL_TYPE_FLAG: + if (dl->json_output) + jsonw_string(dl->jw, nl_data ? "true" : "false"); + else + pr_out("%s ", nl_data ? "true" : "false"); + break; + case MNL_TYPE_U8: + u8 = mnl_attr_get_u8(nl_data); + if (dl->json_output) + jsonw_uint(dl->jw, u8); + else + pr_out("%u ", u8); + break; + case MNL_TYPE_U16: + u16 = mnl_attr_get_u16(nl_data); + if (dl->json_output) + jsonw_uint(dl->jw, u16); + else + pr_out("%u ", u16); + break; + case MNL_TYPE_U32: + u32 = mnl_attr_get_u32(nl_data); + if (dl->json_output) + jsonw_uint(dl->jw, u32); + else + pr_out("%u ", u32); + break; + case MNL_TYPE_U64: + u64 = mnl_attr_get_u64(nl_data); + if (dl->json_output) + jsonw_u64(dl->jw, u64); + else + pr_out("%lu ", u64); + break; + case MNL_TYPE_STRING: + case MNL_TYPE_NUL_STRING: + str = mnl_attr_get_str(nl_data); + if (dl->json_output) + jsonw_string(dl->jw, str); + else + pr_out("%s ", str); + break; + case MNL_TYPE_BINARY: + len = mnl_attr_get_payload_len(nl_data); + data = mnl_attr_get_payload(nl_data); + i = 0; + while (i < len) { + if (dl->json_output) + jsonw_printf(dl->jw, "%d", data[i]); + else + pr_out("%02x ", data[i]); + i++; + }
If 'len' is an arbitrary size then line lengths can be ridiculous here.
+ break; + default: + return -EINVAL; + } + return MNL_CB_OK; +} +
...
+static int jiffies_to_logtime(uint64_t jiffies, char *output)
+{
+ struct sysinfo s_info;
+ struct tm *info;
+ time_t now, sec;
+ int err;
+
+ time(&now);
+ info = localtime(&now);
+ strftime(output, HEALTH_REPORTER_TS_LEN, "%Y-%b-%d %H:%M:%S", info);This strftime should really be done in the error path of sysinfo as opposed to every call to this function calling strftime twice.
+ err = sysinfo(&s_info); + if (err) + return err; + /*substruct uptime in sec from now + * add jiffies and 5 minutes factor*/
fix the comment style.
+ sec = now - (s_info.uptime - jiffies/1000) + 300; + info = localtime(&sec); + strftime(output, HEALTH_REPORTER_TS_LEN, "%Y-%b-%d %H:%M:%S", info); + + return 0; +} +