tc: police: fix control action parsing

Message ID 01774dea3239d1127d2e174a48419db4089c4de3.1511805077.git.mprivozn@redhat.com
State Superseded
Delegated to: stephen hemminger
Headers show
Series
  • tc: police: fix control action parsing
Related show

Commit Message

Michal Privoznik Nov. 27, 2017, 5:51 p.m.
parse_action_control helper does advancing of the arg inside. So don't
do it outside.

Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tc/m_police.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger Nov. 27, 2017, 8:32 p.m. | #1
On Mon, 27 Nov 2017 19:00:14 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> parse_action_control helper does advancing of the arg inside. So don't
> do it outside.
> 
> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

The helpers are not helping here.
Adding another layer of indirection on moving argc/argv then causing caller
to have to keep track is bad design.

Also since pars_action_control_slash is only used by police, why was it
moved into tc_util in the first place? I would prefer just to rip out that
bit and put it back in policer.
Michal Privoznik Nov. 28, 2017, 12:57 p.m. | #2
On 11/27/2017 09:32 PM, Stephen Hemminger wrote:
> On Mon, 27 Nov 2017 19:00:14 +0100
> Michal Privoznik <mprivozn@redhat.com> wrote:
> 
>> parse_action_control helper does advancing of the arg inside. So don't
>> do it outside.
>>
>> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> The helpers are not helping here.
> Adding another layer of indirection on moving argc/argv then causing caller
> to have to keep track is bad design.
> 
> Also since pars_action_control_slash is only used by police, why was it
> moved into tc_util in the first place? I would prefer just to rip out that
> bit and put it back in policer.
> 

I'm not the author of that function so I'll let Jiri to answer that.

Michal
Jiri Pirko Nov. 28, 2017, 1:02 p.m. | #3
Mon, Nov 27, 2017 at 09:32:59PM CET, stephen@networkplumber.org wrote:
>On Mon, 27 Nov 2017 19:00:14 +0100
>Michal Privoznik <mprivozn@redhat.com> wrote:
>
>> parse_action_control helper does advancing of the arg inside. So don't
>> do it outside.
>> 
>> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>
>The helpers are not helping here.
>Adding another layer of indirection on moving argc/argv then causing caller
>to have to keep track is bad design.
>
>Also since pars_action_control_slash is only used by police, why was it
>moved into tc_util in the first place? I would prefer just to rip out that
>bit and put it back in policer.

I tried to make all the x-specific parsing to go away and make all done
in core. That should have been done from the very beginning, we would
lot of mess.
Michal Privoznik Nov. 29, 2017, 9:06 a.m. | #4
On 11/28/2017 02:02 PM, Jiri Pirko wrote:
> Mon, Nov 27, 2017 at 09:32:59PM CET, stephen@networkplumber.org wrote:
>> On Mon, 27 Nov 2017 19:00:14 +0100
>> Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>>> parse_action_control helper does advancing of the arg inside. So don't
>>> do it outside.
>>>
>>> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> The helpers are not helping here.
>> Adding another layer of indirection on moving argc/argv then causing caller
>> to have to keep track is bad design.
>>
>> Also since pars_action_control_slash is only used by police, why was it
>> moved into tc_util in the first place? I would prefer just to rip out that
>> bit and put it back in policer.
> 
> I tried to make all the x-specific parsing to go away and make all done
> in core. That should have been done from the very beginning, we would
> lot of mess.
> 

Okay, would it be a better solution if __parse_action_control() wouldn't
call NEXT_ARG_FWD() at the end? Then this patch wouldn't be needed and
every place that calls parse_action_control() would not need to special
case it.

Just a bit of background:
Libvirt has a capability of setting QoS on bridges/TAPs it manages. And
as part of that it issues the following command to set traffic limiting:

  tc filter add dev virbr0 parent ffff: protocol all u32 match u32 0 0 \
  police rate 50kbps burst 50kb mtu 64kb drop flowid :1

More info can be found in this bug:

  https://bugzilla.redhat.com/show_bug.cgi?id=1514963

Fortunately, no a lot of distros ship 4.13+ so libvirt ain't broken yet.
But soon.

Michal
Stephen Hemminger Dec. 6, 2017, 5:55 p.m. | #5
On Wed, 29 Nov 2017 10:06:26 +0100
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 11/28/2017 02:02 PM, Jiri Pirko wrote:
> > Mon, Nov 27, 2017 at 09:32:59PM CET, stephen@networkplumber.org wrote:  
> >> On Mon, 27 Nov 2017 19:00:14 +0100
> >> Michal Privoznik <mprivozn@redhat.com> wrote:
> >>  
> >>> parse_action_control helper does advancing of the arg inside. So don't
> >>> do it outside.
> >>>
> >>> Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>  
> >>
> >> The helpers are not helping here.
> >> Adding another layer of indirection on moving argc/argv then causing caller
> >> to have to keep track is bad design.
> >>
> >> Also since pars_action_control_slash is only used by police, why was it
> >> moved into tc_util in the first place? I would prefer just to rip out that
> >> bit and put it back in policer.  
> > 
> > I tried to make all the x-specific parsing to go away and make all done
> > in core. That should have been done from the very beginning, we would
> > lot of mess.
> >   
> 
> Okay, would it be a better solution if __parse_action_control() wouldn't
> call NEXT_ARG_FWD() at the end? Then this patch wouldn't be needed and
> every place that calls parse_action_control() would not need to special
> case it.
> 
> Just a bit of background:
> Libvirt has a capability of setting QoS on bridges/TAPs it manages. And
> as part of that it issues the following command to set traffic limiting:
> 
>   tc filter add dev virbr0 parent ffff: protocol all u32 match u32 0 0 \
>   police rate 50kbps burst 50kb mtu 64kb drop flowid :1
> 
> More info can be found in this bug:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1514963
> 
> Fortunately, no a lot of distros ship 4.13+ so libvirt ain't broken yet.
> But soon.
> 
> Michal

Michal, if you and Jiri can work out cleaner parse action semantic that would
be best. If not, then this patch would do.  Let me know next week what you figure out.

Patch

diff --git a/tc/m_police.c b/tc/m_police.c
index ff1dcb7d..62cc6f86 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -75,6 +75,7 @@  int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
 		return -1;
 
 	while (argc > 0) {
+		bool advance = true;
 
 		if (matches(*argv, "index") == 0) {
 			NEXT_ARG();
@@ -154,11 +155,13 @@  int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
 			   matches(*argv, "goto") == 0) {
 			if (parse_action_control(&argc, &argv, &p.action, false))
 				return -1;
+			advance = false;
 		} else if (strcmp(*argv, "conform-exceed") == 0) {
 			NEXT_ARG();
 			if (parse_action_control_slash(&argc, &argv, &p.action,
 						       &presult, true))
 				return -1;
+			advance = false;
 		} else if (matches(*argv, "overhead") == 0) {
 			NEXT_ARG();
 			if (get_u16(&overhead, *argv, 10)) {
@@ -175,7 +178,9 @@  int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
 			break;
 		}
 		ok++;
-		argc--; argv++;
+		if (advance) {
+			argc--; argv++;
+		}
 	}
 
 	if (!ok)