Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH iproute2-next v4 1/1] police: Add support for json output
@ 2021-06-07  6:44 Roi Dayan
  2021-06-11  2:35 ` David Ahern
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Roi Dayan @ 2021-06-07  6:44 UTC (permalink / raw)
  To: netdev
  Cc: Roi Dayan, Paul Blakey, David Ahern, Stephen Hemminger, Jamal Hadi Salim

Change to use the print wrappers instead of fprintf().

This is example output of the options part before this commit:

        "options": {
            "handle": 1,
            "in_hw": true,
            "actions": [ {
                    "order": 1 police 0x2 ,
                    "control_action": {
                        "type": "drop"
                    },
                    "control_action": {
                        "type": "continue"
                    }overhead 0b linklayer unspec
        ref 1 bind 1
,
                    "used_hw_stats": [ "delayed" ]
                } ]
        }

This is the output of the same dump with this commit:

        "options": {
            "handle": 1,
            "in_hw": true,
            "actions": [ {
                    "order": 1,
                    "kind": "police",
                    "index": 2,
                    "control_action": {
                        "type": "drop"
                    },
                    "control_action": {
                        "type": "continue"
                    },
                    "overhead": 0,
                    "linklayer": "unspec",
                    "ref": 1,
                    "bind": 1,
                    "used_hw_stats": [ "delayed" ]
                } ]
        }

Signed-off-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Paul Blakey <paulb@nvidia.com>
---

Notes:
    v2
    - fix json output to match correctly the other actions
      i.e. output the action name in key 'kind' and unsigned for the index
    
    v3
    - print errors to stderr.
    - return -1 on null key.
    
    v4
    - removed left over debug that was forgotten. sorry for that.

 tc/m_police.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/tc/m_police.c b/tc/m_police.c
index 9ef0e40b861b..2594c08979e0 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -278,18 +278,19 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
 	__u64 rate64, prate64;
 	__u64 pps64, ppsburst64;
 
+	print_string(PRINT_ANY, "kind", "%s", "police");
 	if (arg == NULL)
 		return 0;
 
 	parse_rtattr_nested(tb, TCA_POLICE_MAX, arg);
 
 	if (tb[TCA_POLICE_TBF] == NULL) {
-		fprintf(f, "[NULL police tbf]");
-		return 0;
+		fprintf(stderr, "[NULL police tbf]");
+		return -1;
 	}
 #ifndef STOOPID_8BYTE
 	if (RTA_PAYLOAD(tb[TCA_POLICE_TBF])  < sizeof(*p)) {
-		fprintf(f, "[truncated police tbf]");
+		fprintf(stderr, "[truncated police tbf]");
 		return -1;
 	}
 #endif
@@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
 	    RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64))
 		rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]);
 
-	fprintf(f, " police 0x%x ", p->index);
+	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);
 	tc_print_rate(PRINT_FP, NULL, "rate %s ", rate64);
 	buffer = tc_calc_xmitsize(rate64, p->burst);
 	print_size(PRINT_FP, NULL, "burst %s ", buffer);
 	print_size(PRINT_FP, NULL, "mtu %s ", p->mtu);
 	if (show_raw)
-		fprintf(f, "[%08x] ", p->burst);
+		print_hex(PRINT_FP, NULL, "[%08x] ", p->burst);
 
 	prate64 = p->peakrate.rate;
 	if (tb[TCA_POLICE_PEAKRATE64] &&
@@ -327,8 +328,8 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
 		pps64 = rta_getattr_u64(tb[TCA_POLICE_PKTRATE64]);
 		ppsburst64 = rta_getattr_u64(tb[TCA_POLICE_PKTBURST64]);
 		ppsburst64 = tc_calc_xmitsize(pps64, ppsburst64);
-		fprintf(f, "pkts_rate %llu ", pps64);
-		fprintf(f, "pkts_burst %llu ", ppsburst64);
+		print_u64(PRINT_ANY, "pkts_rate", "pkts_rate %llu ", pps64);
+		print_u64(PRINT_ANY, "pkts_burst", "pkts_burst %llu ", ppsburst64);
 	}
 
 	print_action_control(f, "action ", p->action, "");
@@ -337,14 +338,17 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
 		__u32 action = rta_getattr_u32(tb[TCA_POLICE_RESULT]);
 
 		print_action_control(f, "/", action, " ");
-	} else
-		fprintf(f, " ");
+	} else {
+		print_string(PRINT_FP, NULL, " ", NULL);
+	}
 
-	fprintf(f, "overhead %ub ", p->rate.overhead);
+	print_uint(PRINT_ANY, "overhead", "overhead %u ", p->rate.overhead);
 	linklayer = (p->rate.linklayer & TC_LINKLAYER_MASK);
 	if (linklayer > TC_LINKLAYER_ETHERNET || show_details)
-		fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b2));
-	fprintf(f, "\n\tref %d bind %d", p->refcnt, p->bindcnt);
+		print_string(PRINT_ANY, "linklayer", "linklayer %s ",
+			     sprint_linklayer(linklayer, b2));
+	print_int(PRINT_ANY, "ref", "ref %d ", p->refcnt);
+	print_int(PRINT_ANY, "bind", "bind %d ", p->bindcnt);
 	if (show_stats) {
 		if (tb[TCA_POLICE_TM]) {
 			struct tcf_t *tm = RTA_DATA(tb[TCA_POLICE_TM]);
@@ -352,7 +356,7 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
 			print_tm(f, tm);
 		}
 	}
-	fprintf(f, "\n");
+	print_nl();
 
 
 	return 0;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-06-07  6:44 [PATCH iproute2-next v4 1/1] police: Add support for json output Roi Dayan
@ 2021-06-11  2:35 ` David Ahern
  2021-07-05 10:41 ` Hangbin Liu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: David Ahern @ 2021-06-11  2:35 UTC (permalink / raw)
  To: Roi Dayan, netdev; +Cc: Paul Blakey, Stephen Hemminger, Jamal Hadi Salim

On 6/7/21 12:44 AM, Roi Dayan wrote:
> Change to use the print wrappers instead of fprintf().
> 
> This is example output of the options part before this commit:
> 
>         "options": {
>             "handle": 1,
>             "in_hw": true,
>             "actions": [ {
>                     "order": 1 police 0x2 ,
>                     "control_action": {
>                         "type": "drop"
>                     },
>                     "control_action": {
>                         "type": "continue"
>                     }overhead 0b linklayer unspec
>         ref 1 bind 1
> ,
>                     "used_hw_stats": [ "delayed" ]
>                 } ]
>         }
> 
> This is the output of the same dump with this commit:
> 
>         "options": {
>             "handle": 1,
>             "in_hw": true,
>             "actions": [ {
>                     "order": 1,
>                     "kind": "police",
>                     "index": 2,
>                     "control_action": {
>                         "type": "drop"
>                     },
>                     "control_action": {
>                         "type": "continue"
>                     },
>                     "overhead": 0,
>                     "linklayer": "unspec",
>                     "ref": 1,
>                     "bind": 1,
>                     "used_hw_stats": [ "delayed" ]
>                 } ]
>         }
> 
> Signed-off-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Paul Blakey <paulb@nvidia.com>
> ---
> 
> Notes:
>     v2
>     - fix json output to match correctly the other actions
>       i.e. output the action name in key 'kind' and unsigned for the index
>     
>     v3
>     - print errors to stderr.
>     - return -1 on null key.
>     
>     v4
>     - removed left over debug that was forgotten. sorry for that.
> 
>  tc/m_police.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 

applied to iproute2-next


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-06-07  6:44 [PATCH iproute2-next v4 1/1] police: Add support for json output Roi Dayan
  2021-06-11  2:35 ` David Ahern
