Daniel P. Berrangé <berra...@redhat.com> writes:
> On Wed, Jul 16, 2025 at 12:41:31PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berra...@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é <phi...@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?
[...]
