Re: [RFC PATCH 1/2] system/os-win32: Remove unused Error** argument in qemu_socket_unselect

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jul 16, 2025 at 12:41:31PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Jul 15, 2025 at 10:35:16AM +0200, Philippe Mathieu-Daudé wrote:
>> >> @errp is always NULL. Remove it, as unused.
>> >> 
>> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> >> ---
>> >>  include/system/os-win32.h | 2 +-
>> >>  io/channel-socket.c       | 4 ++--
>> >>  util/oslib-win32.c        | 6 +++---
>> >>  3 files changed, 6 insertions(+), 6 deletions(-)
>> >> 
>> >> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
>> >> index 3aa6cee4c23..40712a948c3 100644
>> >> --- a/include/system/os-win32.h
>> >> +++ b/include/system/os-win32.h
>> >> @@ -172,7 +172,7 @@ static inline void qemu_funlockfile(FILE *f)
>> >>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>> >>                          long lNetworkEvents, Error **errp);
>> >>  
>> >> -bool qemu_socket_unselect(int sockfd, Error **errp);
>> >> +bool qemu_socket_unselect(int sockfd);
>> >>  
>> >>  /* We wrap all the sockets functions so that we can set errno based on
>> >>   * WSAGetLastError(), and use file-descriptors instead of SOCKET.
>> >> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> >> index 3b7ca924ff3..6ee6217e7ac 100644
>> >> --- a/io/channel-socket.c
>> >> +++ b/io/channel-socket.c
>> >> @@ -454,7 +454,7 @@ static void qio_channel_socket_finalize(Object *obj)
>> 
>>     static void qio_channel_socket_finalize(Object *obj)
>>     {
>>         QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
>> 
>>         if (ioc->fd != -1) {
>>             QIOChannel *ioc_local = QIO_CHANNEL(ioc);
>>             if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN)) {
>>                 Error *err = NULL;
>> 
>>                 socket_listen_cleanup(ioc->fd, &err);
>>                 if (err) {
>>                     error_report_err(err);
>>                     err = NULL;
>> >>              }
>> >>          }
>> >>  #ifdef WIN32
>> >> -        qemu_socket_unselect(ioc->fd, NULL);
>> >> +        qemu_socket_unselect(ioc->fd);
>> >>  #endif
>> >
>> > It seems to me like this code should instead be using
>> > &error_warn, because the errors are still relevant and
>> > potentially a sign of a bug, but we don't want to stop
>> > this finalization path.
>> 
>> Would such a warning be actionable for the user?
>> 
>> Why is this failure a warning, but the failure right above is an error?
>> 
>> What are the possible failures?
>> 
>> On reporting errors with error_report() & friends: doing so within a
>> function that uses an Error **errp parameter to return errors is almost
>> always wrong.  Can qio_channel_socket_finalize() run within such a
>> function?
>
> No finalize() function can ever propagate errors - these are run
> transparently when the "unref" drops the last reference count.
> So error_report is the best we can manage in finalize functions.
>
> It is actionable for the user in so much as if they see such
> warnings, it is sign of a code bug somewhere and this will be
> a useful diagnostic to receive in the bug report.

Then the diagnostic should tell the user this is a bug!

Messages like

    warning: failed to WSAEventSelect()

show cavalier disregard for users.  What is WSAEventSelect, why should I
care, is my data safe, and what am I supposed to do with this now?

[...]