> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Friday, October 17, 2014 4:10 AM
> To: Liu, Jijiang
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 4/8]librte_ether:add a common filter API
>
> I don't review the common API as it should be done in an unique place and
> there are many copies in different patchsets. Let's focus on tunnels.
>
> 2014-10-11 13:55, Jijiang Liu:
> > +/**** TUNNEL FILTER DATA DEFINATION *** */
>
> We cannot miss this comment :)
>
> > +#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.
> --
> Thomas
