Re: [PATCH 08/12] util/oslib-win32: Revert warning on WSAEventSelect() failure

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> 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 <marcandre.lureau@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  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?

>> -
>>      if (s == INVALID_SOCKET) {
>>          error_setg(errp, "invalid socket fd=%d", sockfd);
>>          return false;
>> -- 
>> 2.49.0
>> 
>> 
>
> With regards,
> Daniel