| 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
| ||
|
Date: Wed, 23 Jan 2019 11:27:56 +0000
From: Aya Levin <ayal@...lanox.com>
To: David Ahern <dsahern@...il.com>,
"netdev@...r.kernel.org" <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
נכתב על ידי David Ahern, ב־1/23/2019 בשעה 5:37 AM:
> 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.
typo should have been o_all
>
>
>> 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.
Do you mean somthing like:
#define requiered_not_found(o_found, o_required, flag, msg) \
({ \
if ((o_required & flag) && !(o_found & flag)) { \
pr_err("%s \n", msg); \
return -EINVAL; \
} \
})
>
>
>> +
>> 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