* [PATCH iproute2] etf: make printing of variable JSON friendly
@ 2019-07-19 21:40 Vedang Patel
2019-07-19 22:26 ` Stephen Hemminger
2019-07-22 18:21 ` David Ahern
0 siblings, 2 replies; 7+ messages in thread
From: Vedang Patel @ 2019-07-19 21:40 UTC (permalink / raw)
To: netdev
Cc: jhs, xiyou.wangcong, jiri, stephen, vinicius.gomes,
leandro.maciel.dorileo, dsahern, Vedang Patel
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 <stephen@networkplumber.org>
Signed-off-by: Vedang Patel <vedang.patel@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;
}
--
2.7.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-19 21:40 [PATCH iproute2] etf: make printing of variable JSON friendly Vedang Patel @ 2019-07-19 22:26 ` Stephen Hemminger 2019-07-22 18:21 ` David Ahern 1 sibling, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2019-07-19 22:26 UTC (permalink / raw) To: Vedang Patel Cc: netdev, jhs, xiyou.wangcong, jiri, vinicius.gomes, leandro.maciel.dorileo, dsahern On Fri, 19 Jul 2019 14:40:43 -0700 Vedang Patel <vedang.patel@intel.com> 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 <stephen@networkplumber.org> > Signed-off-by: Vedang Patel <vedang.patel@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; > } Thanks, but if you are only going to print json boolean if true, then a common way to indicate that something is enabled is to use print_null(). Could you do that instead? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-19 21:40 [PATCH iproute2] etf: make printing of variable JSON friendly Vedang Patel 2019-07-19 22:26 ` Stephen Hemminger @ 2019-07-22 18:21 ` David Ahern 2019-07-22 20:11 ` Patel, Vedang 1 sibling, 1 reply; 7+ messages in thread From: David Ahern @ 2019-07-22 18:21 UTC (permalink / raw) To: Vedang Patel, netdev Cc: jhs, xiyou.wangcong, jiri, stephen, vinicius.gomes, leandro.maciel.dorileo 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 <stephen@networkplumber.org> > Signed-off-by: Vedang Patel <vedang.patel@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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-22 18:21 ` David Ahern @ 2019-07-22 20:11 ` Patel, Vedang 2019-07-23 0:11 ` David Ahern 0 siblings, 1 reply; 7+ messages in thread From: Patel, Vedang @ 2019-07-22 20:11 UTC (permalink / raw) To: David Ahern Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Stephen Hemminger, Gomes, Vinicius, Dorileo, Leandro > On Jul 22, 2019, at 11:21 AM, David Ahern <dsahern@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 <stephen@networkplumber.org> >> Signed-off-by: Vedang Patel <vedang.patel@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? Thanks, Vedang ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-22 20:11 ` Patel, Vedang @ 2019-07-23 0:11 ` David Ahern 2019-07-23 21:34 ` Patel, Vedang 0 siblings, 1 reply; 7+ messages in thread From: David Ahern @ 2019-07-23 0:11 UTC (permalink / raw) To: Patel, Vedang Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Stephen Hemminger, Gomes, Vinicius, Dorileo, Leandro On 7/22/19 1:11 PM, Patel, Vedang wrote: > > >> On Jul 22, 2019, at 11:21 AM, David Ahern <dsahern@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 <stephen@networkplumber.org> >>> Signed-off-by: Vedang Patel <vedang.patel@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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-23 0:11 ` David Ahern @ 2019-07-23 21:34 ` Patel, Vedang 2019-07-23 23:23 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Patel, Vedang @ 2019-07-23 21:34 UTC (permalink / raw) To: David Ahern Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Stephen Hemminger, Gomes, Vinicius, Dorileo, Leandro > On Jul 22, 2019, at 5:11 PM, David Ahern <dsahern@gmail.com> wrote: > > On 7/22/19 1:11 PM, Patel, Vedang wrote: >> >> >>> On Jul 22, 2019, at 11:21 AM, David Ahern <dsahern@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 <stephen@networkplumber.org> >>>> Signed-off-by: Vedang Patel <vedang.patel@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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] etf: make printing of variable JSON friendly 2019-07-23 21:34 ` Patel, Vedang @ 2019-07-23 23:23 ` Stephen Hemminger 0 siblings, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2019-07-23 23:23 UTC (permalink / raw) To: Patel, Vedang Cc: David Ahern, netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Gomes, Vinicius, Dorileo, Leandro On Tue, 23 Jul 2019 21:34:46 +0000 "Patel, Vedang" <vedang.patel@intel.com> wrote: > > On Jul 22, 2019, at 5:11 PM, David Ahern <dsahern@gmail.com> wrote: > > > > On 7/22/19 1:11 PM, Patel, Vedang wrote: > >> > >> > >>> On Jul 22, 2019, at 11:21 AM, David Ahern <dsahern@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 <stephen@networkplumber.org> > >>>> Signed-off-by: Vedang Patel <vedang.patel@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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-23 23:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-19 21:40 [PATCH iproute2] etf: make printing of variable JSON friendly Vedang Patel 2019-07-19 22:26 ` Stephen Hemminger 2019-07-22 18:21 ` David Ahern 2019-07-22 20:11 ` Patel, Vedang 2019-07-23 0:11 ` David Ahern 2019-07-23 21:34 ` Patel, Vedang 2019-07-23 23:23 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).