[RFC PATCH 0/2] system/win32: Remove unused Error argument in qemu_socket_[un]select()

[RFC PATCH 0/2] system/win32: Remove unused Error argument in qemu_socket_[un]select()

[Philippe Mathieu-Daudé]

We should use Error** more, but in this case it is always NULL so
we never really care. Remove the argument...

Philippe Mathieu-Daudé (2):
  system/os-win32: Remove unused Error** argument in
    qemu_socket_unselect
  system/os-win32: Remove unused Error** argument in qemu_socket_select

 include/system/os-win32.h |  4 ++--
 io/channel-socket.c       |  4 ++--
 util/aio-win32.c          |  2 +-
 util/oslib-win32.c        | 17 +++++++----------
 4 files changed, 12 insertions(+), 15 deletions(-)

-- 
2.49.0

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

[Philippe Mathieu-Daudé]

@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)
             }
         }
 #ifdef WIN32
-        qemu_socket_unselect(ioc->fd, NULL);
+        qemu_socket_unselect(ioc->fd);
 #endif
         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
         if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
             socket_listen_cleanup(sioc->fd, errp);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b7351634ece..7ac3482d449 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -180,7 +180,7 @@ static int socket_error(void)
 void qemu_socket_set_block(int fd)
 {
     unsigned long opt = 0;
-    qemu_socket_unselect(fd, NULL);
+    qemu_socket_unselect(fd);
     ioctlsocket(fd, FIONBIO, &opt);
 }
 
@@ -313,9 +313,9 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
     return true;
 }
 
-bool qemu_socket_unselect(int sockfd, Error **errp)
+bool qemu_socket_unselect(int sockfd)
 {
-    return qemu_socket_select(sockfd, NULL, 0, errp);
+    return qemu_socket_select(sockfd, NULL, 0, NULL);
 }
 
 int qemu_socketpair(int domain, int type, int protocol, int sv[2])
-- 
2.49.0

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

[Philippe Mathieu-Daudé]

@errp is always NULL. Remove it, as unused.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/system/os-win32.h |  2 +-
 util/aio-win32.c          |  2 +-
 util/oslib-win32.c        | 13 +++++--------
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/include/system/os-win32.h b/include/system/os-win32.h
index 40712a948c3..47882bc2f49 100644
--- a/include/system/os-win32.h
+++ b/include/system/os-win32.h
@@ -170,7 +170,7 @@ static inline void qemu_funlockfile(FILE *f)
 
 /* Helper for WSAEventSelect, to report errors */
 bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
-                        long lNetworkEvents, Error **errp);
+                        long lNetworkEvents);
 
 bool qemu_socket_unselect(int sockfd);
 
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 6583d5c5f31..9c2f0fb86e7 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
 
         QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
         event = event_notifier_get_handle(&ctx->notifier);
