Re: [p4tc-discussions] Re: [PATCH RFC v2 net-next 04/28] net/sched: act_api: add init_ops to struct tc_action_op

[Jamal Hadi Salim <hadi@mojatatu.com>]

On Mon, Jun 5, 2023 at 7:39 AM Dan Carpenter via p4tc-discussions
<p4tc-discussions@netdevconf.info> wrote:
>
> On Mon, Jun 05, 2023 at 11:51:14AM +0200, Simon Horman wrote:
> > > @@ -1494,8 +1494,13 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> > >                     }
> > >             }
> > >
> > > -           err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp,
> > > -                           userflags.value | flags, extack);
> > > +           if (a_o->init)
> > > +                   err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp,
> > > +                                   userflags.value | flags, extack);
> > > +           else if (a_o->init_ops)
> > > +                   err = a_o->init_ops(net, tb[TCA_ACT_OPTIONS], est, &a,
> > > +                                       tp, a_o, userflags.value | flags,
> > > +                                       extack);
> >
> > By my reading the initialisation of a occurs here.
> > Which is now conditional.
> >
>
> Right.  Presumably the author knows that one (and only one) of the
> ->init or ->init_ops pointers is set.

Yes, this is correct and the code above checks i.e
 -     if (!act->act || !act->dump || !act->init)
 +     if (!act->act || !act->dump || (!act->init && !act->init_ops))
               return -EINVAL;

> This kind of relationship between
> two variables is something that Smatch tries to track inside a function
> but outside of functions, like here, then Smatch doesn't track it.
> I can't really think of a scalable way to track this.

Could you have used the statement i referred to above as part of the state?

> So there are a couple options:
>
> 1) Ignore the warning.
> 2) Remove the second if.
>
>         if (a_o->init)
>                 err = a_o->init();
>         else
>                 err = a_o->init_ops();
>
> I kind of like this, because I think it communicates the if ->init()
> isn't set then ->init_ops() must be.

I like this approach - we'll refactor to remove the !police. (note
police using some old tc versions is still a pariah and has typically
to be checked separately, at some point we should audit the code and
remove any police specific checks).

cheers,
jamal

> 3) Add a return.
>
>         if (a_o->init) {
>                 err = a_o->init();
>         } else if (a_o->init_ops) {
>                 err = a_o->init_ops();
>         } else {
>                 WARN_ON(1);
>                 return ERR_PTR(-EINVAL);
>         }
>
> 4) Add an unreachable.  But the last time I suggested this it led to
> link errors and I didn't get a chance to investigate so probably don't
> do this:
>
>         if (a_o->init) {
>                 err = a_o->init();
>         } else if (a_o->init_ops) {
>                 err = a_o->init_ops();
>         } else {
>                 unreachable();
>         }
>
> regards,
> dan carpenter
>
> _______________________________________________
> p4tc-discussions mailing list -- p4tc-discussions@netdevconf.info
> To unsubscribe send an email to p4tc-discussions-leave@netdevconf.info