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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list