Subject: Re: [PATCH 0/4] net_sched: actions - Add default lookup and walker

From: Jamal Hadi Salim <[email-masked]>
Date: Tue, 03 Dec 2013 08:34:12 -0500

On 12/02/13 16:27, David Miller wrote:
>
> Ok, but can you respin this set, adding in changes to remove
> the code that checks for NULL in these operations?
>
> Specifically, we still have:
>
> 	if (a->ops->lookup == NULL)
> 		goto err_mod;
>
> in tcf_action_get_1().  And:
>
> 	if (a_o->walk == NULL) {
>
> in tc_dump_action().
>

Ok, I think i finaly understood what you were saying earlier;->
I was too focused on the simple fix needed for Eric B.
Will make this change ..

> Furthermore, there is a hard assumption that a_o->init() is non-NULL
> as well.  So maybe add a:
>
> 	if (!act->init)
> 		return -EINVAL;
>
> at the top of tcf_register_action().
>

Makes sense.

> It also occurred to me that it would be simpler to audit and be a
> simpler patch set to review if you just added the missing ".lookup ="
> assignments to the actions

Those two methods have proven to be always the defaults and only the
oddball action deviates. So my preference is to have defaults as per
that earlier patch (and overrides for the oddball).

 >and added a similar "if (!act->lookup)" et
> al. check to tcf_register_action().
>
> That looks more like a rigorous requirement to have a valid method
> present for these three operations: init, lookup, walk.
>
> What do you think?

Yep - makes sense (I think you meant act, dump, cleanup and init)
I will add an extra patch..

cheers,
jamal

> Thanks!