Re: [PATCH 1/8] log error if virConnectCacheOnceInit() fails
On 2/1/21 4:18 AM, Michal Privoznik wrote:
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(a)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 :-)
You're correct. Sometimes my brain shifts back to 1995 mode, when I
worked with people who preferred it as above. I'll switch it.
> +
Maybe drop this empty line?
Sure.
> + virReportSystemError(errno, "%s",
> + _("Unable to initialize thread local
> variable"));
> return -1;
> + }
> +
> return 0;
> }
>
Michal