Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5fd2df286f6d5bf813361cc8c907a155976a5c82.camel@redhat.com>
Subject: Re: [v4,iproute2-next 1/2] iproute2-next:tc:action: add a gate
 control action
From:   Davide Caratti <dcaratti@redhat.com>
To:     Po Liu <Po.Liu@nxp.com>, dsahern@gmail.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc:     vinicius.gomes@intel.com, stephen@networkplumber.org,
        davem@davemloft.net, vlad@buslov.dev, claudiu.manoil@nxp.com,
        vladimir.oltean@nxp.com, alexandru.marginean@nxp.com
In-Reply-To: <20200506084020.18106-1-Po.Liu@nxp.com>
References: <20200503063251.10915-2-Po.Liu@nxp.com>
         <20200506084020.18106-1-Po.Liu@nxp.com>
Organization: red hat
Content-Type: text/plain; charset="UTF-8"
Date:   Wed, 06 May 2020 14:54:26 +0200
MIME-Version: 1.0
User-Agent: Evolution 3.36.1 (3.36.1-1.fc32) 
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 2020-05-06 at 16:40 +0800, Po Liu wrote:
> Introduce a ingress frame gate control flow action.
[...]

hello Po Liu,

[...]

> +create_entry:
> +			e = create_gate_entry(gate_state, interval,
> +					      ipv, maxoctets);
> +			if (!e) {
> +				fprintf(stderr, "gate: not enough memory\n");
> +				free_entries(&gate_entries);
> +				return -1;
> +			}
> +
> +			list_add_tail(&e->list, &gate_entries);
> +			entry_num++;
> +
> +		} else if (matches(*argv, "reclassify") == 0 ||
> +			   matches(*argv, "drop") == 0 ||
> +			   matches(*argv, "shot") == 0 ||
> +			   matches(*argv, "continue") == 0 ||
> +			   matches(*argv, "pass") == 0 ||
> +			   matches(*argv, "ok") == 0 ||
> +			   matches(*argv, "pipe") == 0 ||
> +			   matches(*argv, "goto") == 0) {
> +			if (parse_action_control(&argc, &argv,
> +						 &parm.action, false)) {
> +				free_entries(&gate_entries);
> +				return -1;
> +			}
> +		} else if (matches(*argv, "help") == 0) {
> +			usage();
> +		} else {
> +			break;
> +		}
> +
> +		argc--;
> +		argv++;
> +	}
> +
> +	parse_action_control_dflt(&argc, &argv, &parm.action,
> +				  false, TC_ACT_PIPE);

it seems that the control action is parsed twice, and the first time it
does not allow "jump" and "trap". Is that intentional? IOW, are there some
"act_gate" configurations that don't allow jump or trap?

I don't see anything similar in kernel act_gate.c, where tcf_gate_act()
can return TC_ACT_SHOT or whatever is written in parm.action. That's why
I'm asking, if these two control actions are forbidden you should let the
kernel return -EINVAL with a proper extack in tcf_gate_init(). Can you
please clarify?

thank you in advance!
-- 
davide