[PATCH] src: report error from failing to add timer/FD watches
From: Daniel P. Berrangé <berrange@redhat.com> The virEventAddHandle/Timeout APIs are unusual in that they do not report errors on failure, because they call through to function callbacks which might be provided externally to libvirt and thus won't be using libvirt's error reporting APIs. This is a rather unfortunate design characteristic as we can see most callers forgot about this special behaviour and so we are lacking error reporting in many cases. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 ++ src/logging/log_cleaner.c | 4 ++++ src/logging/log_handler.c | 4 ++++ src/lxc/lxc_controller.c | 5 ++++- src/node_device/node_device_udev.c | 10 +++++++++- src/remote/remote_ssh_helper.c | 10 ++++++++-- src/rpc/virkeepalive.c | 5 ++++- src/rpc/virnetclientstream.c | 2 ++ src/rpc/virnetserverclient.c | 5 ++++- src/rpc/virnetserverservice.c | 2 ++ 10 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 308c0372aa..6a2e2ab964 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -170,6 +170,7 @@ libxlFDRegisterEventHook(void *priv, info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event watch for FD %d", fd); VIR_FREE(info); return -1; } @@ -255,6 +256,7 @@ libxlTimeoutRegisterEventHook(void *priv, info->id = virEventAddTimeout(timeout, libxlTimerCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event timer"); VIR_FREE(info); return -1; } diff --git a/src/logging/log_cleaner.c b/src/logging/log_cleaner.c index d247fdf829..7110dfcff6 100644 --- a/src/logging/log_cleaner.c +++ b/src/logging/log_cleaner.c @@ -251,6 +251,10 @@ virLogCleanerInit(virLogHandler *handler) handler->cleanup_log_timer = virEventAddTimeout(CLEANER_LOG_TIMEOUT_MS, virLogCleanerTimer, handler, NULL); + if (handler->cleanup_log_timer < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add log cleanup timer")); + } return handler->cleanup_log_timer; } diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c index 71517bbbe5..6ad3e33ee8 100644 --- a/src/logging/log_handler.c +++ b/src/logging/log_handler.c @@ -302,6 +302,8 @@ virLogHandlerNewPostExecRestart(virJSONValue *object, virLogHandlerDomainLogFileEvent, handler, NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to add watch on log FD %1$d"), file->pipefd); VIR_DELETE_ELEMENT(handler->files, handler->nfiles - 1, handler->nfiles); goto error; } @@ -386,6 +388,8 @@ virLogHandlerDomainOpenLogFile(virLogHandler *handler, virLogHandlerDomainLogFileEvent, handler, NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to add watch on log FD %1$d"), file->pipefd); VIR_DELETE_ELEMENT(handler->files, handler->nfiles - 1, handler->nfiles); goto error; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index fb7f8e0bc2..ae00f36eb3 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -206,8 +206,11 @@ static virLXCController *virLXCControllerNew(const char *name) if ((ctrl->timerShutdown = virEventAddTimeout(-1, virLXCControllerQuitTimer, ctrl, - NULL)) < 0) + NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add shutdown timer")); goto error; + } cleanup: virLXCControllerDriverFree(driver); diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 30c2ddf568..85468150c1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2318,6 +2318,10 @@ scheduleMdevctlUpdate(udevEventData *data) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); + if (data->mdevctlTimeout < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add mdev update timer")); + } } @@ -2609,8 +2613,12 @@ nodeStateInitialize(bool privileged, priv->watch = virEventAddHandle(udev_monitor_get_fd(priv->udev_monitor), VIR_EVENT_HANDLE_READABLE, udevEventHandleCallback, virObjectRef(priv), virObjectUnref); - if (priv->watch == -1) + if (priv->watch == -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to add watch on udev FD %1$d"), + udev_monitor_get_fd(priv->udev_monitor)); goto unlock; + } if (mdevctlEnableMonitor(priv) < 0) goto unlock; diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index 2d332a39b6..48896fd559 100644 --- a/src/remote/remote_ssh_helper.c +++ b/src/remote/remote_ssh_helper.c @@ -316,15 +316,21 @@ virRemoteSSHHelperRun(virNetSocket *sock) VIR_EVENT_HANDLE_READABLE, virRemoteSSHHelperEventOnStdin, &proxy, - NULL)) < 0) + NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add watch on stdin")); goto cleanup; + } if ((proxy.stdoutWatch = virEventAddHandle(STDOUT_FILENO, 0, virRemoteSSHHelperEventOnStdout, &proxy, - NULL)) < 0) + NULL)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add watch on stdout")); goto cleanup; + } if (virNetSocketAddIOCallback(proxy.sock, VIR_EVENT_HANDLE_READABLE, diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index d96bd347ad..690bc08b2e 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -276,8 +276,11 @@ virKeepAliveStart(virKeepAlive *ka, ka->intervalStart = now - (ka->interval - timeout); ka->timer = virEventAddTimeout(timeout * 1000, virKeepAliveTimer, ka, virObjectUnref); - if (ka->timer < 0) + if (ka->timer < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add keepalive timer")); goto cleanup; + } /* the timer now has another reference to this object */ virObjectRef(ka); diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index 98034d737d..380b785869 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -725,6 +725,8 @@ int virNetClientStreamEventAddCallback(virNetClientStream *st, virNetClientStreamEventTimer, st, virObjectUnref)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add timer to event loop")); virObjectUnref(st); goto cleanup; } diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 355aab4b04..e2967e5e1f 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -396,8 +396,11 @@ virNetServerClientNewInternal(unsigned long long id, client->sockTimer = virEventAddTimeout(-1, virNetServerClientSockTimerFunc, client, NULL); - if (client->sockTimer < 0) + if (client->sockTimer < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add socket timer")); goto error; + } /* Prepare one for packet receive */ if (!(client->rx = virNetMessageNew(true))) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 682b2091c1..babdedee35 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -154,6 +154,8 @@ virNetServerServiceNewSocket(virNetSocket **socks, svc->timer = virEventAddTimeout(-1, virNetServerServiceTimerFunc, svc, virObjectUnref); if (svc->timer < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add service timer")); virObjectUnref(svc); goto error; } -- 2.50.1
On Fri, Oct 03, 2025 at 13:43:31 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The virEventAddHandle/Timeout APIs are unusual in that they do not report errors on failure, because they call through to function callbacks which might be provided externally to libvirt and thus won't be using libvirt's error reporting APIs.
This is a rather unfortunate design characteristic as we can see most callers forgot about this special behaviour and so we are lacking error reporting in many cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 ++ src/logging/log_cleaner.c | 4 ++++ src/logging/log_handler.c | 4 ++++ src/lxc/lxc_controller.c | 5 ++++- src/node_device/node_device_udev.c | 10 +++++++++- src/remote/remote_ssh_helper.c | 10 ++++++++-- src/rpc/virkeepalive.c | 5 ++++- src/rpc/virnetclientstream.c | 2 ++ src/rpc/virnetserverclient.c | 5 ++++- src/rpc/virnetserverservice.c | 2 ++ 10 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 308c0372aa..6a2e2ab964 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -170,6 +170,7 @@ libxlFDRegisterEventHook(void *priv, info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event watch for FD %d", fd); VIR_FREE(info); return -1; } @@ -255,6 +256,7 @@ libxlTimeoutRegisterEventHook(void *priv, info->id = virEventAddTimeout(timeout, libxlTimerCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event timer"); VIR_FREE(info); return -1; }
IMO adding a warning here will be questionably useful as it's just logged. Does the code that invokes these callbacks report an error that would be correlatable with the warning?
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 30c2ddf568..85468150c1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2318,6 +2318,10 @@ scheduleMdevctlUpdate(udevEventData *data) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); + if (data->mdevctlTimeout < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add mdev update timer")); + }
This code path doesn't raise errors, so it'll likely only get logged. For the rest: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Fri, Oct 03, 2025 at 02:58:59PM +0200, Peter Krempa wrote:
On Fri, Oct 03, 2025 at 13:43:31 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The virEventAddHandle/Timeout APIs are unusual in that they do not report errors on failure, because they call through to function callbacks which might be provided externally to libvirt and thus won't be using libvirt's error reporting APIs.
This is a rather unfortunate design characteristic as we can see most callers forgot about this special behaviour and so we are lacking error reporting in many cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 ++ src/logging/log_cleaner.c | 4 ++++ src/logging/log_handler.c | 4 ++++ src/lxc/lxc_controller.c | 5 ++++- src/node_device/node_device_udev.c | 10 +++++++++- src/remote/remote_ssh_helper.c | 10 ++++++++-- src/rpc/virkeepalive.c | 5 ++++- src/rpc/virnetclientstream.c | 2 ++ src/rpc/virnetserverclient.c | 5 ++++- src/rpc/virnetserverservice.c | 2 ++ 10 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 308c0372aa..6a2e2ab964 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -170,6 +170,7 @@ libxlFDRegisterEventHook(void *priv, info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event watch for FD %d", fd); VIR_FREE(info); return -1; } @@ -255,6 +256,7 @@ libxlTimeoutRegisterEventHook(void *priv, info->id = virEventAddTimeout(timeout, libxlTimerCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event timer"); VIR_FREE(info); return -1; }
IMO adding a warning here will be questionably useful as it's just logged. Does the code that invokes these callbacks report an error that would be correlatable with the warning?
Yeah it isn't ideal, but these two methods are callbacks that are registered with libxl.so, so we don't have a better way to reoprt errors from them AFAIK.
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 30c2ddf568..85468150c1 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -2318,6 +2318,10 @@ scheduleMdevctlUpdate(udevEventData *data) virEventRemoveTimeout(data->mdevctlTimeout); data->mdevctlTimeout = virEventAddTimeout(100, submitMdevctlUpdate, data, NULL); + if (data->mdevctlTimeout < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add mdev update timer")); + }
This code path doesn't raise errors, so it'll likely only get logged.
Yep, guess I could use VIR_WARN instead to make that more obvious.
For the rest:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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 :|
On Fri, Oct 03, 2025 at 14:05:39 +0100, Daniel P. Berrangé wrote:
On Fri, Oct 03, 2025 at 02:58:59PM +0200, Peter Krempa wrote:
On Fri, Oct 03, 2025 at 13:43:31 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The virEventAddHandle/Timeout APIs are unusual in that they do not report errors on failure, because they call through to function callbacks which might be provided externally to libvirt and thus won't be using libvirt's error reporting APIs.
This is a rather unfortunate design characteristic as we can see most callers forgot about this special behaviour and so we are lacking error reporting in many cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 ++ src/logging/log_cleaner.c | 4 ++++ src/logging/log_handler.c | 4 ++++ src/lxc/lxc_controller.c | 5 ++++- src/node_device/node_device_udev.c | 10 +++++++++- src/remote/remote_ssh_helper.c | 10 ++++++++-- src/rpc/virkeepalive.c | 5 ++++- src/rpc/virnetclientstream.c | 2 ++ src/rpc/virnetserverclient.c | 5 ++++- src/rpc/virnetserverservice.c | 2 ++ 10 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 308c0372aa..6a2e2ab964 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -170,6 +170,7 @@ libxlFDRegisterEventHook(void *priv, info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event watch for FD %d", fd); VIR_FREE(info); return -1; } @@ -255,6 +256,7 @@ libxlTimeoutRegisterEventHook(void *priv, info->id = virEventAddTimeout(timeout, libxlTimerCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event timer"); VIR_FREE(info); return -1; }
IMO adding a warning here will be questionably useful as it's just logged. Does the code that invokes these callbacks report an error that would be correlatable with the warning?
Yeah it isn't ideal, but these two methods are callbacks that are registered with libxl.so, so we don't have a better way to reoprt errors from them AFAIK.
Is it reasonably certain that these won't be called repeatedly if they start failing? So that we don't spam the logs continuously. If yes then consider my R-b also on the warnings.
On Fri, Oct 03, 2025 at 03:27:15PM +0200, Peter Krempa wrote:
On Fri, Oct 03, 2025 at 14:05:39 +0100, Daniel P. Berrangé wrote:
On Fri, Oct 03, 2025 at 02:58:59PM +0200, Peter Krempa wrote:
On Fri, Oct 03, 2025 at 13:43:31 +0100, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The virEventAddHandle/Timeout APIs are unusual in that they do not report errors on failure, because they call through to function callbacks which might be provided externally to libvirt and thus won't be using libvirt's error reporting APIs.
This is a rather unfortunate design characteristic as we can see most callers forgot about this special behaviour and so we are lacking error reporting in many cases.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/libxl/libxl_driver.c | 2 ++ src/logging/log_cleaner.c | 4 ++++ src/logging/log_handler.c | 4 ++++ src/lxc/lxc_controller.c | 5 ++++- src/node_device/node_device_udev.c | 10 +++++++++- src/remote/remote_ssh_helper.c | 10 ++++++++-- src/rpc/virkeepalive.c | 5 ++++- src/rpc/virnetclientstream.c | 2 ++ src/rpc/virnetserverclient.c | 5 ++++- src/rpc/virnetserverservice.c | 2 ++ 10 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 308c0372aa..6a2e2ab964 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -170,6 +170,7 @@ libxlFDRegisterEventHook(void *priv, info->id = virEventAddHandle(fd, vir_events, libxlFDEventCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event watch for FD %d", fd); VIR_FREE(info); return -1; } @@ -255,6 +256,7 @@ libxlTimeoutRegisterEventHook(void *priv, info->id = virEventAddTimeout(timeout, libxlTimerCallback, info, libxlOSEventHookInfoFree); if (info->id < 0) { + VIR_WARN("Failed to add event timer"); VIR_FREE(info); return -1; }
IMO adding a warning here will be questionably useful as it's just logged. Does the code that invokes these callbacks report an error that would be correlatable with the warning?
Yeah it isn't ideal, but these two methods are callbacks that are registered with libxl.so, so we don't have a better way to reoprt errors from them AFAIK.
Is it reasonably certain that these won't be called repeatedly if they start failing? So that we don't spam the logs continuously.
If yes then consider my R-b also on the warnings.
In practice these ones should never fail, as we're inside the daemon and thus can expect our glib event loop impl. Only parts of this patch which could run outside the deamon are liable to see failures from a custom event loop, or more likely, app forgot to register any event loop 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 :|
participants (2)
-
Daniel P. Berrangé -
Peter Krempa