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