* [PATCH] tc: police: fix control action parsing
@ 2017-11-27 17:51 Michal Privoznik
2017-11-27 20:32 ` [PATCH iproute2] " Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Michal Privoznik @ 2017-11-27 17:51 UTC (permalink / raw)
To: netdev; +Cc: jiri
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(-)
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)
--
2.13.6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH iproute2] tc: police: fix control action parsing
2017-11-27 17:51 [PATCH] tc: police: fix control action parsing Michal Privoznik
@ 2017-11-27 20:32 ` Stephen Hemminger
2017-11-28 12:57 ` Michal Privoznik
2017-11-28 13:02 ` Jiri Pirko
0 siblings, 2 replies; 6+ messages in thread
From: Stephen Hemminger @ 2017-11-27 20:32 UTC (permalink / raw)
To: Michal Privoznik; +Cc: netdev, jiri
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH iproute2] tc: police: fix control action parsing
2017-11-27 20:32 ` [PATCH iproute2] " Stephen Hemminger
@ 2017-11-28 12:57 ` Michal Privoznik
2017-11-28 13:02 ` Jiri Pirko
1 sibling, 0 replies; 6+ messages in thread
From: Michal Privoznik @ 2017-11-28 12:57 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, jiri
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH iproute2] tc: police: fix control action parsing
2017-11-27 20:32 ` [PATCH iproute2] " Stephen Hemminger
2017-11-28 12:57 ` Michal Privoznik
@ 2017-11-28 13:02 ` Jiri Pirko
2017-11-29 9:06 ` Michal Privoznik
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Pirko @ 2017-11-28 13:02 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Michal Privoznik, netdev, jiri
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH iproute2] tc: police: fix control action parsing
2017-11-28 13:02 ` Jiri Pirko
@ 2017-11-29 9:06 ` Michal Privoznik
2017-12-06 17:55 ` Stephen Hemminger
0 siblings, 1 reply; 6+ messages in thread
From: Michal Privoznik @ 2017-11-29 9:06 UTC (permalink / raw)
To: Jiri Pirko, Stephen Hemminger; +Cc: netdev, jiri
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH iproute2] tc: police: fix control action parsing
2017-11-29 9:06 ` Michal Privoznik
@ 2017-12-06 17:55 ` Stephen Hemminger
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Hemminger @ 2017-12-06 17:55 UTC (permalink / raw)
To: Michal Privoznik; +Cc: Jiri Pirko, netdev, jiri
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-06 17:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-27 17:51 [PATCH] tc: police: fix control action parsing Michal Privoznik
2017-11-27 20:32 ` [PATCH iproute2] " Stephen Hemminger
2017-11-28 12:57 ` Michal Privoznik
2017-11-28 13:02 ` Jiri Pirko
2017-11-29 9:06 ` Michal Privoznik
2017-12-06 17:55 ` 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).