netdev - Re: [PATCH iproute2-next 2/6] lib: Move print

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201130163547.23a06e79@hermes.local>
Date:   Mon, 30 Nov 2020 16:35:47 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Petr Machata <me@...chata.org>
Cc:     netdev@...r.kernel.org, dsahern@...il.com, Po.Liu@....com,
        toke@...e.dk, dave.taht@...il.com, edumazet@...gle.com,
        tahiliani@...k.edu.in, vtlam@...gle.com, leon@...nel.org
Subject: Re: [PATCH iproute2-next 2/6] lib: Move print_rate() from tc here;
 modernize

On Mon, 30 Nov 2020 23:59:38 +0100
Petr Machata <me@...chata.org> wrote:

> The functions print_rate() and sprint_rate() are useful for formatting
> rate-like values. The DCB tool would find these useful in the maxrate
> subtool. However, the current interface to these functions uses a global
> variable use_iec as a flag indicating whether 1024- or 1000-based powers
> should be used when formatting the rate value. For general use, a global
> variable is not a great way of passing arguments to a function. Besides, it
> is unlike most other printing functions in that it deals in buffers and
> ignores JSON.
> 
> Therefore make the interface to print_rate() explicit by converting use_iec
> to an ordinary parameter. Since the interface changes anyway, convert it to
> follow the pattern of other json_print functions (except for the
> now-explicit use_iec parameter). Move to json_print.c.
> 
> Add a wrapper to tc, so that all the call sites do not need to repeat the
> use_iec global variable argument, and convert all call sites.
> 
> In q_cake.c, the conversion is not straightforward due to usage of a macro
> that is shared across numerous data types. Simply hand-roll the
> corresponding code, which seems better than making an extra helper for one
> call site.
> 
> Drop sprint_rate() now that everybody just uses print_rate().
> 
> Signed-off-by: Petr Machata <me@...chata.org>
> ---
>  include/json_print.h | 10 ++++++++++
>  lib/json_print.c     | 32 ++++++++++++++++++++++++++++++++
>  tc/m_police.c        |  9 ++++-----
>  tc/q_cake.c          | 28 ++++++++++++++++------------
>  tc/q_cbq.c           | 14 ++++----------
>  tc/q_fq.c            | 25 +++++++++----------------
>  tc/q_hfsc.c          |  4 ++--
>  tc/q_htb.c           |  4 ++--
>  tc/q_mqprio.c        |  8 ++++----
>  tc/q_netem.c         |  4 +---
>  tc/q_tbf.c           |  7 ++-----
>  tc/tc_util.c         | 37 +++++++------------------------------
>  tc/tc_util.h         |  4 ++--
>  13 files changed, 95 insertions(+), 91 deletions(-)
> 
> diff --git a/include/json_print.h b/include/json_print.h
> index 096a999a4de4..b6c4c0c80833 100644
> --- a/include/json_print.h
> +++ b/include/json_print.h
> @@ -86,4 +86,14 @@ _PRINT_NAME_VALUE_FUNC(uint, unsigned int, u);
>  _PRINT_NAME_VALUE_FUNC(string, const char*, s);
>  #undef _PRINT_NAME_VALUE_FUNC
>  
> +int print_color_rate(bool use_iec, enum output_type t, enum color_attr color,
> +		     const char *key, const char *fmt, unsigned long long rate);
> +
> +static inline int print_rate(bool use_iec, enum output_type t,
> +			     const char *key, const char *fmt,
> +			     unsigned long long rate)
> +{
> +	return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate);
> +}
> +

Overall this looks good, but is there any case where color output
makes sense for this field? If not then why do all the color wrappers.