Hi Adrien, > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Wednesday, August 30, 2017 3:39 PM > To: Iremonger, Bernard <bernard.iremon...@intel.com> > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yi...@intel.com>; Ananyev, > Konstantin <konstantin.anan...@intel.com>; Dumitrescu, Cristian > <cristian.dumitre...@intel.com> > Subject: Re: [PATCH v2 3/6] librte_ether: initialise IPv4 protocol mask for > rte_flow > > On Wed, Aug 30, 2017 at 01:28:04PM +0000, Iremonger, Bernard wrote: > > Hi Adrien, > > > > > -----Original Message----- > > > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > > > Sent: Wednesday, August 30, 2017 1:39 PM > > > To: Iremonger, Bernard <bernard.iremon...@intel.com> > > > Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yi...@intel.com>; Ananyev, > > > Konstantin <konstantin.anan...@intel.com>; Dumitrescu, Cristian > > > <cristian.dumitre...@intel.com> > > > Subject: Re: [PATCH v2 3/6] librte_ether: initialise IPv4 protocol > > > mask for rte_flow > > > > > > Hi Bernard, > > > > > > On Fri, Aug 25, 2017 at 05:10:35PM +0100, Bernard Iremonger wrote: > > > > Initialise the next_proto_id mask in the default mask for > > > > rte_flow_item_type_ipv4. > > > > > > > > Signed-off-by: Bernard Iremonger <bernard.iremon...@intel.com> > > > > --- > > > > lib/librte_ether/rte_flow.h | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/lib/librte_ether/rte_flow.h > > > > b/lib/librte_ether/rte_flow.h index bba6169..59c42fa 100644 > > > > --- a/lib/librte_ether/rte_flow.h > > > > +++ b/lib/librte_ether/rte_flow.h > > > > @@ -489,6 +489,7 @@ struct rte_flow_item_ipv4 { #ifndef > > > > __cplusplus static const struct rte_flow_item_ipv4 > rte_flow_item_ipv4_mask = { > > > > .hdr = { > > > > + .next_proto_id = 0xff, > > > > > > Please don't change the default mask to cover this field as it means > > > all rte_flow-based applications that do not provide a specific mask > > > (.mask == NULL) have to always set this field to some valid value. > > > This is not a convenient default behavior. > > > > > > > .src_addr = RTE_BE32(0xffffffff), > > > > .dst_addr = RTE_BE32(0xffffffff), > > > > }, > > > > -- > > > > 1.9.1 > > > > > > > > > > I'll have to NACK this change. The example application should define > > > its own mask if next_proto_id must be always set. > > > > Surely for IPv4 the next_proto_id will always be set to TCP(6) , UDP(17) or > SCTP (132). > > If the mask is 0 for next_proto_id then it is not possible to match on the > protocol. > > Applications normally match the next protocol implicitly by providing it as > the > subsequent item (e.g. in testpmd by writing "eth / ip / tcp" instead of "eth / > ip next_proto_id spec 6"). > > This change forces users to write "eth / ip next_proto_id spec 6 / tcp" or > face > an error due to an uninitialized next_proto_id (which might be garbage due > to uninitialized memory, not just 0). > > > I can define an ipv4_mask in the application if you insist. > > Yes please, a better suggestion would be to rely on the subsequent item > type and not on the value of this field. > > These default masks must only cover basic fields like source/destination > addresses and ports for most protocols. > > -- > Adrien Mazarguil > 6WIND
I will drop this patch and send a v3 patch set. I will define an ipv4_mask in the application. Regards, Bernard.
