On 04/04/16(Mon) 22:56, David Gwynne wrote:
> On Mon, Apr 04, 2016 at 08:07:47PM +1000, David Gwynne wrote:
> > > On 4 Apr 2016, at 6:41 PM, Martin Pieuchot <[email protected]> wrote:
> > > On 04/04/16(Mon) 13:09, David Gwynne wrote:
> > >> #ifdef DDB
> > >> @@ -593,6 +594,12 @@ if_enqueue(struct ifnet *ifp, struct mbu
> > >> struct mbuf_queue if_input_queue = MBUF_QUEUE_INITIALIZER(8192, IPL_NET);
> > >> struct task if_input_task = TASK_INITIALIZER(if_input_process,
> > >> &if_input_queue);
> > >>
> > >> +int
> > >> +if_input_filter(void *if_bpf, const struct mbuf *m)
> > >> +{
> > >> + return bpf_mtap_ether(if_bpf, m, BPF_DIRECTION_IN);
> > >> +}
> > >> +
> > >
> > > I find this filtering function confusing. What about introducing a
> > > bpf_mtap() variant dealing with an ``mbuf_list'' : bpf_mltap()?
> >
> > i could, but passing the direction through is difficult. i could have
> > different wrappers for different directions though.
struct mbuf_list bpf_mltap(cadd_t arg, struct mbuf_list *ml, u_int dir);
How is that difficult?
> > > What about returning a mbuf (or a mbuf_list) instead of a boolean and
> > > check for NULL? This would fit in the input_handler model where a
> > > packet is "consumed", here by bpf(4), and processing should stop.
> > >
> > > You could then write:
> > >
> > > m = bpf_mtap(if->if_bpf, m, BPF_DIRECTION_IN);
> > > if (m == NULL)
> > > return;
> >
> > i wouldnt be able to pass the mbuf as a const struct mbuf * and return it
> > without casting it to drop the const qualifier. to me it feels more like
> > its being tested than consumed.
It is tested because you define it as const, or is it the other way
around?
> also consider that if bpf_mtap freed the mbuf, every caller would
> have to be fixed to check for that. at the moment we only respect
> drops on incoming packets.
So put a KASSERT if the direction is outgoing so you don't have to fix
all the callers.
> void
> if_input(struct ifnet *ifp, struct mbuf_list *ml)
> {
> @@ -614,8 +621,15 @@ if_input(struct ifnet *ifp, struct mbuf_
> #if NBPFILTER > 0
> if_bpf = ifp->if_bpf;
> if (if_bpf) {
> - MBUF_LIST_FOREACH(ml, m)
> - bpf_mtap_ether(if_bpf, m, BPF_DIRECTION_IN);
> + struct mbuf *m0;
> +
> + m0 = ml_filter(ml, if_input_filter, if_bpf);
> + while (m0 != NULL) {
> + m = m0;
> + m0 = m->m_nextpkt;
> +
> + m_freem(m);
> + }
Do you really want to add this mbuf horror show here? It looks like
you've spent too much time in kern/uipc_mbuf.c recently!
