public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>, passt-dev@passt.top
Cc: David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH v2 07/12] packet: Remove unhelpful packet_get_try() macro
Date: Fri, 20 Dec 2024 19:35:30 +1100	[thread overview]
Message-ID: <20241220083535.1372523-8-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20241220083535.1372523-1-david@gibson.dropbear.id.au>

Two places in the code use the packet_get_try() variant on packet_get().
The difference is that packet_get_try() passes a NULL 'func' to
packet_get_do(), which suppresses log messages.  The places we use this
are ones where we expect to sometimes have a failure retreiving the packet
range, even in normal cases.  So, suppressing the log messages seems like
it makes sense, except:

1) It suppresses log messages on all errors.  We (somewhat) expect to hit
   the case where the requested range is not within the received packet.
   However, it also suppresses message in cases where the requested packet
   index doesn't exist, the requested range has a nonsensical length or
   doesn't lie in even the right vague part of memory.  None of those
   latter cases are expected, and the message would be helpful if we ever
   actually hit them.

2) The suppressed messages aren't actually that disruptive.  For the case
   in ip.c, we'd log only if we run out of IPv6 packet before reaching a
   (non-option) L4 header.  That shouldn't be the case in normal operation
   so getting a message (at trave level) is not unreasonable.
   For the case in dhcpv6.c we do suppress a message every time we look for
   but don't find a specific DHCPv6 option.  That can happen in perfectly
   ok cases, but, again these are trace() level and DHCPv6 transactions
   aren't that common.  Suppressing the message for this case doesn't
   seem worth the drawbacks of (1).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 dhcpv6.c |  2 +-
 ip.c     |  2 +-
 packet.c | 18 +++++-------------
 packet.h |  3 ---
 4 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/dhcpv6.c b/dhcpv6.c
index 0523bba8..c0d05131 100644
--- a/dhcpv6.c
+++ b/dhcpv6.c
@@ -271,7 +271,7 @@ static struct opt_hdr *dhcpv6_opt(const struct pool *p, size_t *offset,
 	if (!*offset)
 		*offset = sizeof(struct udphdr) + sizeof(struct msg_hdr);
 
-	while ((o = packet_get_try(p, 0, *offset, sizeof(*o), &left))) {
+	while ((o = packet_get(p, 0, *offset, sizeof(*o), &left))) {
 		unsigned int opt_len = ntohs(o->l) + sizeof(*o);
 
 		if (ntohs(o->l) > left)
diff --git a/ip.c b/ip.c
index 2cc7f654..e391f81b 100644
--- a/ip.c
+++ b/ip.c
@@ -51,7 +51,7 @@ char *ipv6_l4hdr(const struct pool *p, int idx, size_t offset, uint8_t *proto,
 	if (!IPV6_NH_OPT(nh))
 		goto found;
 
-	while ((o = packet_get_try(p, idx, offset, sizeof(*o), dlen))) {
+	while ((o = packet_get(p, idx, offset, sizeof(*o), dlen))) {
 		nh = o->nexthdr;
 		hdrlen = (o->hdrlen + 1) * 8;
 
diff --git a/packet.c b/packet.c
index bcac0375..c921aa15 100644
--- a/packet.c
+++ b/packet.c
@@ -112,27 +112,19 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
 	char *ptr;
 
 	if (idx >= p->size || idx >= p->count) {
-		if (func) {
-			trace("packet %zu from pool size: %zu, count: %zu, "
-			      "%s:%i", idx, p->size, p->count, func, line);
-		}
+		trace("packet %zu from pool size: %zu, count: %zu, %s:%i",
+		      idx, p->size, p->count, func, line);
 		return NULL;
 	}
 
 	if (len > PACKET_MAX_LEN) {
-		if (func) {
-			trace("packet data length %zu, %s:%i",
-			      len, func, line);
-		}
+		trace("packet data length %zu, %s:%i", len, func, line);
 		return NULL;
 	}
 
 	if (len + offset > p->pkt[idx].iov_len) {
-		if (func) {
-			trace("data length %zu, offset %zu from length %zu, "
-			      "%s:%i", len, offset, p->pkt[idx].iov_len,
-			      func, line);
-		}
+		trace("data length %zu, offset %zu from length %zu, %s:%i",
+		      len, offset, p->pkt[idx].iov_len, func, line);
 		return NULL;
 	}
 
diff --git a/packet.h b/packet.h
index 05d37695..f95cda08 100644
--- a/packet.h
+++ b/packet.h
@@ -45,9 +45,6 @@ void pool_flush(struct pool *p);
 #define packet_get(p, idx, offset, len, left)				\
 	packet_get_do(p, idx, offset, len, left, __func__, __LINE__)
 
-#define packet_get_try(p, idx, offset, len, left)			\
-	packet_get_do(p, idx, offset, len, left, NULL, 0)
-
 #define PACKET_POOL_DECL(_name, _size, _buf)				\
 struct _name ## _t {							\
 	char *buf;							\
-- 
@@ -45,9 +45,6 @@ void pool_flush(struct pool *p);
 #define packet_get(p, idx, offset, len, left)				\
 	packet_get_do(p, idx, offset, len, left, __func__, __LINE__)
 
-#define packet_get_try(p, idx, offset, len, left)			\
-	packet_get_do(p, idx, offset, len, left, NULL, 0)
-
 #define PACKET_POOL_DECL(_name, _size, _buf)				\
 struct _name ## _t {							\
 	char *buf;							\
-- 
2.47.1


  parent reply	other threads:[~2024-12-20  8:58 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-20  8:35 [PATCH v2 00/12] Cleanups to packet pool handling and sizing David Gibson
2024-12-20  8:35 ` [PATCH v2 01/12] test focus David Gibson
2024-12-20  8:35 ` [PATCH v2 02/12] hack: stop on fail, but not perf fail David Gibson
2024-12-20  8:35 ` [PATCH v2 03/12] make passt dumpable David Gibson
2024-12-20  8:35 ` [PATCH v2 04/12] packet: Use flexible array member in struct pool David Gibson
2024-12-20  8:35 ` [PATCH v2 05/12] packet: Don't pass start and offset separately too packet_check_range() David Gibson
2024-12-20  8:35 ` [PATCH v2 06/12] packet: Don't hard code maximum packet size to UINT16_MAX David Gibson
2024-12-20  8:35 ` David Gibson [this message]
2024-12-20  8:35 ` [PATCH v2 08/12] util: Add abort_with_msg() and ASSERT_WITH_MSG() helpers David Gibson
2024-12-20  8:35 ` [PATCH v2 09/12] packet: Distinguish severities of different packet_{add,git}_do() errors David Gibson
2024-12-20  8:35 ` [PATCH v2 10/12] packet: Move packet length checks into packet_check_range() David Gibson
2024-12-20  8:35 ` [PATCH v2 11/12] tap: Don't size pool_tap[46] for the maximum number of packets David Gibson
2024-12-20  8:35 ` [PATCH v2 12/12] packet: More cautious checks to avoid pointer arithmetic UB David Gibson
2024-12-20  9:00 ` [PATCH v2 00/12] Cleanups to packet pool handling and sizing David Gibson
2024-12-20 10:06   ` Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241220083535.1372523-8-david@gibson.dropbear.id.au \
    --to=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).