@ 2021-07-05 10:41 ` Hangbin Liu
  2021-07-06  8:27   ` Davide Caratti
  2021-07-05 16:28 ` Stephen Hemminger
  2021-07-05 16:29 ` Stephen Hemminger
  3 siblings, 1 reply; 17+ messages in thread
From: Hangbin Liu @ 2021-07-05 10:41 UTC (permalink / raw)
  To: Roi Dayan
  Cc: netdev, Paul Blakey, David Ahern, Stephen Hemminger,
	Jamal Hadi Salim, Roman Mashak, Davide Caratti, Baowen Zheng

On Mon, Jun 07, 2021 at 09:44:08AM +0300, Roi Dayan wrote:
> Change to use the print wrappers instead of fprintf().
> 
> This is example output of the options part before this commit:
> 
>         "options": {
>             "handle": 1,
>             "in_hw": true,
>             "actions": [ {
>                     "order": 1 police 0x2 ,
>                     "control_action": {
>                         "type": "drop"
>                     },
>                     "control_action": {
>                         "type": "continue"
>                     }overhead 0b linklayer unspec
>         ref 1 bind 1
> ,
>                     "used_hw_stats": [ "delayed" ]
>                 } ]
>         }
> 
> This is the output of the same dump with this commit:
> 
>         "options": {
>             "handle": 1,
>             "in_hw": true,
>             "actions": [ {
>                     "order": 1,
>                     "kind": "police",
>                     "index": 2,
>                     "control_action": {
>                         "type": "drop"
>                     },
>                     "control_action": {
>                         "type": "continue"
>                     },
>                     "overhead": 0,
>                     "linklayer": "unspec",
>                     "ref": 1,
>                     "bind": 1,
>                     "used_hw_stats": [ "delayed" ]
>                 } ]
>         }
> 
> Signed-off-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Paul Blakey <paulb@nvidia.com>
> ---

