Thanks Jasvinder's comments! Li is on vacation so I help to update the code changes based on your comments, V9 is sent with your ack.
> -----Original Message----- > From: Singh, Jasvinder <[email protected]> > Sent: Monday, April 19, 2021 8:35 PM > To: Li Zhang <[email protected]>; [email protected]; Ori Kam > <[email protected]>; Slava Ovsiienko <[email protected]>; Matan > Azrad <[email protected]>; Shahaf Shuler <[email protected]>; > Dumitrescu, Cristian <[email protected]>; [email protected]; > [email protected]; Yigit, Ferruh <[email protected]>; > [email protected]; Wisam Monther <[email protected]>; Li, > Xiaoyun <[email protected]>; NBU-Contact-Thomas Monjalon > <[email protected]>; Andrew Rybchenko > <[email protected]>; Ray Kinsella <[email protected]>; Neil > Horman <[email protected]> > Cc: [email protected]; Raslan Darawsheh <[email protected]>; Roni Bar Yanai > <[email protected]>; Haifei Luo <[email protected]>; Jiawei(Jonny) Wang > <[email protected]> > Subject: RE: [PATCH v8 1/2] ethdev: add pre-defined meter policy API > > <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 <[email protected]> > >
