@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
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 :|
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);
[...]
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 :|
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?
[...]
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>
© 2016 - 2025 Red Hat, Inc.