[...]
> 
> @@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
>  	    RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64))
>  		rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]);
>  
> -	fprintf(f, " police 0x%x ", p->index);
> +	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);

Hi everyone,

This update break all policy checking in kernel tc selftest actions/police.json.
As the new output would like 

total acts 1

        action order 0: police   index 1 rate 1Kbit burst 10Kb mtu 2Kb action reclassify overhead 0 ref 1 bind 0


And the current test checks like

	"matchPattern": "action order [0-9]*:  police 0x1 rate 1Kbit burst 10Kb"

I plan to update the kselftest to mach the new output. But I have a question.
Why need we add a "\t" before index output? Is it needed or could be removed?

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-06-07  6:44 [PATCH iproute2-next v4 1/1] police: Add support for json output Roi Dayan
  2021-06-11  2:35 ` David Ahern
  2021-07-05 10:41 ` Hangbin Liu
@ 2021-07-05 16:28 ` Stephen Hemminger
  2021-07-05 16:30   ` David Ahern
  2021-07-05 16:29 ` Stephen Hemminger
  3 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2021-07-05 16:28 UTC (permalink / raw)
  To: Roi Dayan; +Cc: netdev, Paul Blakey, David Ahern, Jamal Hadi Salim

On Mon, 7 Jun 2021 09:44:08 +0300
Roi Dayan <roid@nvidia.com> wrote:

> -	fprintf(f, " police 0x%x ", p->index);
> +	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);

Why did output format have to change here? Why not:

	print_hex(PRINT_ANY, "police", " police %#x", p->index);

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-06-07  6:44 [PATCH iproute2-next v4 1/1] police: Add support for json output Roi Dayan
                   ` (2 preceding siblings ...)
  2021-07-05 16:28 ` Stephen Hemminger
@ 2021-07-05 16:29 ` Stephen Hemminger
  3 siblings, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2021-07-05 16:29 UTC (permalink / raw)
  To: Roi Dayan; +Cc: netdev, Paul Blakey, David Ahern, Jamal Hadi Salim

