From: Jamal Hadi Salim <jhs@mojatatu•com>
To: David Miller <davem@davemloft•net>
Cc: netdev@vger•kernel.org, eric.dumazet@gmail•com,
alexander.h.duyck@intel•com, ebiederm@xmission•com
Subject: Re: [PATCH 0/4] net_sched: actions - Add default lookup and walker
Date: Tue, 03 Dec 2013 08:34:12 -0500 [thread overview]
Message-ID: <529DDDD4.6070706@mojatatu.com> (raw)
In-Reply-To: <20131202.162752.2143617473594386587.davem@davemloft.net>
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!
>
prev parent reply other threads:[~2013-12-03 13:34 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-01 22:30 [PATCH 0/4] net_sched: actions - Add default lookup and walker Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 1/4] Provide default lookup function for actions that dont provide one Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 2/4] Use default lookup functions. Users are free to override when needed Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 3/4] Provide default walker function for actions that dont provide one Jamal Hadi Salim
2013-12-01 22:30 ` [PATCH 4/4] Use default walker functions. Users who need something special can override Jamal Hadi Salim
2013-12-02 21:27 ` [PATCH 0/4] net_sched: actions - Add default lookup and walker David Miller
2013-12-03 13:34 ` Jamal Hadi Salim [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=529DDDD4.6070706@mojatatu.com \
--to=jhs@mojatatu$(echo .)com \
--cc=alexander.h.duyck@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=ebiederm@xmission$(echo .)com \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox