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