Re: [PATCH 1/5] tap: Better report errors receiving from QEMU socket

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 26 Jul 2024 17:20:27 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> If we get an error on recv() from the QEMU socket, we currently don't
> print any kind of error.  Although this can happen in a non-fatal situation
> such as a guest restarting, it's unusual enough that we realy should report
> something for debugability.
> 
> Add an error message in this case.  Also always report when the qemu
> connection closes for any reason, not just when it will cause us to exit
> (--one-off).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tap.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 44bd4445..a2ae7c2a 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -969,10 +969,10 @@ void tap_add_packet(struct ctx *c, ssize_t l2len, char *p)
>   */
>  static void tap_sock_reset(struct ctx *c)
>  {
> -	if (c->one_off) {
> -		info("Client closed connection, exiting");
> +	info("Client connection closed%s", c->one_off ? ", exiting" : "");
> +
> +	if (c->one_off)
>  		exit(EXIT_SUCCESS);
> -	}
>  
>  	/* Close the connected socket, wait for a new connection */
>  	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
> @@ -1005,8 +1005,10 @@ redo:
>  
>  	n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT);
>  	if (n < 0) {
> -		if (errno != EINTR && errno != EAGAIN && errno != EWOULDBLOCK)
> +		if (errno != EINTR && errno != EAGAIN && errno != EWOULDBLOCK) {
> +			err_perror("Error receiving from QEMU socket");

Nit I'm fixing up on merge: it's not necessarily QEMU, because libkrun
and perhaps others also use this path (in fact, the whole problem was
reported as part of the libkrun integration).

Maybe it's obvious to users anyway, but this might seriously confuse
somebody who's using e.g. krun on Asahi Linux (is QEMU running, one
might wonder):
  https://github.com/slp/krun#motivation
  https://github.com/slp/krun/blob/main/crates/krun/src/net.rs

On top of that, now that we have an error message, I guess it would be
nice to state we're resetting the connection, because it's not really
obvious: the subsequent message from tap_sock_reset() makes it look like
the client decided to close the connection on its own.

So I'm changing this to:

			err("Error receiving from guest, resetting connection");

...if you see an issue with it, I'll send another patch.

-- 
Stefano