On Wed, Jun 21, 2006 at 09:47:49AM +0200, Pavel Cahyna wrote:

Some correction:

> Solution: simplify this common idiom. Let's make a mptr(m, type) macro which

should have been "mptr(m, type, off) and mptr_pullup(m, type, off)"

> would be used like:
> 
> eh = mptr_pullup(m, struct ether_header, 0)
> if (eh == NULL)
> 	recovery from error;

mptr_pullup can need to free the mbuf m and allocate a new one. As it has
to be a macro, it could do it and change the "m" argument. But to provide
a more function-like semantics, it would be better to pass a pointer to
the mbuf pointer, so the "prototype" would look like:

const type * mptr_pullup(struct mbuf **mp, type, int off).

In general, functions which may allocate a new mbuf and free the old one
should IMHO have a similar prototype, instead of accepting a mbuf pointer
and returning a new one like m_pullup does, to reduce risks of using the
old, invalid pointer.

The mptr variand which does not move data does not need it, but I would
like to change it to the same "prototype" for consistency.

Another detail is error handling. Currently m_pullup/pulldown consider a
too short mbuf chain an error and free it, returning NULL, just as they do
when failing to allocate memory. With the proposed macros, it is possible
to report errors in a more fine-grained way. When the packet is too short,
the macros could return NULL without altering the mbuf chain, while on
allocation failure, they could both return NULL and free the mbuf chain,
while setting the mbuf pointer to NULL.

A question arises, what should the mptr variant (which does not do an
equivalent of m_pullup) do when the packet is too short, but contiguous?
Should it panic, so that the responsibility for checking is left to the
caller, or should it rather report the error to the caller?

The implementation I'm working on is flexible enough to allow quickly
changing such details, so I think such decisions should be postponed until
I get more experience with the needs of the consumers of this API.

Pavel Cahyna