| Message ID | 01774dea3239d1127d2e174a48419db4089c4de3.1511805077.git.mprivozn@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | tc: police: fix control action parsing | expand |
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.
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
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.
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
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.
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)
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(-)