| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4fa4b872-1617-a795-bf86-5c02ed0d2feb@gmail.com>
Date: Tue, 22 Jan 2019 20:37:59 -0700
From: David Ahern <dsahern@...il.com>
To: Aya Levin <ayal@...lanox.com>, netdev@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>,
Jiri Pirko <jiri@...lanox.com>
Cc: Moshe Shemesh <moshe@...lanox.com>,
Eran Ben Elisha <eranbe@...lanox.com>,
Tal Alon <talal@...lanox.com>,
Ariel Almog <ariela@...lanox.com>
Subject: Re: [PATCH iproute2-next v2] devlink: Add health command support
On 1/20/19 2:27 AM, Aya Levin wrote:
> 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
> * 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
> 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.
> 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.
> } 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.
> +
> 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;
> +}
> +
Powered by blists - more mailing lists