[dpdk-dev] [PATCH v3 11/14] net/mlx5: support MPLS-in-GRE and MPLS-in-UDP

[Nélio Laranjeiro]

On Fri, Apr 13, 2018 at 03:22:50PM +0000, Xueming(Steven) Li wrote:
>[...] 
> > @@
> > > > static
> > > > > const struct mlx5_flow_items mlx5_flow_items[] = {
> > > > >  		.convert = mlx5_flow_create_vxlan_gpe,
> > > > >  		.dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> > > > >  	},
> > > > > +	[RTE_FLOW_ITEM_TYPE_MPLS] = {
> > > > > +		.items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > > > +			       RTE_FLOW_ITEM_TYPE_IPV4,
> > > > > +			       RTE_FLOW_ITEM_TYPE_IPV6),
> > > > > +		.actions = valid_actions,
> > > > > +		.mask = &(const struct rte_flow_item_mpls){
> > > > > +			.label_tc_s = "\xff\xff\xf0",
> > > > > +		},
> > > > > +		.default_mask = &rte_flow_item_mpls_mask,
> > > > > +		.mask_sz = sizeof(struct rte_flow_item_mpls),
> > > > > +		.convert = mlx5_flow_create_mpls, #ifdef
> > > > > +HAVE_IBV_DEVICE_MPLS_SUPPORT
> > > > > +		.dst_sz = sizeof(struct ibv_flow_spec_mpls), #endif
> > > > > +	},
> > > >
> > > > Why the whole item is not under ifdef?
> > >
> > > If apply macro to whole item, there will be a null pointer if create
> > mpls flow.
> > > There is a macro in function mlx5_flow_create_mpls() to avoid using this
> > invalid data.
> > 
> > I think there is some kind of confusion here, what I mean is moving the
> > #ifdef to embrace the whole stuff i.e.:
> > 
> >  #ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT
> >  [RTE_FLOW_ITEM_TYPE_MPLS] = {
> >   .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> >   	       RTE_FLOW_ITEM_TYPE_IPV4,
> >   	       RTE_FLOW_ITEM_TYPE_IPV6),
> >   .actions = valid_actions,
> >   .mask = &(const struct rte_flow_item_mpls){
> >   	.label_tc_s = "\xff\xff\xf0",
> >   },
> >   .default_mask = &rte_flow_item_mpls_mask,
> >   .mask_sz = sizeof(struct rte_flow_item_mpls),
> >   .convert = mlx5_flow_create_mpls,
> >   .dst_sz = sizeof(struct ibv_flow_spec_mpls)  #endif
> > 
> > Not having this item in this static array ends by not supporting it, this
> > is what I mean.
> 
> Yes, I know. There is a code using this array w/o NULL check:
> 		cur_item = &mlx5_flow_items[items->type];
> 		ret = cur_item->convert(items,
> 					(cur_item->default_mask ?
> 					 cur_item->default_mask :
> 					 cur_item->mask),
> 					 &data);
> 
> 

This code is after the mlx5_flow_convert_items_validate() which refuses
unknown items, if you you see an unknown item reaching this code above,
there is bug somewhere and it should be fixed.  Un-supported items
should not be in the static array.  

Regards,

-- 
Nélio Laranjeiro
6WIND