From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine 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=gNgT1n/U; 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 B07FF5A0280 for ; Mon, 26 May 2025 16:19:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1748269164; 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=vgHe0Crm1D5I4YsVHTHRZC0/RvHG3x5AisnX69ezftU=; b=gNgT1n/UQ9K9UnCBPlzmBT1RA+Bi3aldHrjxh0wFK7aQRBQPEwy9VPXqqlppNDvgXWNuAl JiAKrP6p7ZBd6VSe+6JjSHAe3nV0F5k1jrrKtFMzwgWD+stcUhrUL2A93oyiy1/qqAC40U T7DD5zAIq5k/4hXEoznusx9aTX7EDQ0= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-264-OZfpuMTAM0C8LEYzuZtKvA-1; Mon, 26 May 2025 10:19:22 -0400 X-MC-Unique: OZfpuMTAM0C8LEYzuZtKvA-1 X-Mimecast-MFC-AGG-ID: OZfpuMTAM0C8LEYzuZtKvA_1748269161 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-3a4cceb558aso915190f8f.3 for ; Mon, 26 May 2025 07:19:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1748269160; x=1748873960; 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=vgHe0Crm1D5I4YsVHTHRZC0/RvHG3x5AisnX69ezftU=; b=BpNyDkqwm+JAnWvUZdZlzslZW2aIuKvN2bJW08bIo9ITrZGAq2sQex1fcVGkFF+8XW 11H6pw33OxS+QJvBVPOk0IZVvvMilCt0xCNNkp+MlWQaqipabbkH+bcSNg2SgszQCbEf S0GrBlK3/t/EGJP2NWbrcbZdY7ABMqoLT0J9p9EBwoueiJa6JLwP75RLY7yKgnh+QNdA PxTmDJqmqNTUV5Oa8LL2hsTiUAlQ3vt4Ov2xcztY0OUT3gJxgg6iBLace5LBiXGPkLL5 Lhqn0KYGQmMuVofuKKw1+jSE2pUVDKpNVOoZ6bQvEkn6XNlDDsGcUpYdWBubjO8RElsU V5sg== X-Gm-Message-State: AOJu0Yy2aIhrw7AO6DLFZt3noxOCElHl6W/HOGuXVNU4lZ3bzqHiZlG6 5CeG+0ZhVVcb7GFE4p0jtqb49I13jI2fHw8GOpUoHJO5w3QGEfDR3SzDT9OImS06yo1H0YB2ybW NKFWLsA96lWZB/vwRt9u4qpnZniu6SMaAHweQxp/DpcyJXxuo9A440IEhgtpWBbrcbrHo01ayUf wOOE9j9EfBysZlvK233PNgAiUGEF9u3Uw222v9 X-Gm-Gg: ASbGncuSKssfL4HhoCBb2oWo6zweSHTWQ5w0DlYNfOgl1B66CP3yyWt76LEPVVCNsrm SCbDwZ2vjagrrYc4T8Dwe0Oyf28USwTE4ygO7NJJoto+zASZuT4SZxMU31ozipOGz915G5tzuzz nrn/JmEvqP/1StxlOCAyslhrDWsUounkKn2qG2CRvQcfodMCUmUzZ3t++l/MVPUINfkJ0oEqScU ILqYj9Q6V5fqA+y603ssY+DYRsjmY7lp3lnN+zqYKHBUVu20yDU5a1yQJtfN9dd9QCUaDtpm3F7 MFw0bURaoVVGmjcKnZGG8NY= X-Received: by 2002:a5d:5f8b:0:b0:3a3:4bb4:86bf with SMTP id ffacd0b85a97d-3a4cb432e29mr7021785f8f.9.1748269160273; Mon, 26 May 2025 07:19:20 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH83IEY98mEORZgltX8PgkkemQNCcerFMqUTHoMYr8RyFYUbTAdT+00czjVD54oOAFzH8C10Q== X-Received: by 2002:a5d:5f8b:0:b0:3a3:4bb4:86bf with SMTP id ffacd0b85a97d-3a4cb432e29mr7021753f8f.9.1748269159774; Mon, 26 May 2025 07:19:19 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3a35ca5a79asm34823873f8f.25.2025.05.26.07.19.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 26 May 2025 07:19:19 -0700 (PDT) Date: Mon, 26 May 2025 16:19:18 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v5 03/29] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() Message-ID: <20250526161918.6fface54@elisabeth> In-Reply-To: <20250417165136.2688884-4-lvivier@redhat.com> References: <20250417165136.2688884-1-lvivier@redhat.com> <20250417165136.2688884-4-lvivier@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: NXfrRt7pCvE9K0vvZi0OnSWfVWz83m_ptBFcTMVW_lo_1748269161 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: QKYI3Y2XXW6GS74DSXEGN72QYNQD6XVR X-Message-ID-Hash: QKYI3Y2XXW6GS74DSXEGN72QYNQD6XVR 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 Thu, 17 Apr 2025 18:51:10 +0200 Laurent Vivier wrote: > Provide a temporary variable of the wanted type to store > the header if the memory in the iovec array is not contiguous. > > Signed-off-by: Laurent Vivier > --- > iov.c | 53 ++++++++++++++++++++++++++++++++++++++++++++--------- > iov.h | 52 ++++++++++++++++++++++++++++++++++++++-------------- > tcp_buf.c | 2 +- > 3 files changed, 83 insertions(+), 24 deletions(-) > > diff --git a/iov.c b/iov.c > index 047fcbce7fcd..907cd5339369 100644 > --- a/iov.c > +++ b/iov.c > @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, > * > * Returns: The number of bytes successfully copied. > */ > -/* cppcheck-suppress unusedFunction */ > +/* cppcheck-suppress [staticFunction] */ > size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, > size_t offset, void *buf, size_t bytes) > { > @@ -126,6 +126,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, > /* copying data */ > for (copied = 0; copied < bytes && i < iov_cnt; i++) { > size_t len = MIN(iov[i].iov_len - offset, bytes - copied); > + /* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */ > memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset, > len); > copied += len; > @@ -260,7 +261,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len) > } > > /** > - * iov_peek_header_() - Get pointer to a header from an IOV tail > + * iov_check_header() - Check if a header can be accessed > * @tail: IOV tail to get header from > * @len: Length of header to get, in bytes > * @align: Required alignment of header, in bytes > @@ -271,8 +272,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len) > * overruns the IO vector, is not contiguous or doesn't have the > * requested alignment. > */ > -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ > -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) > +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t align) > { > char *p; > > @@ -292,27 +292,62 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align) > return p; > } > > +/** > + * iov_peek_header_() - Get pointer to a header from an IOV tail > + * @tail: IOV tail to get header from > + * @v: Temporary memory to use if the memory in @tail > + * is discontinuous > + * @len: Length of header to get, in bytes > + * @align: Required alignment of header, in bytes > + * > + * @tail may be pruned, but will represent the same bytes as before. > + * > + * Returns: Pointer to the first @len logical bytes of the tail, or to > + * a copy if that overruns the IO vector, is not contiguous or > + * doesn't have the requested alignment. NULL if that overruns the > + * IO vector. > + */ > +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */ > +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align) > +{ > + char *p = iov_check_header(tail, len, align); > + size_t l; > + > + if (p) > + return p; > + > + l = iov_to_buf(tail->iov, tail->cnt, tail->off, v, len); This effectively bypasses three checks performed by iov_check_header(), that is, if there's nothing left in the iov_tail, if 'len' exceeds it, or if it's not aligned, we'll proceed calling iov_to_buf(), whereas we should only call it if the buffer is not contiguous, I think. Perhaps it would make more sense to fail on iov_check_header() failure, and take the contiguity check out of it, so that we preserve the early return on those failures. Another alternative might be to call iov_to_buf from the old version of iov_peek_header_(), but I guess things would be easier to follow if iov_check_header() really indicates failure, instead. Nit: ... tail->cnt, tail->off ... > + if (l != len) > + return NULL; > + > + return v; > +} > + > /** > * iov_remove_header_() - Remove a header from an IOV tail > * @tail: IOV tail to remove header from (modified) > + * @v: Temporary memory to use if the memory in @tail > + * is discontinuous > * @len: Length of header to remove, in bytes > * @align: Required alignment of header, in bytes > * > * On success, @tail is updated so that it longer includes the bytes of the > * returned header. > * > - * Returns: Pointer to the first @len logical bytes of the tail, NULL if that > - * overruns the IO vector, is not contiguous or doesn't have the > - * requested alignment. > + * Returns: Pointer to the first @len logical bytes of the tail, or to > + * a copy if that overruns the IO vector, is not contiguous or > + * doesn't have the requested alignment. NULL if that overruns the > + * IO vector. > */ > -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align) > +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align) > { > - char *p = iov_peek_header_(tail, len, align); > + char *p = iov_peek_header_(tail, v, len, align); > > if (!p) > return NULL; > > tail->off = tail->off + len; > + > return p; > } > > diff --git a/iov.h b/iov.h > index 809f84a2c0a0..a6e0f41e8033 100644 > --- a/iov.h > +++ b/iov.h > @@ -73,41 +73,65 @@ struct iov_tail { > #define IOV_TAIL(iov_, cnt_, off_) \ > (struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) } > > +/** > + * IOV_TAIL_FROM_BUF() - Create a new IOV tail from a buffer > + * @buf_: Buffer address to use in the iovec > + * @len_: Buffer size > + * @off_: Byte offset in the buffer where the tail begins > + */ > +#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \ > + IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), .iov_len = (len_) }), 1, (off_)) Something like: #define IOV_TAIL_FROM_BUF(buf_, len_, off_) \ IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), \ .iov_len = (len_) }), \ 1, \ (off_)) doesn't just have the advantage of not exceeding 80 columns, but I think it's also more readable. > + > bool iov_tail_prune(struct iov_tail *tail); > size_t iov_tail_size(struct iov_tail *tail); > bool iov_tail_drop(struct iov_tail *tail, size_t len); > -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align); > -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align); > +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align); > +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align); > int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt, > struct iov_tail *tail, size_t *bytes); > > /** > * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail > * @tail_: IOV tail to get header from > - * @type_: Data type of the header > + * @var_: Temporary buffer of the type of the header to use if > + * the memory in the iovec array is not contiguous. > * > * @tail_ may be pruned, but will represent the same bytes as before. > * > - * Returns: Pointer of type (@type_ *) located at the start of @tail_, NULL if > - * we can't get a contiguous and aligned pointer. > + * Returns: Pointer of type (@type_ *) located at the start of @tail_ > + * or to @var_ if iovec memory is not contiguous, NULL if > + * that overruns the iovec. > */ > -#define IOV_PEEK_HEADER(tail_, type_) \ > - ((type_ *)(iov_peek_header_((tail_), \ > - sizeof(type_), __alignof__(type_)))) > + > +#define IOV_PEEK_HEADER(tail_, var_) \ > + ((__typeof__(var_) *)(iov_peek_header_((tail_), &(var_), \ > + sizeof(var_), __alignof__(var_)))) It would be nice to preserve the usual visual alignment of arguments to iov_peek_header_(). > > /** > * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail > * @tail_: IOV tail to remove header from (modified) > - * @type_: Data type of the header to remove > + * @var_: Temporary buffer of the type of the header to use if > + * the memory in the iovec array is not contiguous. > * > * On success, @tail_ is updated so that it longer includes the bytes of the > * returned header. > * > - * Returns: Pointer of type (@type_ *) located at the old start of @tail_, NULL > - * if we can't get a contiguous and aligned pointer. > + * Returns: Pointer of type (@type_ *) located at the start of @tail_ > + * or to @var_ if iovec memory is not contiguous, NULL if > + * that overruns the iovec. > + */ > + > +#define IOV_REMOVE_HEADER(tail_, var_) \ > + ((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_), \ > + sizeof(var_), __alignof__(var_)))) > + > +/** IOV_DROP_HEADER() -- Remove a typed header from an IOV tail Nit: [...] - Remove ... > + * @tail_: IOV tail to remove header from (modified) > + * @type_: Data type of the header to remove > + * > + * Returns: true if the tail still contains any bytes, otherwise false > */ > -#define IOV_REMOVE_HEADER(tail_, type_) \ > - ((type_ *)(iov_remove_header_((tail_), \ > - sizeof(type_), __alignof__(type_)))) > +#define IOV_DROP_HEADER(tail_, type_) \ > + iov_tail_drop((tail_), sizeof(type_)) No need for a newline now: #define IOV_DROP_HEADER(tail_, type_) iov_tail_drop((tail_), sizeof(type_)) > #endif /* IOVEC_H */ > diff --git a/tcp_buf.c b/tcp_buf.c > index 05305636b503..4bcc1acb245a 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -160,7 +160,7 @@ static void tcp_l2_buf_fill_headers(const struct tcp_tap_conn *conn, > uint32_t seq, bool no_tcp_csum) > { > struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); > - struct tcphdr *th = IOV_REMOVE_HEADER(&tail, struct tcphdr); > + struct tcphdr thc, *th = IOV_REMOVE_HEADER(&tail, thc); > struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; > const struct flowside *tapside = TAPFLOW(conn); > const struct in_addr *a4 = inany_v4(&tapside->oaddr); -- Stefano