diff mbox series

[iproute2-next,v2,03/11] lib: utils: Add print_on_off_bool()

Message ID 5ed9e2e7cdf9326e8f7ec80f33f0f11eafc3a425.1604059429.git.me@pmachata.org
State New
Headers show
Series None | expand

Commit Message

Petr Machata Oct. 30, 2020, 12:29 p.m. UTC
The value of a number of booleans is shown as "on" and "off" in the plain
output, and as an actual boolean in JSON mode. Add a function that does
that.

Signed-off-by: Petr Machata <me@pmachata.org>
---
 include/utils.h | 1 +
 lib/utils.c     | 8 ++++++++
 2 files changed, 9 insertions(+)

Comments

Stephen Hemminger Oct. 30, 2020, 4:05 p.m. UTC | #1
On Fri, 30 Oct 2020 13:29:50 +0100
Petr Machata <me@pmachata.org> wrote:

> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

> +{

> +	if (is_json_context())

> +		print_bool(PRINT_JSON, flag, NULL, val);

> +	else

> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

> +}


Please use print_string(PRINT_FP for non json case.
I am use fprintf(fp as indicator for code that has not been already
converted to JSON.

And passing the FILE *fp is also indication of old code.
Original iproute2 passed it around but it was always stdout.
David Ahern Oct. 31, 2020, 3:38 p.m. UTC | #2
On 10/30/20 6:29 AM, Petr Machata wrote:
> diff --git a/include/utils.h b/include/utils.h

> index bd62cdcd7122..e3cdb098834a 100644

> --- a/include/utils.h

> +++ b/include/utils.h

> @@ -328,5 +328,6 @@ int do_batch(const char *name, bool force,

>  int parse_one_of(const char *msg, const char *realval, const char * const *list,

>  		 size_t len, int *p_err);

>  int parse_on_off(const char *msg, const char *realval, int *p_err);

> +void print_on_off_bool(FILE *fp, const char *flag, bool val);

>  

>  #endif /* __UTILS_H__ */

> diff --git a/lib/utils.c b/lib/utils.c

> index 930877ae0f0d..8deec86ecbcd 100644

> --- a/lib/utils.c

> +++ b/lib/utils.c

> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

>  

>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

>  }

> +

> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

> +{

> +	if (is_json_context())

> +		print_bool(PRINT_JSON, flag, NULL, val);

> +	else

> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

> +}

> 


I think print_on_off should be fine and aligns with parse_on_off once it
returns a bool.
Petr Machata Oct. 31, 2020, 9:23 p.m. UTC | #3
David Ahern <dsahern@gmail.com> writes:

> On 10/30/20 6:29 AM, Petr Machata wrote:

>> diff --git a/lib/utils.c b/lib/utils.c

>> index 930877ae0f0d..8deec86ecbcd 100644

>> --- a/lib/utils.c

>> +++ b/lib/utils.c

>> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

>>

>>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

>>  }

>> +

>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

>> +{

>> +	if (is_json_context())

>> +		print_bool(PRINT_JSON, flag, NULL, val);

>> +	else

>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

>> +}

>>

>

> I think print_on_off should be fine and aligns with parse_on_off once it

> returns a bool.


print_on_off() is already used in the RDMA tool, and actually outputs
"on" and "off", unlike this. So I chose this instead.

I could rename the RDMA one though -- it's used in two places, whereas
this is used in about two dozen instances across the codebase.
Petr Machata Oct. 31, 2020, 9:24 p.m. UTC | #4
Stephen Hemminger <stephen@networkplumber.org> writes:

> On Fri, 30 Oct 2020 13:29:50 +0100

> Petr Machata <me@pmachata.org> wrote:

>

>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

>> +{

>> +	if (is_json_context())

>> +		print_bool(PRINT_JSON, flag, NULL, val);

>> +	else

>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

>> +}

>

> Please use print_string(PRINT_FP for non json case.

> I am use fprintf(fp as indicator for code that has not been already

> converted to JSON.

>

> And passing the FILE *fp is also indication of old code.

> Original iproute2 passed it around but it was always stdout.


Ack, will fix.
David Ahern Nov. 1, 2020, 11:55 p.m. UTC | #5
On 10/31/20 3:23 PM, Petr Machata wrote:
> 

> David Ahern <dsahern@gmail.com> writes:

> 

>> On 10/30/20 6:29 AM, Petr Machata wrote:

>>> diff --git a/lib/utils.c b/lib/utils.c

>>> index 930877ae0f0d..8deec86ecbcd 100644

>>> --- a/lib/utils.c

>>> +++ b/lib/utils.c

>>> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

>>>

>>>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

>>>  }

>>> +

>>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

>>> +{

>>> +	if (is_json_context())

>>> +		print_bool(PRINT_JSON, flag, NULL, val);

>>> +	else

>>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

>>> +}

>>>

>>

>> I think print_on_off should be fine and aligns with parse_on_off once it

>> returns a bool.

> 

> print_on_off() is already used in the RDMA tool, and actually outputs

> "on" and "off", unlike this. So I chose this instead.

> 

> I could rename the RDMA one though -- it's used in two places, whereas

> this is used in about two dozen instances across the codebase.

> 


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.
Leon Romanovsky Nov. 2, 2020, 6:37 a.m. UTC | #6
On Sun, Nov 01, 2020 at 04:55:42PM -0700, David Ahern wrote:
> On 10/31/20 3:23 PM, Petr Machata wrote:

> >

> > David Ahern <dsahern@gmail.com> writes:

> >

> >> On 10/30/20 6:29 AM, Petr Machata wrote:

> >>> diff --git a/lib/utils.c b/lib/utils.c

> >>> index 930877ae0f0d..8deec86ecbcd 100644

> >>> --- a/lib/utils.c

> >>> +++ b/lib/utils.c

> >>> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

> >>>

> >>>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

> >>>  }

> >>> +

> >>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

> >>> +{

> >>> +	if (is_json_context())

> >>> +		print_bool(PRINT_JSON, flag, NULL, val);

> >>> +	else

> >>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

> >>> +}

> >>>

> >>

> >> I think print_on_off should be fine and aligns with parse_on_off once it

> >> returns a bool.

> >

> > print_on_off() is already used in the RDMA tool, and actually outputs

> > "on" and "off", unlike this. So I chose this instead.

> >

> > I could rename the RDMA one though -- it's used in two places, whereas

> > this is used in about two dozen instances across the codebase.

> >

>

> 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.

However I don't understand why print_on_off_bool() is implemented in
utils.c and not in lib/json_print.c that properly handles JSON context,
provide colorized output and doesn't require to supply FILE *fp.

Thanks
David Ahern Nov. 2, 2020, 3:10 p.m. UTC | #7
On 11/1/20 11:37 PM, Leon Romanovsky wrote:
> However I don't understand why print_on_off_bool() is implemented in

> utils.c and not in lib/json_print.c that properly handles JSON context,

> provide colorized output and doesn't require to supply FILE *fp.


Good point, I missed that earlier.
Petr Machata Nov. 2, 2020, 11:05 p.m. UTC | #8
Leon Romanovsky <leon@kernel.org> writes:

> On Sun, Nov 01, 2020 at 04:55:42PM -0700, David Ahern wrote:

>> On 10/31/20 3:23 PM, Petr Machata wrote:

>> >

>> > David Ahern <dsahern@gmail.com> writes:

>> >

>> >> On 10/30/20 6:29 AM, Petr Machata wrote:

>> >>> diff --git a/lib/utils.c b/lib/utils.c

>> >>> index 930877ae0f0d..8deec86ecbcd 100644

>> >>> --- a/lib/utils.c

>> >>> +++ b/lib/utils.c

>> >>> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

>> >>>

>> >>>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

>> >>>  }

