Re: [dpdk-dev] [PATCH v5 4/8]librte_ether:add a common filter API

["Liu, Jijiang" <jijiang.liu@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, October 17, 2014 3:48 PM
> To: Liu, Jijiang
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 4/8]librte_ether:add a common filter API
> 
> 2014-10-17 06:53, Liu, Jijiang:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-10-11 13:55, Jijiang Liu:
> > > > +#define ETH_TUNNEL_FILTER_OMAC  0x01
> > > > +#define ETH_TUNNEL_FILTER_OIP   0x02
> > > > +#define ETH_TUNNEL_FILTER_TENID 0x04 #define
> > > > +ETH_TUNNEL_FILTER_IMAC  0x08 #define ETH_TUNNEL_FILTER_IVLAN
> 0x10
> > > > +#define ETH_TUNNEL_FILTER_IIP   0x20
> > > > +
> > > > +#define RTE_TUNNEL_FLAGS_TO_QUEUE 1
> > >
> > > These values requires some comments.
> > OK, add comments for these MACROs
> > > > +/*
> > > > + * Tunneled filter type
> > > > + */
> > > > +enum rte_tunnel_filter_type {
> > > > +	RTE_TUNNEL_FILTER_TYPE_NONE = 0,
> > > > +	RTE_TUNNEL_FILTER_OIP = ETH_TUNNEL_FILTER_OIP,
> > > > +	RTE_TUNNEL_FILTER_IMAC_IVLAN =
> > > > +		ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN,
> > > > +	RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID =
> > > > +		ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN |
> > > > +		ETH_TUNNEL_FILTER_TENID,
> > > > +	RTE_TUNNEL_FILTER_IMAC_TENID =
> > > > +		ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_TENID,
> > > > +	RTE_TUNNEL_FILTER_IMAC = ETH_TUNNEL_FILTER_IMAC,
> > > > +	RTE_TUNNEL_FILTER_OMAC_TENID_IMAC =
> > > > +		ETH_TUNNEL_FILTER_OMAC | ETH_TUNNEL_FILTER_TENID |
> > > > +		ETH_TUNNEL_FILTER_IMAC,
> > > > +	RTE_TUNNEL_FILTER_IIP = ETH_TUNNEL_FILTER_IIP,
> > > > +	RTE_TUNNEL_FILTER_TYPE_MAX,
> > > > +};
> > >
> > > It's absolutely impossible to understand. Keep in mind the first
> > > goal of an
> > > API: be used (which imply to be understood by users).
> > > And I really don't understand why you define values for combination
> > > of previous flags. Please, keep it simple.
> >
> > The goal of defining values for combination of filter type in order to
> > easily distinguish/check if the mandatory parameters are valid for a
> > specific filter type, for example, if the filter type is
> > RTE_TUNNEL_FILTER_IMAC_IVLAN,  we just need to check if the inner MAC
> > address and inner VLAN ID are valid.
> > To limit sanity checks to valid parameters the rte_tunnel_filter_type
> > enumeration can be replaced/initialized by bit mask.
> >
> > Furthermore, please look at i40e_tunnel_filter_param_check () function
> > in "[PATCH v5 5/8]i40e:implement API of VxLAN packet filter in
> librte_pmd_i40e" patch.
> > static int
> > +i40e_tunnel_filter_param_check(struct i40e_pf *pf,
> > +			struct rte_eth_tunnel_filter_conf *filter) {
> > +    ...
> >
> > +	if ((filter->filter_type & ETH_TUNNEL_FILTER_OMAC) &&
> > +		(is_zero_ether_addr(filter->outer_mac))) {
> > +		PMD_DRV_LOG(ERR, "Cannot add NULL outer MAC
> address\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if ((filter->filter_type & ETH_TUNNEL_FILTER_IMAC) &&
> > +		(is_zero_ether_addr(filter->inner_mac))) {
> > +		PMD_DRV_LOG(ERR, "Cannot add NULL inner MAC
> address\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> >
> > Actually, If you really don't like rte_tunnel_filter_type definition
> > style, and I can change it.
> 
> Yes, you can just  replace this "enum rte_tunnel_filter_type" by an integer
> like uint16_t. It won't change your tests.

Ok,thanks.
> --
> Thomas