Re: [PATCH] tap: Drop frames if no client connected

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 11 Sep 2025 16:55:19 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> If no client is attached, discard outgoing frames and report them as
> sent. This mimics the behavior of a physical host with its network
> cable unplugged.
> 
> Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Yumei Huang <yuhuang@redhat.com>

Thanks, the fix itself obviously makes sense, but I have a few questions
and comments:

- first off, what happens if we don't return early in tap_send_frames()?
  Commit messages for fixes (assuming this is a fix) should always say
  what concrete problem we had, what is going to be fixed, or if we're
  not aware of any real issue but things are just fragile / wrong

- until a while ago, this couldn't happen at all. We were just blocking
  the whole execution as long as the tap / guest / container interface
  wasn't up and running.

  I wonder when this changed and if it makes sense to go back to the
  previous behaviour. I had just a quick look and I wonder if I
  accidentally broke this in c9b241346569 ("conf, passt, tap: Open
  socket and PID files before switching UID/GID").

  Before that, main() would call tap_sock_init(), which would call
  tap_sock_unix_open(), a blocking function.

  Should we make the whole thing blocking again? If not, is there
  anything else that's breaking with that? Timers, other inputs, etc.

  I didn't really have time to investigate until now, I can try to
  have another look soon though, unless you find out more meanwhile.

> ---
>  tap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tap.c b/tap.c
> index 7ba6399..e01219d 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -507,13 +507,17 @@ static size_t tap_send_frames_passt(const struct ctx *c,
>   * @iov must have total length @bufs_per_frame * @nframes, with each set of
>   * @bufs_per_frame contiguous buffers representing a single frame.
>   *
> - * Return: number of frames actually sent
> + * Return: number of frames actually sent, or accounted as sent
>   */
>  size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
>  		       size_t bufs_per_frame, size_t nframes)
>  {
>  	size_t m;
>  
> +	if (c->fd_tap == -1)
> +		/* If no client connected, account the frames have been sent */

I think the comment is redundant because, well, if c->fd_tap is -1
(obvious, documented), we return 'nframes' (also documented).

If it's not redundant, for any reason, "to account" in this sense
isn't transitive. You could say: "consider that the frames have been
sent" but not "account that the frames have been sent".

You can pick a different meaning of "to account" and say "account the
frames as sent", though.

> +		return nframes;
> +
>  	if (!nframes)
>  		return 0;
>  

-- 
Stefano