From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top, Laurent Vivier <lvivier@redhat.com>
Subject: Re: [PATCH 5/6] pcap: Allow pcap_frame() and pcap_multiple() to take multi-buffer frames
Date: Tue, 27 Feb 2024 15:26:26 +0100 [thread overview]
Message-ID: <20240227152626.3e1f560d@elisabeth> (raw)
In-Reply-To: <20240222055602.1872516-6-david@gibson.dropbear.id.au>
On Thu, 22 Feb 2024 16:56:01 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> pcap_frame() explicitly takes a single frame, and only allows a single
> buffer (iovec) to be passed. pcap_multiple() takes multiple buffers, but
> explicitly expects exactly one frame per buffer.
>
> Future changes are going to want to split single frames across multiple
> buffers in some circumstances, so extend the pcap functions to allow for
> that.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> pcap.c | 24 ++++++++++++++----------
> pcap.h | 3 ++-
> tap.c | 2 +-
> 3 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/pcap.c b/pcap.c
> index eeb71a3c..9eb4f3d2 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -31,6 +31,7 @@
> #include "util.h"
> #include "passt.h"
> #include "log.h"
> +#include "pcap.h"
>
> #define PCAP_VERSION_MINOR 4
>
> @@ -67,14 +68,15 @@ struct pcap_pkthdr {
>
> /**
> * pcap_frame() - Capture a single frame to pcap file with given timestamp
> - * @iov: iovec referencing buffer containing frame (with L2 headers)
> - * @offset: Offset of the frame from @iov->iov_base
> + * @iov: IO vector containing frame (with L2 headers and tap headers)
> + * @iovcnt: Number of buffers (@iov entries) in frame
> + * @offset: Byte offset of the L2 headers within @iov
I swear I didn't read "Byte offset" from here before commenting on 1/6,
which is rather promising.
> * @tv: Timestamp
> *
> * Returns: 0 on success, -errno on error writing to the file
> */
> -static void pcap_frame(const struct iovec *iov, size_t offset,
> - const struct timeval *tv)
> +static void pcap_frame(const struct iovec *iov, size_t iovcnt,
> + size_t offset, const struct timeval *tv)
> {
> size_t len = iov->iov_len - offset;
> struct pcap_pkthdr h = {
> @@ -86,7 +88,7 @@ static void pcap_frame(const struct iovec *iov, size_t offset,
> struct iovec hiov = { &h, sizeof(h) };
>
> if (write_remainder(pcap_fd, &hiov, 1, 0) < 0 ||
> - write_remainder(pcap_fd, iov, 1, offset) < 0)
> + write_remainder(pcap_fd, iov, iovcnt, offset) < 0)
> debug("Cannot log packet, length %zu: %s",
> len, strerror(errno));
> }
> @@ -105,16 +107,18 @@ void pcap(const char *pkt, size_t len)
> return;
>
> gettimeofday(&tv, NULL);
> - pcap_frame(&iov, 0, &tv);
> + pcap_frame(&iov, 1, 0, &tv);
> }
>
> /**
> * pcap_multiple() - Capture multiple frames
> - * @iov: Array of iovecs, one entry per frame
> + * @iov: Array of iovecs, every @framebufs entries is one frame
> + * @framebufs: Number of buffers per frame
I found this a bit hard to understand. What about:
* @iov: Array of IO vectors, with item count @frame_parts * @n
* @frame_parts: Number of IO vector items for each frame
?
The rest of the series looks good to me.
--
Stefano
next prev parent reply other threads:[~2024-02-27 14:27 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 5:55 [PATCH 0/6] Allow more use of iovecs in pcap and tap interfaces David Gibson
2024-02-22 5:55 ` [PATCH 1/6] util: Add helper to find offset into io vector David Gibson
2024-02-27 14:23 ` Stefano Brivio
2024-02-27 23:27 ` David Gibson
2024-02-22 5:55 ` [PATCH 2/6] pcap: Update pcap_frame() to take an iovec and offset David Gibson
2024-02-27 14:23 ` Stefano Brivio
2024-02-22 5:55 ` [PATCH 3/6] util: Add write_remainder() helper David Gibson
2024-02-27 14:25 ` Stefano Brivio
2024-02-28 0:44 ` David Gibson
2024-02-28 6:24 ` Stefano Brivio
2024-02-28 9:04 ` David Gibson
2024-02-28 9:22 ` Stefano Brivio
2024-02-22 5:56 ` [PATCH 4/6] pcap: Handle short writes in pcap_frame() David Gibson
2024-02-22 5:56 ` [PATCH 5/6] pcap: Allow pcap_frame() and pcap_multiple() to take multi-buffer frames David Gibson
2024-02-27 14:26 ` Stefano Brivio [this message]
2024-02-22 5:56 ` [PATCH 6/6] tap: Use write_remainder() in tap_send_frames_passt() David Gibson
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=20240227152626.3e1f560d@elisabeth \
--to=sbrivio@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=lvivier@redhat.com \
--cc=passt-dev@passt.top \
/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).