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); 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. 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) maybe with a better name for buf - data_copy_buf? > > > For "extra" support, > > you need something with the sophistication of pktb_malloc2. > > If extra == 0, > > pktb_malloc2 optimises by leaving the packet data where it was. > > With this patchset, the user is in control of the data buffer memory > area that is attached to the pkt_buff head, so you can just allocate > the as many extra byte as you require. > > > Actually pktb_malloc2 doesn't need to make this decision. > > That can be deferred to pktb_mangle, > > which could do the copy if it has been told to expand a packet > > and the copy has not already been done (new "copy done" flag in the opaque > > struct pkt_buff). > > 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 > > > My nfq-based accidentally-written ad blocker would benefit from that > > deferment - I allow extra bytes in case I have to lengthen a domain name, > > but most of the time I'm shortening them. > > Thanks for explaining. Cheers ... Duncan.
