From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH iproute2] police: don't skip parameters after actions Date: Mon, 29 Jan 2018 08:07:23 -0800 Message-ID: <20180129080723.5f2b5311@xeon-e3> References: <20180129111311.21610-1-w.bumiller@proxmox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Jiri Pirko To: Wolfgang Bumiller Return-path: Received: from mail-pg0-f52.google.com ([74.125.83.52]:33723 "EHLO mail-pg0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750959AbeA2QH0 (ORCPT ); Mon, 29 Jan 2018 11:07:26 -0500 Received: by mail-pg0-f52.google.com with SMTP id u1so4808275pgr.0 for ; Mon, 29 Jan 2018 08:07:26 -0800 (PST) In-Reply-To: <20180129111311.21610-1-w.bumiller@proxmox.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 29 Jan 2018 12:13:11 +0100 Wolfgang Bumiller wrote: > The 'parse_action_control()' helper advances the argument > pointers to past its parsed action already, so don't > advance it further in 'act_parse_polic()'. > > Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions") > Signed-off-by: Wolfgang Bumiller > --- > Basically parse_action_control() silently added a NEXT_ARG() while the > cases before didn't have one. Not sure whether the goto is okay > style-wise, let me know if you prefer some other solution. > > Example for triggering this: > Specifying a 'flowid X' after a `police ... drop` will skip the 'flowid' > and error with "What is X" > > $ tc filter add dev eth0 parent ffff: basic police rate 13371337bps burst 1337b mtu 64kb drop flowid :1 > What is ":1"? Thank you for the patch. It is a real problem, and your patch addresses it. I just don't like jumping around in the the argument parsing with goto's. There was a similar problem recently, and the better fix was to fix the semantics of the parsing function to not do the extra implicit NEXT_ARG in the parsing logic. There is less likely to be future problems if all parsing functions leave the with the same argument location. Please try that and resubmit.