On Tue, Jan 24, 2023 at 03:14:36PM +0100, Jan Klemkow wrote:
> On Tue, Jan 24, 2023 at 09:32:53PM +1000, David Gwynne wrote:
> > On Mon, Jan 23, 2023 at 09:25:34AM +0100, Jan Klemkow wrote:
> > > On Wed, Jan 18, 2023 at 03:49:25PM -0700, Theo de Raadt wrote:
> > > > Jan Klemkow <j.klem...@wemelug.de> wrote:
> > > > > On Wed, Jan 18, 2023 at 10:50:25AM +0300, Vitaliy Makkoveev wrote:
> > > > > > On Tue, Jan 17, 2023 at 11:09:17PM +0100, Jan Klemkow wrote:
> > > > > > > we have several drivers which have to parse the content of mbufs.
> > > > > > > This
> > > > > > > diff suggest a central parsing function for this. Thus, we can
> > > > > > > reduce
> > > > > > > redundant code.
> > > > > > >
> > > > > > > I just start with ix(4) and ixl(4) because it was easy to test
> > > > > > > for me.
> > > > > > > But, this could also improve em(4), igc(4), ale(4) and oce(4).
> > > > > > >
> > > > > > > I'm not sure about the name, the api nor the place of this code.
> > > > > > > So, if
> > > > > > > someone has a better idea: i'm open to anything.
> > > > > >
> > > > > > I like code this deduplication.
> > > > > >
> > > > > > This newly introduced function doesn't touch ifnet but only extracts
> > > > > > protocol headers from mbuf(9). I guess mbuf_extract_headers() or
> > > > > > something like is much better for name with the ern/uipc_mbuf2.c as
> > > > > > place.
> > > > >
> > > > > Good Point. Updates diff below.
> > > >
> > > > I agree, "extract" is a better name. dlg, do you have a comment?
> > >
> > > Whats you opinion about this diff?
> >
> > it makes ix and ixl prettier, so that's a good enough reason to do
> > it. it should go in net/if_ethersubr.c as ether_extract_headers()
> > though.
> >
> > could you try using a struct to carry the header pointers around and see
> > what that looks like?
> >
> > struct ether_extracted {
> > struct ether_header *eh;
> > struct ip *ip4;
> > struct ip6_hdr *ip6;
> > struct tcphdr *tcp;
> > struct udphdr *udp;
> > };
> >
> > void ether_extract_headers(struct mbuf *, struct ether_extracted *);
> >
> > you can add a depth or flags argument if you want to be able to
> > tell it to return before looking for the tcp/udp headers if you
> > want.
>
> OK?
>
Looks better then m_extract_headers(). Since ext->eh is always assigned
to non NULL value below, the "ext->eh = NULL;" is not necessary. Also
I'm not sure, but is memset() more reliable for `ext' zeroing? Anyway,
feel free to commit without memset().
> + /* Return NULL if header was not recognized. */
> + ext->eh = NULL;
> + ext->ip4 = NULL;
> + ext->ip6 = NULL;
> + ext->tcp = NULL;
> + ext->udp = NULL;
> +
> + ext->eh = mtod(mp, struct ether_header *);
