On Mon, 2022-06-27 at 11:17 +0200, Eelco Chaudron wrote:
>
>
> 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:
I don't know if someone will use these functions. But currently they
are only for police, as I dump police actions by RTM_GETACTION request.
Anyway, I will update as you suggested.
>
> --- 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
