Re: [dpdk-dev] [PATCH v5 2/6] net/mlx5: support basic flow items and actions

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi Ferruh,

On Fri, Jan 06, 2017 at 01:52:53PM +0000, Ferruh Yigit wrote:
> On 1/4/2017 6:42 PM, Adrien Mazarguil wrote:
> > Hi Ferruh,
> > 
> > On Wed, Jan 04, 2017 at 05:49:46PM +0000, Ferruh Yigit wrote:
> >> Hi Nelio,
> >>
> >> A quick question.
> > 
> > I'll reply since it's related to the API.
> > 
> >> On 12/29/2016 3:15 PM, Nelio Laranjeiro wrote:
> >>> Introduce initial software for rte_flow rules.
> >>>
> >>> VLAN, VXLAN are still not supported.
> >>>
> >>> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> >>> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>
> >> <...>
> >>
> >>> +static int
> >>> +priv_flow_validate(struct priv *priv,
> >>> +		   const struct rte_flow_attr *attr,
> >>> +		   const struct rte_flow_item items[],
> >>> +		   const struct rte_flow_action actions[],
> >>> +		   struct rte_flow_error *error,
> >>> +		   struct mlx5_flow *flow)
> >>> +{
> >>> +	const struct mlx5_flow_items *cur_item = mlx5_flow_items;
> >>
> >> <...>
> >>
> >>> +	for (; items->type != RTE_FLOW_ITEM_TYPE_END; ++items) {
> >> <...>
> >>> +	}
> >>> +	for (; actions->type != RTE_FLOW_ACTION_TYPE_END; ++actions) {
> >> <...>
> >>> +	}
> >>
> >> Is it guarantied in somewhere that items or actions terminated with
> >> TYPE_END?
> > 
> > Yes, since it's now the only way to terminate items/actions lists [1][2].
> > There used to be a "max" value in the original draft but it seemed redundant
> > and proved annoying to use, and was therefore dropped.
> > 
> > END items/actions behave like a NUL terminator for C strings. They are
> > likewise defined with value 0 for convenience.
> 
> At least it is good idea to set END values to 0, but still if user not
> set it, most probably this will crash the app.
> 
> Although most probably this kind of error will be detected easily in
> development phase, still it would be nice to return an error instead of
> crashing when user provide wrong input.

Unfortunately I cannot think of an easy way to do that, even for debugging
purposes, this would be like checking for unterminated strings or linked
lists without a NULL ending pointer. That's the trade-off of any unbounded
data structure.

Note PMDs will likely return errors as they iterate on garbage item/action
types, crashes will also almost always occur when attempting to dereference
the related spec/last/mask/conf pointers.

> >> And these fields are direct inputs from user.
> >> Is there a way to verify user provided values are with TYPE_END terminated?
> > 
> > No, applications must check for its presence (they normally add it
> > themselves) before feeding these lists to PMDs. I think that's safe enough.
> > 
> > Note the testpmd flow command does not allow entering a flow rule without
> > "end" tokens in both lists, there is no way around this restriction.
> > 
> > [1] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#matching-pattern
> > [2] http://dpdk.org/doc/guides/prog_guide/rte_flow.html#actions

-- 
Adrien Mazarguil
6WIND