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

[Yumei Huang <yuhuang@redhat.com>]

Thank you David for helping explain all this!

On Fri, Sep 12, 2025 at 10:02 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Thu, Sep 11, 2025 at 11:54:25AM +0200, Stefano Brivio wrote:
> > 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
>
> Without this we will get an EBADF in either writev() (pasta) or
> sendmsg() (passt).  That's basically harmless, but a bit ugly.
> Explicitly catching this case results in behaviour that's probably a
> bit clearer to debug if we hit it.
>
> Putting that context in the commit message would be useful.
>
> > - 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").
BTW, I read the commit c9b241346569, it didn't change the behaviour.
Before this, tap_sock_init() will call tap_sock_unix_init() without
checking if c->fd_tap.
> >
> >   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 don't think we can quite do that.  I'm not sure if it's the only
> reason, but for vhost-user I believe we need the epoll loop up and
> running before we have the tap connection fully set up, because we
> need it to process the vhost-user control messages.  Laurent, can you
> verify?
>
> There are several different approaches we can take here.  I discussed
> some with Yumei and suggested she take this one.  Here's some
> reasoning (maybe this would also be useful in the commit message,
> though it's rather bulky)
>
> # Don't listen() until the tap connection is ready
>
>  - It's not clear that the host rejecting the connection is better
>    than the host accepting, then the connection stalling until the
>    guest is ready.
>  - Would require substantial rework because we currently listen() as
>    we parse the command line and don't store the information we'd need
>    to do it later.
>
> # Don't accept() until the tap connection is ready
>
>  - To the peer, will behave basically the same as this patch - the
>    host will complete the TCP handshake, then the connection will stall
>    until the guest is ready.
>  - More work to implement, because essentially every sock-side handler
>    has to check fd_tap and abort early
>
> # Drop packets in tap_send_frames(), but return 0
>
>  - To the peer, would behave basically the same
>  - Would make the TCP code do a bunch of busy work attempting to
>    resend, probably to no avail
>  - Handling of errors returned by tap_send_frames() is on the basis
>    that it's probably a transient fault (buffer full) and we want to
>    resend very soon.  That approach doesn't make sense for a missing
>    guest.
>
> >   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).
Agree, I will drop it in v2.
> >
> > 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.
>
> It's an amusing truth of the passt project that you'll get more
> English usage notes from the Italian living in Germany than the native
> English speaker living in an English speaking country :).
Haha, true! I’m still improving my English,  so the suggestions are
very helpful and much appreciated:)
>
> Fwiw, I agree that the comment can probably just be dropped.  If kept,
> I'd suggest:
>         If no client is connected, silently drop the frames
>
> --
> David Gibson (he or they)       | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
>                                 | around.
> http://www.ozlabs.org/~dgibson


-- 
Thanks,

Yumei Huang