From 5bd58f04b831f7086f21ae70c90d1a86b6565762 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Sep 2025 11:09:55 +0200 Subject: util/oslib-win32: Do not treat null @errp as &error_warn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qemu_socket_select() and its wrapper qemu_socket_unselect() treat a null @errp as &error_warn. This is wildly inappropriate. A caller passing null @errp specifies that errors are to be ignored. If warnings are wanted, the caller must pass &error_warn. Change callers to do that, and drop the inappropriate treatment of null @errp. This assumes that warnings are wanted. I'm not familiar with the calling code, so I can't say whether it will work when the socket is invalid, or WSAEventSelect() fails. If it doesn't, then this should be an error instead of a warning. Invalid socket might even be a programming error. These warnings were introduced in commit f5fd677ae7cf (win32/socket: introduce qemu_socket_select() helper). I considered reverting to silence, but Daniel Berrangé asked for the warnings to be preserved. Cc: Marc-André Lureau Signed-off-by: Markus Armbruster Reviewed-by: Richard Henderson Message-ID: <20250923091000.3180122-9-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Akihiko Odaki --- io/channel-socket.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'io/channel-socket.c') diff --git a/io/channel-socket.c b/io/channel-socket.c index e53d9ac76f..fdc7670867 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, &error_warn); #endif close(ioc->fd); ioc->fd = -1; @@ -929,7 +929,7 @@ qio_channel_socket_close(QIOChannel *ioc, if (sioc->fd != -1) { #ifdef WIN32 - qemu_socket_unselect(sioc->fd, NULL); + qemu_socket_unselect(sioc->fd, &error_warn); #endif if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) { socket_listen_cleanup(sioc->fd, errp); -- cgit 1.4.1 From bcb536cabe108e71e2900cdd605f5b4e59ac3e1f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Sep 2025 11:10:00 +0200 Subject: error: Kill @error_warn We added @error_warn some two years ago in commit 3ffef1a55ca (error: add global &error_warn destination). It has multiple issues: * error.h's big comment was not updated for it. * Function contracts were not updated for it. * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from error_prepend() and such. These crash on @error_warn, as pointed out by Akihiko Odaki. All fixable. However, after more than two years, we had just of 15 uses, of which the last few patches removed seven as unclean or otherwise undesirable, adding back five elsewhere. I didn't look closely enough at the remaining seven to decide whether they are desirable or not. I don't think this feature earns its keep. Drop it. Thanks-to: Akihiko Odaki Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Message-ID: <20250923091000.3180122-14-armbru@redhat.com> Reviewed-by: Akihiko Odaki --- hw/display/virtio-gpu.c | 8 ++++++-- hw/net/virtio-net.c | 8 +++++++- include/qapi/error.h | 6 ------ include/system/os-win32.h | 5 ++++- io/channel-socket.c | 4 ++-- io/channel-watch.c | 6 +++--- tests/unit/test-error-report.c | 17 ----------------- ui/gtk.c | 6 +++++- util/aio-win32.c | 2 +- util/error.c | 5 +---- util/oslib-win32.c | 21 ++++++++++++++++++++- 11 files changed, 49 insertions(+), 39 deletions(-) (limited to 'io/channel-socket.c') diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 0a1a625b0e..de35902213 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -242,6 +242,7 @@ static uint32_t calc_image_hostmem(pixman_format_code_t pformat, static void virtio_gpu_resource_create_2d(VirtIOGPU *g, struct virtio_gpu_ctrl_command *cmd) { + Error *err = NULL; pixman_format_code_t pformat; struct virtio_gpu_simple_resource *res; struct virtio_gpu_resource_create_2d c2d; @@ -293,7 +294,8 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g, c2d.width, c2d.height, c2d.height ? res->hostmem / c2d.height : 0, - &error_warn)) { + &err)) { + warn_report_err(err); goto end; } } @@ -1282,6 +1284,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, const VMStateField *field) { VirtIOGPU *g = opaque; + Error *err = NULL; struct virtio_gpu_simple_resource *res; uint32_t resource_id, pformat; int i; @@ -1317,7 +1320,8 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, res->width, res->height, res->height ? res->hostmem / res->height : 0, - &error_warn)) { + &err)) { + warn_report_err(err); g_free(res); return -EINVAL; } diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 6b5b5dace3..7848e26278 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1289,6 +1289,8 @@ exit: static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp) { + Error *err = NULL; + if (!virtio_net_attach_ebpf_to_backend(n->nic, -1)) { return true; } @@ -1306,7 +1308,11 @@ static bool virtio_net_load_ebpf(VirtIONet *n, Error **errp) return virtio_net_load_ebpf_fds(n, errp); } - ebpf_rss_load(&n->ebpf_rss, &error_warn); + ebpf_rss_load(&n->ebpf_rss, &err); + /* Beware, ebpf_rss_load() can return false with @err unset */ + if (err) { + warn_report_err(err); + } return true; } diff --git a/include/qapi/error.h b/include/qapi/error.h index 41e3816380..b16c6303f8 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -533,12 +533,6 @@ static inline void error_propagator_cleanup(ErrorPropagator *prop) G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup); -/* - * Special error destination to warn on error. - * See error_setg() and error_propagate() for details. - */ -extern Error *error_warn; - /* * Special error destination to abort on error. * See error_setg() and error_propagate() for details. diff --git a/include/system/os-win32.h b/include/system/os-win32.h index 3aa6cee4c2..22d72babdf 100644 --- a/include/system/os-win32.h +++ b/include/system/os-win32.h @@ -168,11 +168,14 @@ static inline void qemu_funlockfile(FILE *f) #endif } -/* Helper for WSAEventSelect, to report errors */ +/* Helpers for WSAEventSelect() */ bool qemu_socket_select(int sockfd, WSAEVENT hEventObject, long lNetworkEvents, Error **errp); +void qemu_socket_select_nofail(int sockfd, WSAEVENT hEventObject, + long lNetworkEvents); bool qemu_socket_unselect(int sockfd, Error **errp); +void qemu_socket_unselect_nofail(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 fdc7670867..712b793eaf 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, &error_warn); + qemu_socket_unselect_nofail(ioc->fd); #endif close(ioc->fd); ioc->fd = -1; @@ -929,7 +929,7 @@ qio_channel_socket_close(QIOChannel *ioc, if (sioc->fd != -1) { #ifdef WIN32 - qemu_socket_unselect(sioc->fd, &error_warn); + qemu_socket_unselect_nofail(sioc->fd); #endif if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) { socket_listen_cleanup(sioc->fd, errp); diff --git a/io/channel-watch.c b/io/channel-watch.c index ec76bd1ec6..018648b36b 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -281,9 +281,9 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, GSource *source; QIOChannelSocketSource *ssource; - qemu_socket_select(sockfd, ioc->event, - FD_READ | FD_ACCEPT | FD_CLOSE | - FD_CONNECT | FD_WRITE | FD_OOB, &error_warn); + qemu_socket_select_nofail(sockfd, ioc->event, + FD_READ | FD_ACCEPT | FD_CLOSE | + FD_CONNECT | FD_WRITE | FD_OOB); source = g_source_new(&qio_channel_socket_source_funcs, sizeof(QIOChannelSocketSource)); diff --git a/tests/unit/test-error-report.c b/tests/unit/test-error-report.c index 54319c86c9..0cbde3c4cf 100644 --- a/tests/unit/test-error-report.c +++ b/tests/unit/test-error-report.c @@ -104,22 +104,6 @@ test_error_report_timestamp(void) "); } -static void -test_error_warn(void) -{ - if (g_test_subprocess()) { - error_setg(&error_warn, "Testing &error_warn"); - return; - } - - g_test_trap_subprocess(NULL, 0, 0); - g_test_trap_assert_passed(); - g_test_trap_assert_stderr("\ -test-error-report: warning: Testing &error_warn*\ -"); -} - - int main(int argc, char *argv[]) { @@ -133,7 +117,6 @@ main(int argc, char *argv[]) g_test_add_func("/error-report/glog", test_error_report_glog); g_test_add_func("/error-report/once", test_error_report_once); g_test_add_func("/error-report/timestamp", test_error_report_timestamp); - g_test_add_func("/error-report/warn", test_error_warn); return g_test_run(); } diff --git a/ui/gtk.c b/ui/gtk.c index e91d093a49..9a08cadc88 100644 --- a/ui/gtk.c +++ b/ui/gtk.c @@ -1181,6 +1181,7 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch, void *opaque) { VirtualConsole *vc = opaque; + Error *err = NULL; uint64_t num_slot = GPOINTER_TO_UINT(touch->sequence); int type = -1; @@ -1203,7 +1204,10 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch, console_handle_touch_event(vc->gfx.dcl.con, touch_slots, num_slot, surface_width(vc->gfx.ds), surface_height(vc->gfx.ds), touch->x, - touch->y, type, &error_warn); + touch->y, type, &err); + if (err) { + warn_report_err(err); + } return TRUE; } diff --git a/util/aio-win32.c b/util/aio-win32.c index b125924433..c6fbce64c2 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, &error_warn); + qemu_socket_select_nofail(fd, event, bitmask); } if (old_node) { aio_remove_fd_handler(ctx, old_node); diff --git a/util/error.c b/util/error.c index daea2142f3..0ae08225c0 100644 --- a/util/error.c +++ b/util/error.c @@ -19,7 +19,6 @@ Error *error_abort; Error *error_fatal; -Error *error_warn; static void error_handle(Error **errp, Error *err) { @@ -41,9 +40,7 @@ static void error_handle(Error **errp, Error *err) error_report_err(err); exit(1); } - if (errp == &error_warn) { - warn_report_err(err); - } else if (errp && !*errp) { + if (errp && !*errp) { *errp = err; } else { error_free(err); diff --git a/util/oslib-win32.c b/util/oslib-win32.c index 6a2367c89d..84bc65a765 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -182,7 +182,7 @@ bool qemu_set_blocking(int fd, bool block, Error **errp) unsigned long opt = block ? 0 : 1; if (block) { - qemu_socket_unselect(fd, &error_warn); + qemu_socket_unselect_nofail(fd); } if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) { @@ -311,6 +311,25 @@ bool qemu_socket_unselect(int sockfd, Error **errp) return qemu_socket_select(sockfd, NULL, 0, errp); } +void qemu_socket_select_nofail(int sockfd, WSAEVENT hEventObject, + long lNetworkEvents) +{ + Error *err = NULL; + + if (!qemu_socket_select(sockfd, hEventObject, lNetworkEvents, &err)) { + warn_report_err(err); + } +} + +void qemu_socket_unselect_nofail(int sockfd) +{ + Error *err = NULL; + + if (!qemu_socket_unselect(sockfd, &err)) { + warn_report_err(err); + } +} + int qemu_socketpair(int domain, int type, int protocol, int sv[2]) { struct sockaddr_un addr = { -- cgit 1.4.1