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