| Message ID | 1563572443-10879-1-git-send-email-vedang.patel@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] etf: make printing of variable JSON friendly | expand |
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?
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.
> 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
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.
> 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
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.
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; }
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(-)