[dpdk-dev] [PATCH v8 1/2] ethdev: add pre-defined meter policy API

[Singh, Jasvinder]

<Snip>

> +/* MTR meter policy add */
> +static int
> +pmd_mtr_meter_policy_add(struct rte_eth_dev *dev,
> +	uint32_t meter_policy_id,
> +	struct rte_mtr_meter_policy_params *policy,
> +	struct rte_mtr_error *error)
> +{
> +	struct pmd_internals *p = dev->data->dev_private;
> +	struct softnic_mtr_meter_policy_list *mpl = &p-
> >mtr.meter_policies;
> +	struct softnic_mtr_meter_policy *mp;
> +	const struct rte_flow_action *act;
> +	const struct rte_flow_action_meter_color *recolor;
> +	uint32_t i;
> +	bool valid_act_found;
> +
> +	if (policy == NULL)
> +		return -rte_mtr_error_set(error,
> +			EINVAL,
> +			RTE_MTR_ERROR_TYPE_METER_POLICY,
> +			NULL,
> +			"Null meter policy invalid");
> +
> +	/* Meter policy must not exist. */
> +	mp = softnic_mtr_meter_policy_find(p, meter_policy_id);
> +	if (mp != NULL)
> +		return -rte_mtr_error_set(error,
> +			EINVAL,
> +			RTE_MTR_ERROR_TYPE_METER_POLICY_ID,
> +			NULL,
> +			"Meter policy already exists");
> +
> +	for (i = 0; i < RTE_COLORS; i++) {
> +		if (policy->actions[i] == NULL)
> +			return -rte_mtr_error_set(error,
> +				EINVAL,
> +				RTE_MTR_ERROR_TYPE_METER_POLICY,
> +				NULL,
> +				"Null action list");
> +		for (act = policy->actions[i], valid_act_found = false;
> +			act && act->type != RTE_FLOW_ACTION_TYPE_END;
> +			act++) {

Hi Li,  
No need to check "act" in for loop instruction as it is validated before the for loop.  Also, would be good to skip all the steps inside this loop for the actions like RTE_FLOW_ACTION_TYPE_VOID. After for loop, will be good to check "valid_act_found" is true else return an error for that color action. 

Rest seems good to me  

With above fix for softnic-
Acked-by: Jasvinder Singh <jasvinder.singh at intel.com>