Re: [PATCH 06/12] net/slirp: Clean up error reporting

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Sep 09, 2025 at 01:40:59PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 10:08:17AM +0200, Markus Armbruster wrote:
> >> net_slirp_register_poll_sock() and net_slirp_unregister_poll_sock()
> >> report WSAEventSelect() failure with error_setg(&error_warn, ...).
> >> 
> >> I'm not familiar with liblirp, so I can't say whether the network
> >
> >                        ^^^^^^^^^ 'libslirp'
> 
> Will fix, thanks!
> 
> >> backend will work after such a failure.  If it doesn't, then this
> >> should be an error.  If it does, then why bother the user with a
> >> warning that isn't actionable, and likely confusing?
> >> 
> >> Regardless of that, error_setg_win32(&error_warn, ...) is undesirable
> >> just like error_setg(&error_fatal, ...) and error_setg(&error_abort,
> >> ...)  are.  Replace by warn_report().
> >> 
> >> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  net/slirp.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> >> diff --git a/net/slirp.c b/net/slirp.c
> >> index 9657e86a84..d75b09f16b 100644
> >> --- a/net/slirp.c
> >> +++ b/net/slirp.c
> >> @@ -262,7 +262,8 @@ static void net_slirp_register_poll_sock(slirp_os_socket fd, void *opaque)
> >>      if (WSAEventSelect(fd, event_notifier_get_handle(&ctxt->notifier),
> >>                         FD_READ | FD_ACCEPT | FD_CLOSE |
> >>                         FD_CONNECT | FD_WRITE | FD_OOB) != 0) {
> >> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
> >> +        warn_report("failed to WSAEventSelect(): %s",
> >> +                    g_win32_error_message(WSAGetLastError()));
> >>      }
> >>  #endif
> >
> > IMHO this one ought to be considered fatal. If we can't select the
> > right events on the socket, then we're not going to have a good time
> > trying to poll on events. The libslirp callback API doesn't allow
> > us to return a success/failure code from this function, and IMHO it
> > is not appropriate to use error_fatal here because a fault with slirp
> > should not take down the whole of QEMU. So warn_report is the least
> > worst option I guess. At least it is a hint to the user that all is
> > not well - even if they can't action it, it might alert them if they
> > see network problems in their guest.
> 
> So, we'd make this an error if we could.  But we can't: the function is
> a callback that cannot fail, and outright exit(1) is too harsh.
> 
> That leaves silence or warning.  Warning is less bad.
> 
> Correct?

Yes.

> >> @@ -271,7 +272,8 @@ static void net_slirp_unregister_poll_sock(slirp_os_socket fd, void *opaque)
> >>  {
> >>  #ifdef WIN32
> >>      if (WSAEventSelect(fd, NULL, 0) != 0) {
> >> -        error_setg_win32(&error_warn, WSAGetLastError(), "failed to WSAEventSelect()");
> >> +        warn_report("failed to WSAEventSelect()",
> >> +                    g_win32_error_message(WSAGetLastError()));
> >>      }
> >
> > This one is reasonable to treat as non-fatal, since once we've
> > unregistered the socket for polling
> 
> Whether clearing the event associated with the socket can fail is
> unclear.  Whether it should be treated as an error is also unclear.
> 
> If yes, then same argument as for net_slirp_register_poll_sock() above.

If the callback allowed returning an error, we probably would return
an error, but then I'm confident slirp would do nothing more than
print a warning and continue to close the file handle as normal. It
doesn't make sense to turn file descriptor cleanup into fatal error.

> 
> If no, silence or warning.  Warning is less bad.
> 
> Correct?

Yes.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|