On Fri, Oct 03, 2025 at 13:43:31 +0100, Daniel P. Berrangé via Devel wrote:
> From: Daniel P. Berrangé <[email protected]>
>
> 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é <[email protected]>
> ---
> 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 <[email protected]>
