[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