>> >>> +

>> >>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

>> >>> +{

>> >>> +	if (is_json_context())

>> >>> +		print_bool(PRINT_JSON, flag, NULL, val);

>> >>> +	else

>> >>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

>> >>> +}

>> >>>

>> >>

>> >> I think print_on_off should be fine and aligns with parse_on_off once it

>> >> returns a bool.

>> >

>> > print_on_off() is already used in the RDMA tool, and actually outputs

>> > "on" and "off", unlike this. So I chose this instead.

>> >

>> > I could rename the RDMA one though -- it's used in two places, whereas

>> > this is used in about two dozen instances across the codebase.

>> >

>>

>> 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.

> However I don't understand why print_on_off_bool() is implemented in

> utils.c and not in lib/json_print.c that properly handles JSON context,


There's a whole lot of print_X functions for printing non-fundamental
data types in utils.c. Seemed obvious to put it there. I can move it to
json_print.c, no problem.

I think the current function does handle JSON context, what else do
you have in mind?

> provide colorized output and doesn't require to supply FILE *fp.


Stephen Hemminger already pointed out the FILE *fp bit, I'll be removing
it.
Leon Romanovsky Nov. 3, 2020, 6:24 a.m. UTC | #9
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:

> >> On 10/31/20 3:23 PM, Petr Machata wrote:

