[PATCH 1/8] log error if virConnectCacheOnceInit() fails
Michal Privoznik
mprivozn at redhat.com
Mon Feb 1 09:18:53 UTC 2021
On 2/1/21 7:27 AM, Laine Stump wrote:
> This may be pointless, but I noticed it and it was bugging me.
>
> virGetConnectNetwork() calls
> virGetConnectGeneric(), which calls
> virConnecCacheInitialize(), which is actually a call (only once) to
> virConnectCacheOnceInit() which calls
> virThreadLocalInit() several times, which calls
> pthread_key_create()
>
> If pthread_key_create() fails, it (of course) doesn't log an error
> (because it's not a part of libvirt), nor does any other function on
> the call chain all the way up to virGetConnectNetwork(). But none of
> the callers of virGetConnectNetwork() log an error either, so it is
> possible that an API could fail due to virGetConnectNetwork() failing,
> but would only log "an error was encountered, but the cause is
> unknown. Deal with it." (paraphrasing).
>
> In all likelyhood, virConnectCacheOnceInit() is going to be called at
> some earlier time, and almost certainly pthread_key_create() will
> never fail (and if it does, the user will have *much* bigger problems
> than an obtuse error message from libvirt). So I don't know if there's
> any value at all in pushing this. Just throwing it out there...
>
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
> src/driver.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/driver.c b/src/driver.c
> index e005a89d57..1dae7cf284 100644
> --- a/src/driver.c
> +++ b/src/driver.c
> @@ -116,18 +116,18 @@ virThreadLocal connectStorage;
> static int
> virConnectCacheOnceInit(void)
> {
> - if (virThreadLocalInit(&connectInterface, NULL) < 0)
> - return -1;
> - if (virThreadLocalInit(&connectNetwork, NULL) < 0)
> - return -1;
> - if (virThreadLocalInit(&connectNWFilter, NULL) < 0)
> - return -1;
> - if (virThreadLocalInit(&connectNodeDev, NULL) < 0)
> - return -1;
> - if (virThreadLocalInit(&connectSecret, NULL) < 0)
> - return -1;
> - if (virThreadLocalInit(&connectStorage, NULL) < 0)
> + if (virThreadLocalInit(&connectInterface, NULL) < 0
> + || virThreadLocalInit(&connectNetwork, NULL) < 0
> + || virThreadLocalInit(&connectNWFilter, NULL) < 0
> + || virThreadLocalInit(&connectNodeDev, NULL) < 0
> + || virThreadLocalInit(&connectSecret, NULL) < 0
> + || virThreadLocalInit(&connectStorage, NULL) < 0) {
I think our preferred way of breaking these checks is to put '||' at the
end of each line rather than at the beginning. It it isn't then it
should be :-)
> +
Maybe drop this empty line?
> + virReportSystemError(errno, "%s",
> + _("Unable to initialize thread local variable"));
> return -1;
> + }
> +
> return 0;
> }
>
>
Michal
More information about the libvir-list
mailing list