[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message




On 05/07/2016 11:04 AM, Erik Skultety wrote:
> On 06/05/16 21:34, John Ferlan wrote:
>> Both instances use VIR_WARN() to print the error from a failed
>> virDBusGetSystemBus() call.  Rather than use the virGetLastError
>> and need to check for valid return err pointer, just use the
>> virGetLastErrorMessage.
>>
> 
> I'm afraid these are not equivalent. virGetLastError states that it
> returns NULL if no error occurred, which isn't true in all the cases,
> i.e. both virGetLastError and virGetLastErrorMessage call
> virLastErrorObject which can actually fail when no threadlocal error
> object was found (this is OK), but then we try to create an empty one
> and assign it to the threadlocal storage, so if the allocation fails
> quietly (I think trying to log a proper message would be least of our
> concerns), but if setting the new, empty error object as thread-specific
> data fails, it will return NULL which virGetLastError just passes along
> and the original caller deals with this by checking the pointer and if
> it's NULL, "Unknown error" is used instead. Now, in your case however,
> should such a corner-case scenario happen, virGetLastErrorMessage
> interprets the NULL from virLastErrorObject as "no error" which isn't
> quite the same, it might even get a little confusing back at the client
> side. So I prefer we stick to the "checking" convention, unless we want
> to make some adjustments to the virerror module.
> 
> Erik
> 

Just to followup on Cole's post...  The bite size tasks is where the
"idea" comes from:


http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMessage.28.29

John


>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/network/bridge_driver.c       | 3 +--
>>  src/node_device/node_device_hal.c | 3 +--
>>  2 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index a34da2a..f406f04 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -695,9 +695,8 @@ networkStateInitialize(bool privileged,
>>  
>>  #ifdef HAVE_FIREWALLD
>>      if (!(sysbus = virDBusGetSystemBus())) {
>> -        virErrorPtr err = virGetLastError();
>>          VIR_WARN("DBus not available, disabling firewalld support "
>> -                 "in bridge_network_driver: %s", err->message);
>> +                 "in bridge_network_driver: %s", virGetLastErrorMessage());
>>      } else {
>>          /* add matches for
>>           * NameOwnerChanged on org.freedesktop.DBus for firewalld start/stop
>> diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
>> index 6d18a87..6ddfad0 100644
>> --- a/src/node_device/node_device_hal.c
>> +++ b/src/node_device/node_device_hal.c
>> @@ -641,9 +641,8 @@ nodeStateInitialize(bool privileged ATTRIBUTE_UNUSED,
>>  
>>      dbus_error_init(&err);
>>      if (!(sysbus = virDBusGetSystemBus())) {
>> -        virErrorPtr verr = virGetLastError();
>>          VIR_ERROR(_("DBus not available, disabling HAL driver: %s"),
>> -                    verr->message);
>> +                    virGetLastErrorMessage());
>>          ret = 0;
>>          goto failure;
>>      }
>>
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]