On Tue, Sep 09, 2025 at 01:50:56PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <[email protected]> writes:
>
> > On Fri, Aug 08, 2025 at 10:08:19AM +0200, Markus Armbruster wrote:
> >> qemu_socket_select() and its wrapper qemu_socket_unselect() treat a
> >> NULL @errp as &error_warn. This is wildly inappropriate. A caller
> >> passing NULL specifies that errors are to be ignored. If warnings are
> >> wanted, the caller must pass &error_warn.
> >>
> >> I'm not familiar with the calling code, so I can't say whether it will
> >> work after WSAEventSelect() 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?
> >>
> >> The warning goes back to commit f5fd677ae7cf (win32/socket: introduce
> >> qemu_socket_select() helper). Before that commit, the error was
> >> ignored, as indicated by passing a null @errp. Revert to that
> >> behavior.
> >>
> >> Cc: Marc-André Lureau <[email protected]>
> >> Signed-off-by: Markus Armbruster <[email protected]>
> >> ---
> >> util/oslib-win32.c | 4 ----
> >> 1 file changed, 4 deletions(-)
> >>
> >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> >> index b7351634ec..136a8fe118 100644
> >> --- a/util/oslib-win32.c
> >> +++ b/util/oslib-win32.c
> >> @@ -296,10 +296,6 @@ bool qemu_socket_select(int sockfd, WSAEVENT
> >> hEventObject,
> >> {
> >> SOCKET s = _get_osfhandle(sockfd);
> >>
> >> - if (errp == NULL) {
> >> - errp = &error_warn;
> >> - }
> >
> > This makes sense, but I'd want the callers to be using warn_report
> > instead. Ideally some (but not all) of the callers would propagate
> > the error, but this isn't practical with the QIOChannel create
> > watch function usage. I'd want to keep Error *errp on this function
> > though, and have warn_report as a sign to our future selves that
> > this is still not ideal.
>
> The direct callers are qio_channel_create_socket_watch(),
> aio_set_fd_handler(). Callers via qemu_socket_unselect() are
> qio_channel_socket_finalize(), qio_channel_socket_close(),
> qemu_socket_set_block().
>
> All but qio_channel_socket_close() cannot fail. Would you like me to
> make them pass &error_warn, because warning is less bad than silence
> there?
>
> qio_channel_socket_close() can fail, but it ignores
> qemu_socket_unselect() failure. What do you want me to do there?
I think the overriding important thing is that we /must/ try to
close(), and if close() succeeds claim the whole qio_channel_socket_close
was successful. So I guess I'd say that &error_warn should be passed
from all callers.
>
> >> -
> >> if (s == INVALID_SOCKET) {
> >> error_setg(errp, "invalid socket fd=%d", sockfd);
> >> return false;
> >> --
> >> 2.49.0
> >>
> >>
> >
> > With regards,
> > Daniel
>
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 :|
