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 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?

>
>>          close(ioc->fd);
>>          ioc->fd = -1;
>> @@ -890,7 +890,7 @@ qio_channel_socket_close(QIOChannel *ioc,
>>  
>>      if (sioc->fd != -1) {
>>  #ifdef WIN32
>> -        qemu_socket_unselect(sioc->fd, NULL);
>> +        qemu_socket_unselect(sioc->fd);
>>  #endif
>
> Here too, we don't want to stop the close operation early,
> as we need to 'close()' the FD, but we should diagnose
> the failure none the less

Same questions.

>>          if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
>>              socket_listen_cleanup(sioc->fd, errp);

[...]