Re: [dpdk-dev] [PATCH 2/4] net/ice: rework for generic flow enabling

["Wang, Ying A" <ying.a.wang@intel.com>]

> -----Original Message-----
> From: Ye, Xiaolong
> Sent: Monday, September 9, 2019 5:54 PM
> To: Wang, Ying A <ying.a.wang@intel.com>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; dev@dpdk.org; Zhao1, Wei <wei.zhao1@intel.com>
> Subject: Re: [PATCH 2/4] net/ice: rework for generic flow enabling
> 
> On 09/09, Wang, Ying A wrote:
> >> >+ice_unregister_parser(struct ice_flow_parser *parser,
> >> >+		struct ice_adapter *ad)
> >> >+{
> >> >+	struct ice_pf *pf = &ad->pf;
> >> >+	struct ice_parser_list *list;
> >> >+	struct ice_flow_parser *p_parser;
> >> >+	void *temp;
> >> >+
> >> >+	switch (parser->stage) {
> >> >+	case ICE_FLOW_STAGE_RSS:
> >> >+		list = &pf->rss_parser_list;
> >> >+		break;
> >> >+	case ICE_FLOW_STAGE_PERMISSION:
> >> >+		list = &pf->perm_parser_list;
> >> >+		break;
> >> >+	case ICE_FLOW_STAGE_DISTRIBUTOR:
> >> >+		list = &pf->dist_parser_list;
> >> >+		break;
> >> >+	default:
> >> >+		return;
> >> >+	}
> >>
> >> The switch blocks in above functions are the same, it's better to use
> >> a common function to reduce the duplicated code.
> >
> >The switch blocks in the above two functions have little difference in the
> default behavior, one is return -EINVAL, the other is just return, for
> register/unregister funcs have different return value types. So, Can I just keep
> this format?
> >
> 
> Duplication is bad and I think it should be easy to deal with the return type
> difference,
> 
> struct ice_prase_list *
> ice_get_parser_list(struct ice_flow_parser *parser,
>                 struct ice_adapter *ad)
> {
>         struct ice_parser_list *list = NULL;
>         struct ice_pf *pf = &ad->pf;
> 
>         switch (parser->stage) {
>         case ICE_FLOW_STAGE_RSS:
>                 list = &pf->rss_parser_list;
>                 break;
>         case ICE_FLOW_STAGE_PERMISSION:
>                 list = &pf->perm_parser_list;
>                 break;
>         case ICE_FLOW_STAGE_DISTRIBUTOR:
>                 list = &pf->dist_parser_list;
>                 break;
>         default:
> 		break;
>         }
> 
> 	return list;
> }
> 
> Then you just need to check its return value, if it's NULL, simply return -EINVAL
> on register and directly return on unregister.

OK, thanks for your guidance. I will fix it in v2.

> 
> Thanks,
> Xiaolong