On 21 Jun 2022, at 5:51, Jianbo Liu wrote:
> On Mon, 2022-06-20 at 12:16 +0200, Eelco Chaudron wrote: >> On 27 May 2022, at 11:00, Jianbo Liu wrote: >> >>> As the police actions with indexes of 0x10000000-0x1fffffff are >>> reserved for meter offload, to provide a clean environment for OVS, >>> these reserved police actions should be deleted on startup. So dump >>> all the police actions, delete those actions with indexes in this >>> range. >>> >>> Signed-off-by: Jianbo Liu <[email protected]> >>> --- <SNIP> >>> +int >>> +parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t >>> police_idx[]) >>> +{ >>> + static struct nl_policy >>> actions_orders_policy[TCA_ACT_MAX_PRIO] = {}; >>> + struct nlattr >>> *actions_orders[ARRAY_SIZE(actions_orders_policy)]; >>> + const int max_size = ARRAY_SIZE(actions_orders_policy); >>> + const struct nlattr *actions; >>> + struct tc_flower flower; >>> + struct tcamsg *tca; >>> + int i, cnt = 0; >>> + int err; >>> + >>> + for (i = 0; i < max_size; i++) { >>> + actions_orders_policy[i].type = NL_A_NESTED; >>> + actions_orders_policy[i].optional = true; >>> + } >>> + >>> + tca = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tca); >>> + actions = nl_attr_find(reply, NLMSG_HDRLEN + sizeof *tca, >>> TCA_ACT_TAB); >>> + if (!actions || !nl_parse_nested(actions, >>> actions_orders_policy, >>> + actions_orders, max_size)) { >>> + VLOG_ERR_RL(&error_rl, "Failed to parse police actions"); >> >> These could be all kinds of actions, not only police ones, i.e. >> whatever is programmed. >> Maybe change this to "Failed to parse flower actions". >> > > But this function is intended for police only, and its name is ended > with "_policer". Do you want to make it generel func for all actions? As you could receive any set of actions from netlink (all that exists in the system), the error might not be specific to a police action, that’s why I think it should be removed. >>> + return EPROTO; >>> + } >>> + >>> + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { >>> + if (actions_orders[i]) { >>> + memset(&flower, 0, sizeof(struct tc_flower)); >>> + err = nl_parse_single_action(actions_orders[i], >>> &flower, false); >>> + if (err) { >>> + continue; >>> + } >>> + if (flower.actions[0].police.index) { >> >> You need to make sure this is the Police action (see the previous >> review), i.e., >> > > I remember your comment in previous version. Sorry I didn't change here > becasue the same reason above. Let me try to be more clear. Whatever you receive from netlink could be any kind of action, i.e. whatever is programmed in the system. So if someone manually added add list of actions, in the ID range, you think it’s Police but it could be something else. What about the following for the two comments above: --- a/lib/tc.c +++ b/lib/tc.c @@ -2127,7 +2127,8 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) actions = nl_attr_find(reply, NLMSG_HDRLEN + sizeof *tca, TCA_ACT_TAB); if (!actions || !nl_parse_nested(actions, actions_orders_policy, actions_orders, max_size)) { - VLOG_ERR_RL(&error_rl, "Failed to parse police actions"); + VLOG_ERR_RL(&error_rl, + "Failed to parse actions in netlink to policer"); return EPROTO; } @@ -2135,7 +2136,7 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) if (actions_orders[i]) { memset(&flower, 0, sizeof(struct tc_flower)); err = nl_parse_single_action(actions_orders[i], &flower, false); - if (err) { + if (err || flower.actions[0].type != TC_ACT_POLICE) { continue; } if (flower.actions[0].police.index) { _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
