On 01.03.2016 11:18, Daniel Borkmann wrote:
On 03/01/2016 12:03 AM, Benjamin Poirier wrote:
[...]
Notes:
Changes v1->v2
As suggested by Hannes, move the code to an inline helper and
express it
using "if" rather than "min".
The code is correct, thanks!
Therefore:
Acked-by: Daniel Borkmann <dan...@iogearbox.net>
However, I actually think that v1 was much better/easier as a fix
though. :/
Meaning 1) it's likely easier to backport, and 2) that we now need a
comment
above each skb->reserved_tailroom assignment probably says that min() might
perhaps have been more self-documenting ...
skb_tailroom_reserve() looks quite generic, but it only makes sense to
use in
combination with skb_availroom(), which would have been good to put a
note to
the header comment. Also "the required headroom should already have been
reserved before using this function", places one more requirement for
usage.
If we really want to go that path, maybe rather a skb_setroom() that is
coupled
with skb_availroom() like:
static inline int __skb_tailroom(const struct sk_buff *skb)
{
return skb->end - skb->tail;
}
static inline void skb_setroom(struct sk_buff *skb,
unsigned int needed_headroom,
unsigned int size,
unsigned int needed_tailroom)
{
SKB_LINEAR_ASSERT(skb);
skb_reserve(skb, needed_headroom);
skb->reserved_tailroom = needed_tailroom;
if (size < __skb_tailroom(skb) - needed_tailroom)
skb->reserved_tailroom = __skb_tailroom(skb) - size;
}
Then, skb_tailroom() would internally use __skb_tailroom(), too. And we
can also
spare us the two unneeded skb_is_nonlinear() checks in our helper which
will
currently always evaluate to false anyway.
... just a thought.
I think it is fine. The code will be inlined anyway and probably skb
linear assertions will be optimized away.
I like the current code in its form:
skb_reserve(skb, header);
skb_tailroom_reserve(skb, mtu, tailroom);
Combined form has too many arguments, so one has to look up the header
file again.
Bye,
Hannes
