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é <[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?
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 <[email protected]>
>
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 :|
