On Tue, Oct 16, 2018 at 10:38 AM Davide Caratti <dcara...@redhat.com> wrote: > > On Mon, 2018-10-15 at 11:31 -0700, Cong Wang wrote: > > 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. > ... > > > 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. :) > > whatever fixes the NULL dereference is OK for me. > I thought that the proposal made with > > https://www.mail-archive.com/netdev@vger.kernel.org/msg251933.html > > (i.e., letting init() copy tcfg_paction to tcf_action in case it contained > 'goto chain x') was smart enough to preserve the current behavior, and > also let 'goto chain' work in case it was configured *only* for the > fallback action. > When the action is modified, the change to tcfg_paction is done with the > same spinlock as tcf_action, so I didn't notice anything worse than the > current locking layout. > > (well, after some more thinking I looked again at that patch and yes, it > lacked the most important thing:)
Hmm, as I said, I am not sure if the logic is correct, if we have two different goto actions, we must have two pointers. I will re-think about it tomorrow. (I am at a conference so don't have much time on reviewing this.) Thanks.
