From: Leon Romanovsky <leon@kernel.org>
To: Petr Machata <me@pmachata.org>
Cc: David Ahern <dsahern@gmail.com>,
netdev@vger.kernel.org, stephen@networkplumber.org,
john.fastabend@gmail.com, jiri@nvidia.com, idosch@nvidia.com,
Jakub Kicinski <kuba@kernel.org>, Roman Mashak <mrv@mojatatu.com>
Subject: Re: [PATCH iproute2-next v2 03/11] lib: utils: Add print_on_off_bool()
Date: Wed, 4 Nov 2020 10:15:28 +0200 [thread overview]
Message-ID: <20201104081528.GL5429@unreal> (raw)
In-Reply-To: <874km6i28j.fsf@nvidia.com>
On Tue, Nov 03, 2020 at 10:01:32PM +0100, Petr Machata wrote:
>
> Leon Romanovsky <leon@kernel.org> writes:
>
> > On Tue, Nov 03, 2020 at 12:05:20AM +0100, Petr Machata wrote:
> >>
> >> Leon Romanovsky <leon@kernel.org> writes:
> >>
> >> > On Sun, Nov 01, 2020 at 04:55:42PM -0700, David Ahern wrote:
> >> >
> >> >> yes, the rdma utils are using generic function names. The rdma version
> >> >> should be renamed; perhaps rd_print_on_off. That seems to be once common
> >> >> prefix. Added Leon.
> >> >
> >> > I made fast experiment and the output for the code proposed here and existed
> >> > in the RDMAtool - result the same. So the good thing will be to delete the
> >> > function from the RDMA after print_on_off_bool() will be improved.
> >>
> >> The RDMAtool uses literal "on" and "off" as values in JSON, not
> >> booleans. Moving over to print_on_off_bool() would be a breaking change,
> >> which is problematic especially in JSON output.
> >
> > Nothing prohibits us from adding extra parameter to this new
> > function/json logic/json type that will control JSON behavior. Personally,
> > I don't think that json and stdout outputs should be different, e.g. 1/0 for
> > the json and on/off for the stdout.
>
> Emitting on/off in JSON as true booleans (true / false, not 1 / 0) does
> make sense. It's programmatically-consumed interface, the values should
> be of the right type.
As long as you don't need to use those fields to "set .." after that.
>
> On the other hand, having a FP output use literal "on" and "off" makes
> sense as well. It's an obvious reference to the command line, you can
> actually cut'n'paste it back to shell and it will do the right thing.
Maybe it is not so bad to change RDMAtool to general function, this
on/of print is not widely use yet, just need to decide what is the right one.
>
> Many places in iproute2 do do this dual output, and ideally all new
> instances would behave this way as well. So no toggles, please.
Good example why all utilities in iproute2 are better to use same
input/output code and any attempt to make custom variants should be
banned.
>
> >> I think the current function does handle JSON context, what else do
> >> you have in mind?
> >
> > It handles, but does it twice, first time for is_json_context() and
> > second time inside print_bool.
>
> Gotcha.
next prev parent reply other threads:[~2020-11-04 8:15 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-30 12:29 [PATCH iproute2-next v2 00/11] Add a tool for configuration of DCB Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 01/11] Unify batch processing across tools Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 02/11] lib: Add parse_one_of(), parse_on_off() Petr Machata
2020-10-31 15:37 ` David Ahern
2020-10-31 21:25 ` Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 03/11] lib: utils: Add print_on_off_bool() Petr Machata
2020-10-30 16:05 ` Stephen Hemminger
2020-10-31 21:24 ` Petr Machata
2020-10-31 15:38 ` David Ahern
2020-10-31 21:23 ` Petr Machata
2020-11-01 23:55 ` David Ahern
2020-11-02 6:37 ` Leon Romanovsky
2020-11-02 15:10 ` David Ahern
2020-11-02 23:05 ` Petr Machata
2020-11-03 6:24 ` Leon Romanovsky
2020-11-03 21:01 ` Petr Machata
2020-11-04 8:15 ` Leon Romanovsky [this message]
2020-11-05 20:59 ` Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 04/11] lib: Extract from devlink/mnlg a helper, mnlu_socket_open() Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 05/11] lib: Extract from devlink/mnlg a helper, mnlu_msg_prepare() Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 06/11] lib: Extract from devlink/mnlg a helper, mnlu_socket_recv_run() Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 07/11] lib: Extract from iplink_vlan a helper to parse key:value arrays Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 08/11] lib: parse_mapping: Update argc, argv on error Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 09/11] lib: parse_mapping: Recognize a keyword "all" Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 10/11] Add skeleton of a new tool, dcb Petr Machata
2020-10-30 12:29 ` [PATCH iproute2-next v2 11/11] dcb: Add a subtool for the DCB ETS object Petr Machata
2020-10-31 15:51 ` [PATCH iproute2-next v2 00/11] Add a tool for configuration of DCB David Ahern
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201104081528.GL5429@unreal \
--to=leon@kernel.org \
--cc=dsahern@gmail.com \
--cc=idosch@nvidia.com \
--cc=jiri@nvidia.com \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=me@pmachata.org \
--cc=mrv@mojatatu.com \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.