[libvirt] [PATCH 0/3] Couple of patches to resolve recent Coverity issues
Recent changes have tripped my Coverity checker. John Ferlan (3): Use virGetLastErrorMessage to avoid Coverity message admin: Clean up error path in adminServerListClients conf: Fix error path in virNodeDevPCICapabilityParseXML daemon/admin_server.c | 3 +-- src/conf/node_device_conf.c | 5 ++++- src/network/bridge_driver.c | 3 +-- src/node_device/node_device_hal.c | 3 +-- 4 files changed, 7 insertions(+), 7 deletions(-) -- 2.5.5
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. 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; } -- 2.5.5
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@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; }
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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/05/16 17:46, Cole Robinson wrote:
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.
Well, I do know that in this case it would lead to NULL dereference, but that wasn't the point I was trying to make.
_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.
Exactly as I said at the end, we either stick to the way we do it now (be it or be it not the optimal way) or we need to do some adjustments to virterror module which, as as you pointed out specifically, would lead to some cosmetic improvements/adjustments/clarifications to virGetLastErrorMessage and as long as there is a chance of doing it one day, I'm of course okay with that. Erik
ACK to this patch.
- Cole
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_virGetLastErrorMes... 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; }
Coverity noted that in adminServerListClients if virNetServerGetClients returns a -1 into ret, then the call virObjectListFreeCount in cleanup will not be very happy. Adjust the code to skip the cleanup label and just return -1 if virNetServerGetClients fails. Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/admin_server.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 2fc4675..41f6e82 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -191,14 +191,13 @@ adminServerListClients(virNetServerPtr srv, virCheckFlags(0, -1); if ((ret = virNetServerGetClients(srv, &clts)) < 0) - goto cleanup; + return -1; if (clients) { *clients = clts; clts = NULL; } - cleanup: virObjectListFreeCount(clts, ret); return ret; } -- 2.5.5
On 06/05/16 21:35, John Ferlan wrote:
Coverity noted that in adminServerListClients if virNetServerGetClients returns a -1 into ret, then the call virObjectListFreeCount in cleanup will not be very happy.
Adjust the code to skip the cleanup label and just return -1 if virNetServerGetClients fails.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- daemon/admin_server.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 2fc4675..41f6e82 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -191,14 +191,13 @@ adminServerListClients(virNetServerPtr srv, virCheckFlags(0, -1);
if ((ret = virNetServerGetClients(srv, &clts)) < 0) - goto cleanup; + return -1;
if (clients) { *clients = clts; clts = NULL; }
- cleanup: virObjectListFreeCount(clts, ret); return ret; }
ACK Erik
If the call to virXPathNodeSet to set naddresses fails, Coverity notes that the subsequent VIR_ALLOC_N cannot have a negative value (well it probably wouldn't be negative per se). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a06e450..aed95d5 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1305,7 +1305,10 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, data->pci_dev.physical_function) < 0) goto out; } else if (STREQ(type, "virt_functions")) { - int naddresses = virXPathNodeSet("./address", ctxt, &addresses); + int naddresses; + + if ((naddresses = virXPathNodeSet("./address", ctxt, &addresses)) < 0) + goto out; if (maxFuncsStr && virStrToLong_uip(maxFuncsStr, NULL, 10, -- 2.5.5
On 06/05/16 21:35, John Ferlan wrote:
If the call to virXPathNodeSet to set naddresses fails, Coverity notes that the subsequent VIR_ALLOC_N cannot have a negative value (well it probably wouldn't be negative per se).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/node_device_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index a06e450..aed95d5 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1305,7 +1305,10 @@ virNodeDevPCICapabilityParseXML(xmlXPathContextPtr ctxt, data->pci_dev.physical_function) < 0) goto out; } else if (STREQ(type, "virt_functions")) { - int naddresses = virXPathNodeSet("./address", ctxt, &addresses); + int naddresses; + + if ((naddresses = virXPathNodeSet("./address", ctxt, &addresses)) < 0) + goto out;
if (maxFuncsStr && virStrToLong_uip(maxFuncsStr, NULL, 10,
ACK Erik
participants (3)
-
Cole Robinson -
Erik Skultety -
John Ferlan