-        qemu_socket_select(fd, event, bitmask, NULL);
+        qemu_socket_select(fd, event, bitmask);
     }
     if (old_node) {
         aio_remove_fd_handler(ctx, old_node);
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 7ac3482d449..fed5ab14efb 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -292,21 +292,18 @@ char *qemu_get_pid_name(pid_t pid)
 
 
 bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
-                        long lNetworkEvents, Error **errp)
+                        long lNetworkEvents)
 {
     SOCKET s = _get_osfhandle(sockfd);
 
-    if (errp == NULL) {
-        errp = &error_warn;
-    }
-
     if (s == INVALID_SOCKET) {
-        error_setg(errp, "invalid socket fd=%d", sockfd);
+        error_setg(&error_warn, "invalid socket fd=%d", sockfd);
         return false;
     }
 
     if (WSAEventSelect(s, hEventObject, lNetworkEvents) != 0) {
-        error_setg_win32(errp, WSAGetLastError(), "failed to WSAEventSelect()");
+        error_setg_win32(&error_warn, WSAGetLastError(),
+                         "failed to WSAEventSelect()");
         return false;
     }
 
@@ -315,7 +312,7 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
 
 bool qemu_socket_unselect(int sockfd)
 {
-    return qemu_socket_select(sockfd, NULL, 0, NULL);
+    return qemu_socket_select(sockfd, NULL, 0);
 }
 
 int qemu_socketpair(int domain, int type, int protocol, int sv[2])
-- 
2.49.0

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

[Manos Pitsidianakis]

On Tue, Jul 15, 2025 at 11:36 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> @errp is always NULL. Remove it, as unused.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/system/os-win32.h |  2 +-
>  util/aio-win32.c          |  2 +-
>  util/oslib-win32.c        | 13 +++++--------
>  3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index 40712a948c3..47882bc2f49 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -170,7 +170,7 @@ static inline void qemu_funlockfile(FILE *f)
>
>  /* Helper for WSAEventSelect, to report errors */
>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> -                        long lNetworkEvents, Error **errp);
> +                        long lNetworkEvents);
>
>  bool qemu_socket_unselect(int sockfd);
>
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 6583d5c5f31..9c2f0fb86e7 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
>
>          QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
>          event = event_notifier_get_handle(&ctx->notifier);
> -        qemu_socket_select(fd, event, bitmask, NULL);
> +        qemu_socket_select(fd, event, bitmask);
>      }
>      if (old_node) {
>          aio_remove_fd_handler(ctx, old_node);
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 7ac3482d449..fed5ab14efb 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -292,21 +292,18 @@ char *qemu_get_pid_name(pid_t pid)
>
>
>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> -                        long lNetworkEvents, Error **errp)
> +                        long lNetworkEvents)
>  {
>      SOCKET s = _get_osfhandle(sockfd);
>
> -    if (errp == NULL) {
> -        errp = &error_warn;
> -    }
> -
>      if (s == INVALID_SOCKET) {
> -        error_setg(errp, "invalid socket fd=%d", sockfd);
> +        error_setg(&error_warn, "invalid socket fd=%d", sockfd);
>          return false;
>      }
>
>      if (WSAEventSelect(s, hEventObject, lNetworkEvents) != 0) {
> -        error_setg_win32(errp, WSAGetLastError(), "failed to WSAEventSelect()");
> +        error_setg_win32(&error_warn, WSAGetLastError(),
> +                         "failed to WSAEventSelect()");
>          return false;
>      }
>
> @@ -315,7 +312,7 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>
>  bool qemu_socket_unselect(int sockfd)
>  {
> -    return qemu_socket_select(sockfd, NULL, 0, NULL);
> +    return qemu_socket_select(sockfd, NULL, 0);
>  }
>
>  int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> --
> 2.49.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

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

[Manos Pitsidianakis]

On Tue, Jul 15, 2025 at 11:36 AM Philippe Mathieu-Daudé
<philmd@linaro.org> 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)
>              }
>          }
>  #ifdef WIN32
> -        qemu_socket_unselect(ioc->fd, NULL);
> +        qemu_socket_unselect(ioc->fd);
>  #endif
>          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
>          if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
>              socket_listen_cleanup(sioc->fd, errp);
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ece..7ac3482d449 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -180,7 +180,7 @@ static int socket_error(void)
>  void qemu_socket_set_block(int fd)
>  {
>      unsigned long opt = 0;
> -    qemu_socket_unselect(fd, NULL);
> +    qemu_socket_unselect(fd);
>      ioctlsocket(fd, FIONBIO, &opt);
>  }
>
> @@ -313,9 +313,9 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>      return true;
>  }
>
> -bool qemu_socket_unselect(int sockfd, Error **errp)
> +bool qemu_socket_unselect(int sockfd)
>  {
> -    return qemu_socket_select(sockfd, NULL, 0, errp);
> +    return qemu_socket_select(sockfd, NULL, 0, NULL);
>  }
>
>  int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> --
> 2.49.0
>
>

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

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

[Daniel P. Berrangé]

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)
>              }
>          }
>  #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.


>          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

>          if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
>              socket_listen_cleanup(sioc->fd, errp);
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ece..7ac3482d449 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -180,7 +180,7 @@ static int socket_error(void)
>  void qemu_socket_set_block(int fd)
>  {
>      unsigned long opt = 0;
> -    qemu_socket_unselect(fd, NULL);
> +    qemu_socket_unselect(fd);
>      ioctlsocket(fd, FIONBIO, &opt);
>  }
>  
> @@ -313,9 +313,9 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>      return true;
>  }
>  
> -bool qemu_socket_unselect(int sockfd, Error **errp)
> +bool qemu_socket_unselect(int sockfd)
>  {
> -    return qemu_socket_select(sockfd, NULL, 0, errp);
> +    return qemu_socket_select(sockfd, NULL, 0, NULL);
>  }
>  
>  int qemu_socketpair(int domain, int type, int protocol, int sv[2])
> -- 
> 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 :|

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

[Daniel P. Berrangé]

On Tue, Jul 15, 2025 at 10:35:17AM +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 +-
>  util/aio-win32.c          |  2 +-
>  util/oslib-win32.c        | 13 +++++--------
>  3 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
> index 40712a948c3..47882bc2f49 100644
> --- a/include/system/os-win32.h
> +++ b/include/system/os-win32.h
> @@ -170,7 +170,7 @@ static inline void qemu_funlockfile(FILE *f)
>  
>  /* Helper for WSAEventSelect, to report errors */
>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> -                        long lNetworkEvents, Error **errp);
> +                        long lNetworkEvents);
>  
>  bool qemu_socket_unselect(int sockfd);
>  
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 6583d5c5f31..9c2f0fb86e7 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
>  
>          QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
>          event = event_notifier_get_handle(&ctx->notifier);
> -        qemu_socket_select(fd, event, bitmask, NULL);
> +        qemu_socket_select(fd, event, bitmask);

