From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS, UNWANTED_LANGUAGE_BODY autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29C76C282C0 for ; Wed, 23 Jan 2019 03:38:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CAECA217D4 for ; Wed, 23 Jan 2019 03:38:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="HBzAnMlb" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726952AbfAWDiE (ORCPT ); Tue, 22 Jan 2019 22:38:04 -0500 Received: from mail-pl1-f196.google.com ([209.85.214.196]:33578 "EHLO mail-pl1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726843AbfAWDiE (ORCPT ); Tue, 22 Jan 2019 22:38:04 -0500 Received: by mail-pl1-f196.google.com with SMTP id z23so433011plo.0 for ; Tue, 22 Jan 2019 19:38:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=3GUX6xiRWzKNbodJeLWTp0G2n5ypJYr2P3WWH2ewEMo=; b=HBzAnMlbsdZ/87zZK+919Mqf17rrdvoRJgi+8bPtkHc0wg5yAHTKWSHRuLBHAhZ1mw kqHzFsko3UJJJb9gBj1Azt9TlArnT9XI2x/jH/vkUAQv5azI8Vb6Qa5Bom5QkvtzcYu8 iliBCxCkNqubbxKDOnQUhW0ty5vCjfLc38lieGWwK1OTdnGoZE/ueLIp8bxHnqkWh15K ICEvb/v00S/JQSUyF24XdCMZKkDjxkBj+l6q4Hx80WHQX7xhVAT3JGZ1HIQGQgLGVP75 Px8eAPatCgfIoG93Nb8GmGdExykkfQkAHyeY9rtdUfZvHbgl4gw8823x1CUSflSr7BSN mseQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3GUX6xiRWzKNbodJeLWTp0G2n5ypJYr2P3WWH2ewEMo=; b=hXJdLmsHNmKO2haNVGqcghepSl0PjOs0pe9tUcZQ20BnU3SuLo0+iMWFfnLgabajYl BJnC5LXN5nf4qzSGu6qRyOZrZ7pSNsFIEAbpLK0j8bx0kO+zG9yzXnG9C0CzMTG20Qk/ uWIskjqu/RrNWPa/QwNXPdkiZZn0aD/7dEl68XoM2ydNkcpLYMefDKT1Umy94lysycan i2YduBQs4TkXzJiuYsS8kPraI/ZLT8Mnbk9PgScMunrCI8gZturkCP/XKWa5+IjOmoGx r27sbN7Uym2cn98gUKlC6oc3kCch3Y3QKEWgLWIoak2l5VHOKItTccXeqkDiqVy9GqHZ BvCA== X-Gm-Message-State: AJcUukeJl23iw1nDicshOS0MWxXqmI08Rg4STG4cyTIbW9QeeJy1potl 28fCwmqevEj0BWHsZMH00dTTyxo9 X-Google-Smtp-Source: ALg8bN4qWHp7jZORrTHKVIhCPJSqWMHPcn9o/NCDvR8KwMOZN4Wqzzd2FsfnfbWjH0LDOYyiNeAgzg== X-Received: by 2002:a17:902:b282:: with SMTP id u2mr619743plr.89.1548214682901; Tue, 22 Jan 2019 19:38:02 -0800 (PST) Received: from ?IPv6:2601:282:800:fd80:1169:5cd2:eb20:96a2? ([2601:282:800:fd80:1169:5cd2:eb20:96a2]) by smtp.googlemail.com with ESMTPSA id l5sm28010152pgu.86.2019.01.22.19.38.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 22 Jan 2019 19:38:01 -0800 (PST) Subject: Re: [PATCH iproute2-next v2] devlink: Add health command support To: Aya Levin , netdev@vger.kernel.org, "David S. Miller" , Jiri Pirko Cc: Moshe Shemesh , Eran Ben Elisha , Tal Alon , Ariel Almog References: <1547762360-7075-1-git-send-email-eranbe@mellanox.com> <1547976434-4142-1-git-send-email-ayal@mellanox.com> From: David Ahern Message-ID: <4fa4b872-1617-a795-bf86-5c02ed0d2feb@gmail.com> Date: Tue, 22 Jan 2019 20:37:59 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <1547976434-4142-1-git-send-email-ayal@mellanox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > #include > #include > - > +#include > #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; > +} > +