On Mon, 7 Jun 2021 09:44:08 +0300
Roi Dayan <roid@nvidia.com> wrote:

> -	fprintf(f, "\n\tref %d bind %d", p->refcnt, p->bindcnt);
> +		print_string(PRINT_ANY, "linklayer", "linklayer %s ",
> +			     sprint_linklayer(linklayer, b2));
> +	print_int(PRINT_ANY, "ref", "ref %d ", p->refcnt);
> +	print_int(PRINT_ANY, "bind", "bind %d ", p->bindcnt);
>  	if (show_stats) {

This should add newline like original by using print_nl()

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-05 16:28 ` Stephen Hemminger
@ 2021-07-05 16:30   ` David Ahern
  2021-07-06  8:30     ` Roi Dayan
  0 siblings, 1 reply; 17+ messages in thread
From: David Ahern @ 2021-07-05 16:30 UTC (permalink / raw)
  To: Stephen Hemminger, Roi Dayan; +Cc: netdev, Paul Blakey, Jamal Hadi Salim

On 7/5/21 10:28 AM, Stephen Hemminger wrote:
> On Mon, 7 Jun 2021 09:44:08 +0300
> Roi Dayan <roid@nvidia.com> wrote:
> 
>> -	fprintf(f, " police 0x%x ", p->index);
>> +	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);
> 
> Why did output format have to change here? Why not:
> 
> 	print_hex(PRINT_ANY, "police", " police %#x", p->index);
> 

it should not have. I caught it in the first version in a review
comment; missed it in v4 that was applied.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-05 10:41 ` Hangbin Liu
@ 2021-07-06  8:27   ` Davide Caratti
  2021-07-07  6:53     ` Hangbin Liu
  0 siblings, 1 reply; 17+ messages in thread
From: Davide Caratti @ 2021-07-06  8:27 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: Roi Dayan, netdev, Paul Blakey, David Ahern, Stephen Hemminger,
	Jamal Hadi Salim, Roman Mashak, Baowen Zheng

On Mon, Jul 05, 2021 at 06:41:37PM +0800, Hangbin Liu wrote:
> On Mon, Jun 07, 2021 at 09:44:08AM +0300, Roi Dayan wrote:
> > Change to use the print wrappers instead of fprintf().
> > 
> > Signed-off-by: Roi Dayan <roid@nvidia.com>
> > Reviewed-by: Paul Blakey <paulb@nvidia.com>
> > ---

hello Hangbin,
 
[...]
> > 
> > @@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg)
> >  	    RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64))
> >  		rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]);
> >  
> > -	fprintf(f, " police 0x%x ", p->index);
> > +	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);
> 
> Hi everyone,
> 
> This update break all policy checking in kernel tc selftest actions/police.json.
> As the new output would like 

thanks for catching this!

> total acts 1
> 
>         action order 0: police   index 1 rate 1Kbit burst 10Kb mtu 2Kb action reclassify overhead 0 ref 1 bind 0
> 
> 
> And the current test checks like
> 
> 	"matchPattern": "action order [0-9]*:  police 0x1 rate 1Kbit burst 10Kb"
> 
>  I plan to update the kselftest to mach the new output.

my 2 cents:

what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and
preserve the human-readable printout iproute and kselftests? besides avoiding failures because
of mismatching kselftests / iproute, this would preserve functionality of scripts that
configure / dump the "police" action. WDYT?

