On Tue, 23 Jul 2019 21:34:46 +0000 "Patel, Vedang" <vedang.pa...@intel.com> wrote:
> > On Jul 22, 2019, at 5:11 PM, David Ahern <dsah...@gmail.com> wrote: > > > > On 7/22/19 1:11 PM, Patel, Vedang wrote: > >> > >> > >>> On Jul 22, 2019, at 11:21 AM, David Ahern <dsah...@gmail.com> wrote: > >>> > >>> On 7/19/19 3:40 PM, Vedang Patel wrote: > >>>> In iproute2 txtime-assist series, it was pointed out that print_bool() > >>>> should be used to print binary values. This is to make it JSON friendly. > >>>> > >>>> So, make the corresponding changes in ETF. > >>>> > >>>> Fixes: 8ccd49383cdc ("etf: Add skip_sock_check") > >>>> Reported-by: Stephen Hemminger <step...@networkplumber.org> > >>>> Signed-off-by: Vedang Patel <vedang.pa...@intel.com> > >>>> --- > >>>> tc/q_etf.c | 12 ++++++------ > >>>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/tc/q_etf.c b/tc/q_etf.c > >>>> index c2090589bc64..307c50eed48b 100644 > >>>> --- a/tc/q_etf.c > >>>> +++ b/tc/q_etf.c > >>>> @@ -176,12 +176,12 @@ static int etf_print_opt(struct qdisc_util *qu, > >>>> FILE *f, struct rtattr *opt) > >>>> get_clock_name(qopt->clockid)); > >>>> > >>>> print_uint(PRINT_ANY, "delta", "delta %d ", qopt->delta); > >>>> - print_string(PRINT_ANY, "offload", "offload %s ", > >>>> - (qopt->flags & TC_ETF_OFFLOAD_ON) ? > >>>> "on" : "off"); > >>>> - print_string(PRINT_ANY, "deadline_mode", "deadline_mode %s ", > >>>> - (qopt->flags & TC_ETF_DEADLINE_MODE_ON) > >>>> ? "on" : "off"); > >>>> - print_string(PRINT_ANY, "skip_sock_check", "skip_sock_check %s", > >>>> - (qopt->flags & TC_ETF_SKIP_SOCK_CHECK) > >>>> ? "on" : "off"); > >>>> + if (qopt->flags & TC_ETF_OFFLOAD_ON) > >>>> + print_bool(PRINT_ANY, "offload", "offload ", true); > >>>> + if (qopt->flags & TC_ETF_DEADLINE_MODE_ON) > >>>> + print_bool(PRINT_ANY, "deadline_mode", "deadline_mode > >>>> ", true); > >>>> + if (qopt->flags & TC_ETF_SKIP_SOCK_CHECK) > >>>> + print_bool(PRINT_ANY, "skip_sock_check", > >>>> "skip_sock_check", true); > >>>> > >>>> return 0; > >>>> } > >>>> > >>> > >>> This changes existing output for TC_ETF_OFFLOAD_ON and > >>> TC_ETF_DEADLINE_MODE_ON which were added a year ago. > >> Yes, this is a good point. I missed that. > >> > >> Another idea is to use is_json_context() and call print_bool() there. But, > >> that will still change values corresponding to the json output for the > >> above flags from “on”/“off” to “true”/“false”. I am not sure if this is a > >> big issue. > >> > >> My suggestion is to keep the code as is. what do you think? > >> > > > > I think we need automated checkers for new code. ;-) > > > > The first 2 should not change for backward compatibility - unless there > > is agreement that this feature is too new and long term it is better to > > print as above. > > > > Then the new one should follow context of the other 2 - consistency IMHO > > takes precedence. > Thanks for the inputs. > > Let’s keep whatever is currently present upstream and you can ignore this > patch. > > Thanks, > Vedang Agreed. At this point consistency is better. Maybe at some future point, all the JSON will be reviewed and fixed (yes it would be a breaking flag day). But for now inconsistent usage across ip, tc, and devlink is a fact of life.