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

Cole Robinson crobinso at redhat.com
Sat May 7 15:46:48 UTC 2016


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.
> 

Prior to this patch, virGetLastError returning NULL would lead to a NULL
dereference. So using virGetLastErrorMessage() here is a clear win, and is
exactly what the API is meant to handle.

_if_ the original code had checked for err != NULL before trying to print
err->message, then indeed the behavior would change a bit. That said I think
converting to virGetLastErrorMessage() is still a win, and we should just
improve/clarify virGetLastErrorMessage() separately.

ACK to this patch.

- Cole

> 
>> Signed-off-by: John Ferlan <jferlan at 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;
>>      }
>>
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




More information about the libvir-list mailing list