On Tue, Apr 28, 2020 at 02:33:02PM +1000, Duncan Roe wrote: > On Mon, Apr 27, 2020 at 07:06:56PM +0200, Pablo Neira Ayuso wrote: > > Hi Duncan, > > > > On Mon, Apr 27, 2020 at 09:06:14PM +1000, Duncan Roe wrote: > > > Hi Pablo, > > > > > > On Sun, Apr 26, 2020 at 03:23:53PM +0200, Pablo Neira Ayuso wrote: > > > > Hi Duncan, > > > > > > > > This is another turn / incremental update to the pktbuff API based on > > > > your feedback: > > > > > > > > Patch #1 adds pktb_alloc_head() to allocate the pkt_buff structure. > > > > This patch also adds pktb_build_data() to set up the pktbuff > > > > data pointer. > > > > > > > > Patch #2 updates the existing example to use pktb_alloc_head() and > > > > pktb_build_data(). > > > > > > > > Patch #3 adds a few helper functions to set up the pointer to the > > > > network header. > > > > > > > > Your goal is to avoid the memory allocation and the memcpy() in > > > > pktb_alloc(). With this scheme, users pre-allocate the pktbuff object > > > > from the configuration step, and then this object is recycled for each > > > > packet that is received from the kernel. > > > > > > > > Would this update fit for your usecase? > > > > > > No, sorry. The show-stopper is, no allowance for the "extra" arg, > > > when you might want to mangle a packet tobe larger than it was. > > > > I see, maybe pktb_build_data() can be extended to take the "extra" > > arg. Or something like this: > > > > void pktb_build_data(struct pkt_buff *pktb, uint8_t *payload, uint32_t size, uint32_t len) > > > > where size is the total buffer size, and len is the number of bytes > > that are in used in the buffer. > > I really do not like the direction this is taking. pktb_build_data() is one of 4 > new functions you are suggesting, the others being pktb_alloc_head(), > pktb_reset_network_header() and pktb_set_network_header(). In > https://www.spinics.net/lists/netfilter-devel/msg65830.html, you asked > > > I wonder if all these new functions can be consolidated into one > > single function, something like: > > > > struct pkt_buff *pktb_alloc2(int family, void *head, size_t head_size, void *data, size_t len, size_t extra); pktb_alloc2() still has a memcpy which is not needed by people that do not need to mangle the packet. > That's what I have delivered, except for 2 extra args on the end for the packet > copy buffer. And I get rid of pktb_free(), or at least deprecate and move it off > the main doc page into the "Other functions" page. > > Also pktb_set_network_header() makes no allowance for AF_BRIDGE. This is not a problem, you only have to call this function with ETH_HLEN to set the offset in case of bridge. > Can we please just stick with > > > struct pkt_buff *pktb_alloc2(int family, void *head, size_t headsize, > > void *data, size_t len, size_t extra, > > void *buf, size_t bufsize) I'm fine if you still like the simplified pktb_alloc2() call, that's OK. [...] > > I think it's fine if pktb_mangle() deals with this data buffer > > reallocation in case it needs to expand the packet, a extra patch on > > top of this should be fine. > > OK - will start on a patch based on > https://www.spinics.net/lists/netfilter-devel/msg66710.html Revisiting, I would prefer to keep things simple. The caller should make sure that pktb_mangle() has a buffer containing enough room. I think it's more simple for the caller to allocate a buffer that is large enough for any mangling.