thanks,
-- 
davide


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-05 16:30   ` David Ahern
@ 2021-07-06  8:30     ` Roi Dayan
  2021-07-06  8:36       ` Roi Dayan
  0 siblings, 1 reply; 17+ messages in thread
From: Roi Dayan @ 2021-07-06  8:30 UTC (permalink / raw)
  To: David Ahern, Stephen Hemminger; +Cc: netdev, Paul Blakey, Jamal Hadi Salim



On 2021-07-05 7:30 PM, David Ahern wrote:
> On 7/5/21 10:28 AM, Stephen Hemminger wrote:
>> On Mon, 7 Jun 2021 09:44:08 +0300
>> Roi Dayan <roid@nvidia.com> wrote:
>>
>>> -	fprintf(f, " police 0x%x ", p->index);
>>> +	print_uint(PRINT_ANY, "index", "\t index %u ", p->index);
>>
>> Why did output format have to change here? Why not:
>>
>> 	print_hex(PRINT_ANY, "police", " police %#x", p->index);
>>
> 
> it should not have. I caught it in the first version in a review
> comment; missed it in v4 that was applied.
> 

Hi,

I replied to your review in v0 that I wanted to match all the other
actions output which output as unsigned.
Since I didn't get another reply I thought its ok to continue and sent
another version which other changes that were required.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-06  8:30     ` Roi Dayan
@ 2021-07-06  8:36       ` Roi Dayan
  0 siblings, 0 replies; 17+ messages in thread
From: Roi Dayan @ 2021-07-06  8:36 UTC (permalink / raw)
  To: David Ahern, Stephen Hemminger; +Cc: netdev, Paul Blakey, Jamal Hadi Salim



On 2021-07-06 11:30 AM, Roi Dayan wrote:
> 
> 
> On 2021-07-05 7:30 PM, David Ahern wrote:
>> On 7/5/21 10:28 AM, Stephen Hemminger wrote:
>>> On Mon, 7 Jun 2021 09:44:08 +0300
>>> Roi Dayan <roid@nvidia.com> wrote:
>>>
>>>> -    fprintf(f, " police 0x%x ", p->index);
>>>> +    print_uint(PRINT_ANY, "index", "\t index %u ", p->index);
>>>
>>> Why did output format have to change here? Why not:
>>>
>>>     print_hex(PRINT_ANY, "police", " police %#x", p->index);
>>>
>>
>> it should not have. I caught it in the first version in a review
>> comment; missed it in v4 that was applied.
>>
> 
> Hi,
> 
> I replied to your review in v0 that I wanted to match all the other
> actions output which output as unsigned.
> Since I didn't get another reply I thought its ok to continue and sent
> another version which other changes that were required.
> 
> 

beside the unsigned the json output in all other actions use "index"
as far as I notice.
the action name, e.g. "police", being printed under "kind" also to
match the other actions.
So I wanted the json output for police to match with same keys as
the other actions. at least the keys "kind" and "index" which all
have and not each action use different key.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-06  8:27   ` Davide Caratti
@ 2021-07-07  6:53     ` Hangbin Liu
  2021-07-08  6:57       ` Roi Dayan
  0 siblings, 1 reply; 17+ messages in thread
From: Hangbin Liu @ 2021-07-07  6:53 UTC (permalink / raw)
  To: Davide Caratti
  Cc: Roi Dayan, netdev, Paul Blakey, David Ahern, Stephen Hemminger,
	Jamal Hadi Salim, Roman Mashak, Baowen Zheng

On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
> my 2 cents:
> 
> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and
> preserve the human-readable printout iproute and kselftests? besides avoiding failures because
> of mismatching kselftests / iproute, this would preserve functionality of scripts that
> configure / dump the "police" action. WDYT?

+1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-07  6:53     ` Hangbin Liu
@ 2021-07-08  6:57       ` Roi Dayan
  2021-07-08  7:23         ` Roi Dayan
  2021-07-08 14:46         ` David Ahern
  0 siblings, 2 replies; 17+ messages in thread
From: Roi Dayan @ 2021-07-08  6:57 UTC (permalink / raw)
  To: Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, David Ahern, Stephen Hemminger,
	Jamal Hadi Salim, Roman Mashak, Baowen Zheng



On 2021-07-07 9:53 AM, Hangbin Liu wrote:
> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
>> my 2 cents:
>>
>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and
>> preserve the human-readable printout iproute and kselftests? besides avoiding failures because
>> of mismatching kselftests / iproute, this would preserve functionality of scripts that
>> configure / dump the "police" action. WDYT?
> 
> +1
> 