This should likely be &error_abort, as we never expect this
to fail AFAICT.


> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 7ac3482d449..fed5ab14efb 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -292,21 +292,18 @@ char *qemu_get_pid_name(pid_t pid)
>  
>  
>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
> -                        long lNetworkEvents, Error **errp)
> +                        long lNetworkEvents)
>  {
>      SOCKET s = _get_osfhandle(sockfd);
>  
> -    if (errp == NULL) {
> -        errp = &error_warn;
> -    }

This pre-existing code should never have existed - the caller should
have decided this reporting policy by passing in &error_warn.

> -
>      if (s == INVALID_SOCKET) {
> -        error_setg(errp, "invalid socket fd=%d", sockfd);
> +        error_setg(&error_warn, "invalid socket fd=%d", sockfd);
>          return false;
>      }
>  
>      if (WSAEventSelect(s, hEventObject, lNetworkEvents) != 0) {
> -        error_setg_win32(errp, WSAGetLastError(), "failed to WSAEventSelect()");
> +        error_setg_win32(&error_warn, WSAGetLastError(),
> +                         "failed to WSAEventSelect()");
>          return false;
>      }
>  
> @@ -315,7 +312,7 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,


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

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

[Markus Armbruster]

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);

[...]

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

[Markus Armbruster]

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

> On Tue, Jul 15, 2025 at 10:35:17AM +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 +-
>>  util/aio-win32.c          |  2 +-
>>  util/oslib-win32.c        | 13 +++++--------
>>  3 files changed, 7 insertions(+), 10 deletions(-)
>> 
>> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
>> index 40712a948c3..47882bc2f49 100644
>> --- a/include/system/os-win32.h
>> +++ b/include/system/os-win32.h
>> @@ -170,7 +170,7 @@ static inline void qemu_funlockfile(FILE *f)
>>  
>>  /* Helper for WSAEventSelect, to report errors */
>>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>> -                        long lNetworkEvents, Error **errp);
>> +                        long lNetworkEvents);
>>  
>>  bool qemu_socket_unselect(int sockfd);
>>  
>> diff --git a/util/aio-win32.c b/util/aio-win32.c
>> index 6583d5c5f31..9c2f0fb86e7 100644
>> --- a/util/aio-win32.c
>> +++ b/util/aio-win32.c
>> @@ -121,7 +121,7 @@ void aio_set_fd_handler(AioContext *ctx,
>>  
>>          QLIST_INSERT_HEAD_RCU(&ctx->aio_handlers, node, node);
>>          event = event_notifier_get_handle(&ctx->notifier);
>> -        qemu_socket_select(fd, event, bitmask, NULL);
>> +        qemu_socket_select(fd, event, bitmask);
>
> This should likely be &error_abort, as we never expect this
> to fail AFAICT.
>
>
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index 7ac3482d449..fed5ab14efb 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -292,21 +292,18 @@ char *qemu_get_pid_name(pid_t pid)
>>  
>>  
>>  bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>> -                        long lNetworkEvents, Error **errp)
>> +                        long lNetworkEvents)
>>  {
>>      SOCKET s = _get_osfhandle(sockfd);
>>  
>> -    if (errp == NULL) {
>> -        errp = &error_warn;
>> -    }
>
> This pre-existing code should never have existed - the caller should
> have decided this reporting policy by passing in &error_warn.

Yes.

A function that uses an Error **errp parameter to return errors leaves
handling the error to the caller.  The code you remove violates this
principle.

>
>> -
>>      if (s == INVALID_SOCKET) {
>> -        error_setg(errp, "invalid socket fd=%d", sockfd);
>> +        error_setg(&error_warn, "invalid socket fd=%d", sockfd);

From error_setg()'s contract:

 * Please don't error_setg(&error_fatal, ...), use error_report() and
 * exit(), because that's more obvious.
 * Likewise, don't error_setg(&error_abort, ...), use assert().

Not said: use warn_report() instead of error_set(&error_warn).
Should've been added in commit 3ffef1a55ca (error: add global
&error_warn destination).

I consider &error_warn a mistake.

>>          return false;
>>      }
>>  
>>      if (WSAEventSelect(s, hEventObject, lNetworkEvents) != 0) {
>> -        error_setg_win32(errp, WSAGetLastError(), "failed to WSAEventSelect()");
>> +        error_setg_win32(&error_warn, WSAGetLastError(),
>> +                         "failed to WSAEventSelect()");
>>          return false;
>>      }
>>  
>> @@ -315,7 +312,7 @@ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>
>
> With regards,
> Daniel

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

[Daniel P. Berrangé]

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.

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

Close can propagate errors, but we need a little more careful dance.
We really really really want to still successfully close the underlying
socket FD, and if that causes an error too, we'll prefer to see the
latter error, over this earlier error IMHO. So we should propagate an
error here, but delay propagation until the rest of this method has
run.

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

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

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

[Markus Armbruster]

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?

[...]