> >> >

> >> > David Ahern <dsahern@gmail.com> writes:

> >> >

> >> >> On 10/30/20 6:29 AM, Petr Machata wrote:

> >> >>> diff --git a/lib/utils.c b/lib/utils.c

> >> >>> index 930877ae0f0d..8deec86ecbcd 100644

> >> >>> --- a/lib/utils.c

> >> >>> +++ b/lib/utils.c

> >> >>> @@ -1763,3 +1763,11 @@ int parse_on_off(const char *msg, const char *realval, int *p_err)

> >> >>>

> >> >>>  	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);

> >> >>>  }

> >> >>> +

> >> >>> +void print_on_off_bool(FILE *fp, const char *flag, bool val)

> >> >>> +{

> >> >>> +	if (is_json_context())

> >> >>> +		print_bool(PRINT_JSON, flag, NULL, val);

> >> >>> +	else

> >> >>> +		fprintf(fp, "%s %s ", flag, val ? "on" : "off");

> >> >>> +}

> >> >>>

> >> >>

> >> >> I think print_on_off should be fine and aligns with parse_on_off once it

> >> >> returns a bool.

> >> >

> >> > print_on_off() is already used in the RDMA tool, and actually outputs

> >> > "on" and "off", unlike this. So I chose this instead.

> >> >

> >> > I could rename the RDMA one though -- it's used in two places, whereas

> >> > this is used in about two dozen instances across the codebase.

> >> >

> >>

> >> 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.

>

> > However I don't understand why print_on_off_bool() is implemented in

> > utils.c and not in lib/json_print.c that properly handles JSON context,

>

> There's a whole lot of print_X functions for printing non-fundamental

> data types in utils.c. Seemed obvious to put it there. I can move it to

> json_print.c, no problem.


It looks like it is worth to cleanup util.c and make sure that all
prints are gathered in one place. Out of the scope for this series.

>

> I think the current function does handle JSON context, what else do

> you have in mind?a


It handles, but does it twice, first time for is_json_context() and
second time inside print_bool.

>

> > provide colorized output and doesn't require to supply FILE *fp.

>

> Stephen Hemminger already pointed out the FILE *fp bit, I'll be removing

> it.
Petr Machata Nov. 3, 2020, 9:01 p.m. UTC | #10
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.

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.

Many places in iproute2 do do this dual output, and ideally all new
instances would behave this way as well. So no toggles, please.

>> 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.
Leon Romanovsky Nov. 4, 2020, 8:15 a.m. UTC | #11
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.
Petr Machata Nov. 5, 2020, 8:59 p.m. UTC | #12
Leon Romanovsky <leon@kernel.org> writes:

> 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

OK, if you think the API breakage is acceptable, I'll roll this into the
patchset.

> just need to decide what is the right one.

Yeah, it's kinda fuzzy. Where JSON has an obvious type to use, use it:
arrays should probably be arrays, numbers should probably be numbers.
I'm not so sure about enums, but I guess represent them as strings? As
numbers they will not be more meaningful or easy to consume, and it does
not make sense to do arithmetic on enums.

On/off toggles could be considered enums. But they are also booleans.
Representing on as true and off as false is straightforward and from
this perspective booleans are the obvious type to use.

>> 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.

Yes, I have a clean-up patch that converts these custom on/off helpers
to the new central one. I'll send this together with other refactorings
of this sort after this patch set.
diff mbox series

Patch

diff --git a/include/utils.h b/include/utils.h
index bd62cdcd7122..e3cdb098834a 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -328,5 +328,6 @@  int do_batch(const char *name, bool force,
 int parse_one_of(const char *msg, const char *realval, const char * const *list,
 		 size_t len, int *p_err);
 int parse_on_off(const char *msg, const char *realval, int *p_err);
+void print_on_off_bool(FILE *fp, const char *flag, bool val);
 
 #endif /* __UTILS_H__ */
diff --git a/lib/utils.c b/lib/utils.c
index 930877ae0f0d..8deec86ecbcd 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1763,3 +1763,11 @@  int parse_on_off(const char *msg, const char *realval, int *p_err)
 
 	return parse_one_of(msg, realval, values_on_off, ARRAY_SIZE(values_on_off), p_err);
 }
+
+void print_on_off_bool(FILE *fp, const char *flag, bool val)
+{
+	if (is_json_context())
+		print_bool(PRINT_JSON, flag, NULL, val);
+	else
+		fprintf(fp, "%s %s ", flag, val ? "on" : "off");
+}