why not fix the kselftest to look for the correct output?

all actions output unsigned as the index.
though I did find an issue with the fp output that you pasted
that I missed.


         action order 0: police   index 1 rate 1Kbit burst 10Kb mtu 2Kb 
action reclassify overhead 0 ref 1 bind 0


You asked about the \t before index and actually there is a missing
print_nl() call before printing index and the rest as in the other
actions.

then the match should be something like this

	"matchPattern": "action order [0-9]*: police.*index 0x1 rate 1Kbit 
burst 10Kb"

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-08  6:57       ` Roi Dayan
@ 2021-07-08  7:23         ` Roi Dayan
  2021-07-08 14:46         ` David Ahern
  1 sibling, 0 replies; 17+ messages in thread
From: Roi Dayan @ 2021-07-08  7:23 UTC (permalink / raw)
  To: Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, David Ahern, Stephen Hemminger,
	Jamal Hadi Salim, Roman Mashak, Baowen Zheng



On 2021-07-08 9:57 AM, Roi Dayan wrote:
> 
> 
> On 2021-07-07 9:53 AM, Hangbin Liu wrote:
>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
>>> my 2 cents:
>>>
>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output 
>>> only to show "index", and
>>> preserve the human-readable printout iproute and kselftests? besides 
>>> avoiding failures because
>>> of mismatching kselftests / iproute, this would preserve 
>>> functionality of scripts that
>>> configure / dump the "police" action. WDYT?
>>
>> +1
>>
> 
> 
> why not fix the kselftest to look for the correct output?
> 
> all actions output unsigned as the index.
> though I did find an issue with the fp output that you pasted
> that I missed.
> 
> 
>          action order 0: police   index 1 rate 1Kbit burst 10Kb mtu 2Kb 
> action reclassify overhead 0 ref 1 bind 0
> 
> 
> You asked about the \t before index and actually there is a missing
> print_nl() call before printing index and the rest as in the other
> actions.
> 
> then the match should be something like this
> 
>      "matchPattern": "action order [0-9]*: police.*index 0x1 rate 1Kbit 
> burst 10Kb"

actually its

    "matchPattern": "action order [0-9]*: police.*index 1 rate 1Kbit 
burst 10Kb"

also i found some selftest fail for "overhead" matching as i used
print_uint() instead of print_size().

let me send a fix for this with fix for the kselftest of police.json
i think its better to have the consistent output of all actions
instead of police using hex for the instance number.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-08  6:57       ` Roi Dayan
  2021-07-08  7:23         ` Roi Dayan
@ 2021-07-08 14:46         ` David Ahern
  2021-07-11 10:24           ` Roi Dayan
  1 sibling, 1 reply; 17+ messages in thread
From: David Ahern @ 2021-07-08 14:46 UTC (permalink / raw)
  To: Roi Dayan, Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, Stephen Hemminger, Jamal Hadi Salim,
	Roman Mashak, Baowen Zheng

On 7/8/21 12:57 AM, Roi Dayan wrote:
> 
> 
> On 2021-07-07 9:53 AM, Hangbin Liu wrote:
>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
>>> my 2 cents:
>>>
>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output
>>> only to show "index", and
>>> preserve the human-readable printout iproute and kselftests? besides
>>> avoiding failures because
>>> of mismatching kselftests / iproute, this would preserve
>>> functionality of scripts that
>>> configure / dump the "police" action. WDYT?
>>
>> +1
>>
> 
> 
> why not fix the kselftest to look for the correct output?

That is but 1 user. The general rule is that you do not change the
output like you did.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-08 14:46         ` David Ahern
@ 2021-07-11 10:24           ` Roi Dayan
  2021-07-11 16:00             ` David Ahern
  0 siblings, 1 reply; 17+ messages in thread
