From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=XoTPTWU0; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id C44435A026F for ; Wed, 01 Jan 2025 22:54:49 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1735768488; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=gOOSwhMdiLWkOhHObBQrlIFbiYwq9R7GMNhzGux/aCE=; b=XoTPTWU0RI/duaIFA/MXMROE80QZJJLPJrAjGMvafJUnuC1rYfcs6L2pSrxmJHiMlJws0Z AMQWCNa8cuvqeexNysQSU2ApZmCeD6KHOHGL9Z6DPCvtnjsbDXLnL7OjCDc4f0hxgFVwgO OQe1bKeHH2smcRbxmvoVSMPOagVVwdc= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-70-JA8rDKLIMOyP80rNn_v7kQ-1; Wed, 01 Jan 2025 16:54:44 -0500 X-MC-Unique: JA8rDKLIMOyP80rNn_v7kQ-1 X-Mimecast-MFC-AGG-ID: JA8rDKLIMOyP80rNn_v7kQ Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-4361fc2b2d6so22455485e9.3 for ; Wed, 01 Jan 2025 13:54:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735768481; x=1736373281; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=gOOSwhMdiLWkOhHObBQrlIFbiYwq9R7GMNhzGux/aCE=; b=s8ep9XZ/28JIe6grUGaJXF9O1UzalAP0IufBaWiMEmowon+R24xTuKZO7pnMj4r0MQ mHYNuU5ACK0YLLrcV7be9mIGc3Pv54hYpNDAZKRIit81bXJKltfOFAIviLA7ohLAYf0o RfRLCYkfX4zma/SVlWwNBsYTs3Fv2aQkz1bGHiwX8WhXPZC3edeXIrGAnT3gV1deBARm m1CSw1dZWAKIfhzA7EZW+s4yhB1rhEZUSls0/9x9zHkqSoKVmpu4UF1tqqxwUUsoL+7v oFKUZ4AWneBi17Mm88osP/XPzQI8cwQLiH/zplvkdSSg31DnG40K/MQsTKJpapfTBVSn QMUA== X-Gm-Message-State: AOJu0Yz8omahMXZypMO4kpi2UT1v9lueUghsHqLxUrGK27QLR9yO5We4 cu9l0hqv+ZwH7Dx2HPUS+0qiCoSPQ9tPaliUrSHY3roDf90rV7uKRd6DsvdYdYI+9NmZpdiPwc2 ZNqn9qa9C099DmVro+Z4FrhwZB5LufUtB05rt2CRVWqjBWZ+hxv34/z/TlA== X-Gm-Gg: ASbGnctlUe06ldPE1gKLOcvwZyRMAP9Jtv/8GZ0KxFIoAi+7/8Fx3Eg4YeFtCXoeX7T LyehI5/VHyUz4I9wlZ6M5mnXkK6cg9+ac8d0gY23nzv3fk6opahVgCoDCa4G3mlo7p4qtsmRWWF U5EM23mg4diF+elo2OC1huD+2KMElejE5o06Lzw+csVaqDG9nJLSt937dluEReZP0t3PvV2chm2 G3zwD7nvS8ClvcqWar675gVjUnxuLqmqHNmdEoTxMf6OWF+4gcjKFm8NYr40Ga7Jr141icmUWRc Qoj7SvPG7g== X-Received: by 2002:a05:600c:3b23:b0:434:a525:7257 with SMTP id 5b1f17b1804b1-43668b49a05mr322841445e9.21.1735768481586; Wed, 01 Jan 2025 13:54:41 -0800 (PST) X-Google-Smtp-Source: AGHT+IEG+3sRKbod8p90KSqpYbg6DujVEk0AqRW+3j+nBEsbw1KZeYSal8bkm9PZp7/XY7ODzpsptw== X-Received: by 2002:a05:600c:3b23:b0:434:a525:7257 with SMTP id 5b1f17b1804b1-43668b49a05mr322841395e9.21.1735768481160; Wed, 01 Jan 2025 13:54:41 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4366128a3c9sm433907845e9.40.2025.01.01.13.54.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Jan 2025 13:54:38 -0800 (PST) Date: Wed, 1 Jan 2025 22:54:37 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 07/12] packet: Remove unhelpful packet_get_try() macro Message-ID: <20250101225437.3fc4f71b@elisabeth> In-Reply-To: <20241220083535.1372523-8-david@gibson.dropbear.id.au> References: <20241220083535.1372523-1-david@gibson.dropbear.id.au> <20241220083535.1372523-8-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: ZJQ9RZtu72TRjlcnRCC6__yNo2D8BTg6XFlxl6iQzps_1735768483 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 4U2FOL3IBLD2RBOOQJO5RSUMMQPUANQT X-Message-ID-Hash: 4U2FOL3IBLD2RBOOQJO5RSUMMQPUANQT X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, 20 Dec 2024 19:35:30 +1100 David Gibson wrote: > 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). The reason why I implemented packet_get_try() is that the messages from packet_get_do() indicate serious issues, and if I'm debugging something by looking at traces it's not great to have messages indicating that we hit a serious issue while we're simply validating identity associations. It's not about the amount of logged messages, it's about the type of message being logged and the distracting noise possibly resulting in a substantial time waste. About 1): dhcpv6_opt() always picks pool index 0, and the base offset was already checked by the caller. In ipv6_l4hdr(), the index was already validated by a previous call to packet_get(), and the starting offset as well. I guess the main reason to have this patch in this series is to make a later change simpler, but I'm not sure which one, so I don't know what to suggest as an alternative. > Signed-off-by: David Gibson > --- > 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); If you change this, the documentation of the arguments for packet_get_do() should be updated as well. > 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; \ -- Stefano