[PATCH 1/8] log error if virConnectCacheOnceInit() fails

[Laine Stump]

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 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 :-)


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
>