Re: question about mbuf intialization

[Iain Hibbert <plunky%ogmig.net@localhost>]

On Thu, 26 Sep 2013, Beverly Schwartz wrote:

>
> On Sep 26, 2013, at 10:41 AM, christos%astron.com@localhost (Christos Zoulas) 
> wrote:
>
> > In article <20024705-1886-4E74-A427-2F20D1479CD1%bbn.com@localhost>,
> > Beverly Schwartz  <bschwart%bbn.com@localhost> wrote:
> >
> >> Finally, using rump as a way to test kernel code in user space is really
> >> powerful.  I did a lot of exercising of mbuf code, and initially, I did
> >> a lot of crashing and burning because I didn't realize that m_len wasn't
> >> initialized.  This became a pain to deal with - everytime I got an mbuf
> >> I had to remember to initialize m_len and m_pkthdr.len.  So I just went
> >> and changed the source code, and this discussion has resulted.
> >
> > I've been looking at the code and it seems to me that not only m_get()
> > needs to be changed, but also m_getcl() and MCLGET().
>
> m_getcl (which calls MCLGET) is called with an mbuf in hand.  These
> just add the external buffer and does all apropriate initializations.
> They do not add data, so m_len and m_pkthdr.len do not need to be
> updated.
>
> > In essence, none
> > of the allocators currently set m_len or m_pkthdr.len, and they leave
> > it to the packet formation routines to do so.
>
> Correct, and the two functions that need to change are m_get and m_gethdr.

...except of course, that they do not add any data either.

> > I am just mentioning that if we do that we should fix it consistently
> > (if anyone can call the mbuf API consistent...)

I think it is probably inconsistent, and full of grumble.

> I think I got it right, within uipc_mbuf.c itself.  I did not
> go through the rest of the code base to remove every other
> initialization to zero.

I personally feel that changing the underlying behaviour of a legacy API
such as this without auditing the code that uses it is the wrong thing to
do. How can this little change possibly be a problem you ask?  Well I
don't know, but historically there has been abuse of mbufs (using it for
non-packet data for example) and I am not proposing to change it.

Probably the correct thing to do is to modify the documentation to make it
clear that the length field is external (note that FreeBSD and OpenBSD
have both done this), which also leaves us compatible with other mbuf
implementations.

regards,
iain