Re: [PATCH iproute2-next 2/6] lib: Move print_rate() from tc here; modernize

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 30 Nov 2020 23:59:38 +0100
Petr Machata <me@pmachata.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@pmachata.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.