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
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(-)
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
}
@@ -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()));
}
#endif
}
--
2.49.0
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'
> 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.
> }
> @@ -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
> #endif
> }
> --
> 2.49.0
>
>
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 :|
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?
>
>> }
>> @@ -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 no, silence or warning. Warning is less bad.
Correct?
>> #endif
>> }
[...]
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 :|
Hi
On Fri, Aug 8, 2025 at 12:08 PM Markus Armbruster <armbru@redhat.com> 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
> 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?
>
I don't know if this is recoverable either. It's probably not if the handle
is invalid. I guess you could turn it into a non-fatal error.
>
> 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>
>
lgtm, thanks
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> net/slirp.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> 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
> }
> @@ -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()));
> }
> #endif
> }
> --
> 2.49.0
>
>
© 2016 - 2025 Red Hat, Inc.