From: Roi Dayan @ 2021-07-11 10:24 UTC (permalink / raw)
  To: David Ahern, Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, Stephen Hemminger, Jamal Hadi Salim,
	Roman Mashak, Baowen Zheng



On 2021-07-08 5:46 PM, David Ahern wrote:
> On 7/8/21 12:57 AM, Roi Dayan wrote:
>>
>>
>> On 2021-07-07 9:53 AM, Hangbin Liu wrote:
>>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
>>>> my 2 cents:
>>>>
>>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output
>>>> only to show "index", and
>>>> preserve the human-readable printout iproute and kselftests? besides
>>>> avoiding failures because
>>>> of mismatching kselftests / iproute, this would preserve
>>>> functionality of scripts that
>>>> configure / dump the "police" action. WDYT?
>>>
>>> +1
>>>
>>
>>
>> why not fix the kselftest to look for the correct output?
> 
> That is but 1 user. The general rule is that you do not change the
> output like you did.
> 

but the output was "broken" and not consistent with all actions.
we are not fixing this kind of thing?
so to continue with the suggestion to use print_fp and keep police
action output broken and print_json for the json output?
just to be sure before submitting change back to old output for fp.


...
         action order 1:  police 0x1 rate 1Mbit burst 20Kb mtu 2Kb 
action reclassify overhead 0b




-       print_string(PRINT_ANY, "kind", "%s", "police");
+       print_string(PRINT_JSON, "kind", "%s", "police");

-       print_uint(PRINT_ANY, "index", "\tindex %u ", p->index);
+       print_hex(PRINT_FP, NULL, " police 0x%x ", p->index);
+       print_uint(PRINT_JSON, "index", NULL, p->index);



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-11 10:24           ` Roi Dayan
@ 2021-07-11 16:00             ` David Ahern
  2021-07-12 11:02               ` Jamal Hadi Salim
  0 siblings, 1 reply; 17+ messages in thread
From: David Ahern @ 2021-07-11 16:00 UTC (permalink / raw)
  To: Roi Dayan, Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, Stephen Hemminger, Jamal Hadi Salim,
	Roman Mashak, Baowen Zheng

On 7/11/21 4:24 AM, Roi Dayan wrote:
> 
> 
> On 2021-07-08 5:46 PM, David Ahern wrote:
>> On 7/8/21 12:57 AM, Roi Dayan wrote:
>>>
>>>
>>> On 2021-07-07 9:53 AM, Hangbin Liu wrote:
>>>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote:
>>>>> my 2 cents:
>>>>>
>>>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output
>>>>> only to show "index", and
>>>>> preserve the human-readable printout iproute and kselftests? besides
>>>>> avoiding failures because
>>>>> of mismatching kselftests / iproute, this would preserve
>>>>> functionality of scripts that
>>>>> configure / dump the "police" action. WDYT?
>>>>
>>>> +1
>>>>
>>>
>>>
>>> why not fix the kselftest to look for the correct output?
>>
>> That is but 1 user. The general rule is that you do not change the
>> output like you did.
>>
> 
> but the output was "broken" and not consistent with all actions.
> we are not fixing this kind of thing?

It has been in hex since 2004, and you can not decide in 2021 that it is
'broken' and change it.

> so to continue with the suggestion to use print_fp and keep police
> action output broken and print_json for the json output?
> just to be sure before submitting change back to old output for fp.
> 
> 
> ...
>         action order 1:  police 0x1 rate 1Mbit burst 20Kb mtu 2Kb action
> reclassify overhead 0b
> 
> 
> 
> 
> -       print_string(PRINT_ANY, "kind", "%s", "police");
> +       print_string(PRINT_JSON, "kind", "%s", "police");
> 
> -       print_uint(PRINT_ANY, "index", "\tindex %u ", p->index);
> +       print_hex(PRINT_FP, NULL, " police 0x%x ", p->index);
> +       print_uint(PRINT_JSON, "index", NULL, p->index);
> 
> 

Jamal: opinions?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-11 16:00             ` David Ahern
@ 2021-07-12 11:02               ` Jamal Hadi Salim
  2021-07-12 12:28                 ` Roi Dayan
  0 siblings, 1 reply; 17+ messages in thread
