On Sat, Oct 13, 2018 at 8:23 AM Davide Caratti <dcara...@redhat.com> wrote: > > On Fri, 2018-10-12 at 13:57 -0700, Cong Wang wrote: > > Why not just validate the fallback action in each action init()? > > For example, checking tcfg_paction in tcf_gact_init(). > > > > I don't see the need of making it generic. > > hello Cong, once again thanks for looking at this. > > what you say is doable, and I evaluated doing it before proposing this > patch. > > But I felt unconfortable, because I needed to pass struct tcf_proto *tp in > tcf_gact_init() to initialize a->goto_chain with the chain_idx encoded in > the fallback action. So, I would have changed all the init() functions in > all TC actions, just to fix two of them. > > A (legal?) trick is to let tcf_action store the fallback action when it > contains a 'goto chain' command, I just posted a proposal for gact. If you > think it's ok, I will test and post the same for act_police.
Do we really need to support TC_ACT_GOTO_CHAIN for gact->tcfg_paction etc.? I mean, is it useful in practice or is it just for completeness? IF we don't need to support it, we can just make it invalid without needing to initialize it in ->init() at all. If we do, however, we really need to move it into each ->init(), because we have to lock each action if we are modifying an existing one. With your patch, tcf_action_goto_chain_init() is still called without the per-action lock. What's more, if we support two different actions in gact, that is, tcfg_paction and tcf_action, how could you still only have one a->goto_chain pointer? There should be two pointers for each of them. :) Thanks.
