From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Privoznik Subject: Re: [PATCH iproute2] tc: police: fix control action parsing Date: Wed, 29 Nov 2017 10:06:26 +0100 Message-ID: <16bd923d-f725-e455-f8c3-43708b233564@redhat.com> References: <01774dea3239d1127d2e174a48419db4089c4de3.1511805077.git.mprivozn@redhat.com> <20171127123259.324f20cf@xeon-e3> <20171128130202.GC1971@nanopsycho.orion> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, jiri@mellanox.com To: Jiri Pirko , Stephen Hemminger Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37812 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752802AbdK2JG3 (ORCPT ); Wed, 29 Nov 2017 04:06:29 -0500 In-Reply-To: <20171128130202.GC1971@nanopsycho.orion> Content-Language: en-GB Sender: netdev-owner@vger.kernel.org List-ID: 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 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 >> >> 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