2:34 p.m.
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
2:34 p.m.
New subject: [libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message
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(a)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
10:04 a.m.
New subject: [libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message
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(a)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;
}
10:46 a.m.
New subject: [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.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:05 p.m.
New subject: [libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message
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
11:35 a.m.
New subject: [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_virGetLastError...
John
> Signed-off-by: John Ferlan <jferlan(a)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:35 p.m.
New subject: [libvirt] [PATCH 2/3] admin: Clean up error path in adminServerListClients
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(a)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
9:18 a.m.
New subject: [libvirt] [PATCH 2/3] admin: Clean up error path in adminServerListClients
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(a)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
2:35 p.m.
New subject: [libvirt] [PATCH 3/3] conf: Fix error path in virNodeDevPCICapabilityParseXML
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(a)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
10:12 a.m.
New subject: [libvirt] [PATCH 3/3] conf: Fix error path in virNodeDevPCICapabilityParseXML
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(a)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
3157
days inactive
3158
days old
9 comments
3 participants
participants (3)
-
Cole Robinson -
Erik Skultety -
John Ferlan