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

Philippe Mathieu-Daudé posted 2 patches 4 months ago
Maintainers: Stefan Weil <sw@weilnetz.de>, "Daniel P. Berrangé" <berrange@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Fam Zheng <fam@euphon.net>
[RFC PATCH 2/2] system/os-win32: Remove unused Error** argument in qemu_socket_select
Posted by Philippe Mathieu-Daudé 4 months ago
@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
Posted by Daniel P. Berrangé 4 months ago
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 2/2] system/os-win32: Remove unused Error** argument in qemu_socket_select
Posted by Markus Armbruster 4 months ago
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 2/2] system/os-win32: Remove unused Error** argument in qemu_socket_select
Posted by Manos Pitsidianakis 4 months ago
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>