public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
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


  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).