From: Jamal Hadi Salim @ 2021-07-12 11:02 UTC (permalink / raw)
  To: David Ahern, Roi Dayan, Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, Stephen Hemminger, Roman Mashak, Baowen Zheng

On 2021-07-11 12:00 p.m., David Ahern wrote:

>> ...
>>          action order 1:  police 0x1 rate 1Mbit burst 20Kb mtu 2Kb action
>> reclassify overhead 0b
>>
>>
>>
>>
>> -       print_string(PRINT_ANY, "kind", "%s", "police");
>> +       print_string(PRINT_JSON, "kind", "%s", "police");
>>
>> -       print_uint(PRINT_ANY, "index", "\tindex %u ", p->index);
>> +       print_hex(PRINT_FP, NULL, " police 0x%x ", p->index);
>> +       print_uint(PRINT_JSON, "index", NULL, p->index);
>>
>>
> 
> Jamal: opinions?

Looks good to me. Roi please run the kernel tests to make sure
nothing breaks.

cheers,
jamal


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH iproute2-next v4 1/1] police: Add support for json output
  2021-07-12 11:02               ` Jamal Hadi Salim
@ 2021-07-12 12:28                 ` Roi Dayan
  0 siblings, 0 replies; 17+ messages in thread
From: Roi Dayan @ 2021-07-12 12:28 UTC (permalink / raw)
  To: Jamal Hadi Salim, David Ahern, Hangbin Liu, Davide Caratti
  Cc: netdev, Paul Blakey, Stephen Hemminger, Roman Mashak, Baowen Zheng



On 2021-07-12 2:02 PM, Jamal Hadi Salim wrote:
> On 2021-07-11 12:00 p.m., David Ahern wrote:
> 
>>> ...
>>>          action order 1:  police 0x1 rate 1Mbit burst 20Kb mtu 2Kb 
>>> action
>>> reclassify overhead 0b
>>>
>>>
>>>
>>>
>>> -       print_string(PRINT_ANY, "kind", "%s", "police");
>>> +       print_string(PRINT_JSON, "kind", "%s", "police");
>>>
>>> -       print_uint(PRINT_ANY, "index", "\tindex %u ", p->index);
>>> +       print_hex(PRINT_FP, NULL, " police 0x%x ", p->index);
>>> +       print_uint(PRINT_JSON, "index", NULL, p->index);
>>>
>>>
>>
>> Jamal: opinions?
> 
> Looks good to me. Roi please run the kernel tests to make sure
> nothing breaks.
> 
> cheers,
> jamal
> 

ok
I sent patch titled "police: Fix normal output back to what it was"
I ran the tdc tests and passed.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-07-12 12:29 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-07  6:44 [PATCH iproute2-next v4 1/1] police: Add support for json output Roi Dayan
2021-06-11  2:35 ` David Ahern
2021-07-05 10:41 ` Hangbin Liu
2021-07-06  8:27   ` Davide Caratti
2021-07-07  6:53     ` Hangbin Liu
2021-07-08  6:57       ` Roi Dayan
2021-07-08  7:23         ` Roi Dayan
2021-07-08 14:46         ` David Ahern
2021-07-11 10:24           ` Roi Dayan
2021-07-11 16:00             ` David Ahern
2021-07-12 11:02               ` Jamal Hadi Salim
2021-07-12 12:28                 ` Roi Dayan
2021-07-05 16:28 ` Stephen Hemminger
2021-07-05 16:30   ` David Ahern
2021-07-06  8:30     ` Roi Dayan
2021-07-06  8:36       ` Roi Dayan
2021-07-05 16:29 ` 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).