On Wed, Jun 12, 2024 at 08:34:21AM +0200, Stefano Brivio wrote:On Wed, 12 Jun 2024 16:18:59 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Good point.On Wed, Jun 12, 2024 at 12:09:50AM +0200, Stefano Brivio wrote:It's a single packet it adds, so perhaps tap_add_packet()?On Wed, 5 Jun 2024 17:21:24 +0200 Laurent Vivier <lvivier(a)redhat.com> wrote:I concur, I think tap_handler() is a better name.Consolidate pool_tap4() and pool_tap6() into pool_flush_all(), and tap4_handler() and tap6_handler() into tap_handler_all(). Create a generic packet_add_all() to consolidate packet addition logic and reduce code duplication. The purpose is to ease the export of these functions to use them with the vhost-user backend. Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- tap.c | 113 +++++++++++++++++++++++++++++++++------------------------- tap.h | 7 ++++ 2 files changed, 71 insertions(+), 49 deletions(-) diff --git a/tap.c b/tap.c index 2ea08491a51f..5fb3cb83f3d2 100644 --- a/tap.c +++ b/tap.c @@ -920,6 +920,61 @@ append: return in->count; } +/** + * pool_flush() - Flush both IPv4 and IPv6 packet pools + */ +void pool_flush_all(void) +{ + pool_flush(pool_tap4); + pool_flush(pool_tap6); +} + +/** + * tap_handler_all() - IPv4/IPv4 and ARP packet handler for tap file descriptorIPv4/IPv6+ * @c: Execution context + * @now: Current timestamp + */ +void tap_handler_all(struct ctx *c, const struct timespec *now)I wonder if this shouldn't be named tap_handler() instead. As we already have tap_handler_passt() and tap_handler_pasta(), it's not immediately clear what "all" refers to.So, given this, I think it does make more sense for this to be in tap.c than packet.c. How about calling it tap_add_packets().+{ + tap4_handler(c, pool_tap4, now); + tap6_handler(c, pool_tap6, now); +} + +/** + * packet_add_all_do() - Add a packet to the appropriate TAP poolA couple of remarks here: - it's a bit unexpected that this is still in tap.c (it adds packets to a pool, it should be in packet.c judging by this name/description). If we call it tap_queue_packet(), then it probably makes more sense? - this does more than adding a packet to a pool. It's probably useless to describe in detail what this does, as the function body is anyway rather short and clear, but the current description could be a bit misleading. What about "Queue/capture packet, update notion of guest MAC address"?Yeah, fair enough. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibsonOkay, but even then, it would be obvious from previous tracing output who the caller is. What's relevant here is to log that something went wrong while adding a packet to an IPv4 or IPv6 pool. I don't think we should bother passing around function name and line to log anything else.- what happens if you just call packet_add() from here, without dealing with 'func' and 'line'? I think it's fine to print in tracing output name and lines from this function, instead of the ones from the caller. It's obvious who the caller isIt is as of this patch, but I believe the idea is this will also be called from VU code down the line.