[libvirt] [PATCH 0/3] Resolve some recently uncovered Coverity warnings
Perhaps due to other related changes in the code paths, my daily Coverity scan uncovered a few new issues last week. It didn't seem any of the issues found were critical path, so I waited for the 1.0.3 before posting. I am running Coverity on a daily basis and trying to fine tune the "blame" algorithm. Unfortunately when changes are made in sometimes related code paths it allows Coverity to scan further thus sometimes uncovering some paths that hadn't been covered before or had been blocked by other issues Coverity uncovered in the path. I have a dialogue going with someone at Coverity regarding these types of discoveries. John Ferlan (3): libxl_conf: Resolve Coverity issue with call to regcomp() libxl_driver: Resolve Coverity errors sheepdog: Add Coverity filter src/libxl/libxl_conf.c | 11 ++++++++++- src/libxl/libxl_driver.c | 10 ++++++---- src/storage/storage_backend_sheepdog.c | 2 ++ 3 files changed, 18 insertions(+), 5 deletions(-) -- 1.8.1.2
--- src/libxl/libxl_conf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 4ce5dec..b208dd8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -769,10 +769,19 @@ error: virCapsPtr libxlMakeCapabilities(libxl_ctx *ctx) { + int err; libxl_physinfo phy_info; const libxl_version_info *ver_info; - regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED); + err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED); + if (err != 0) { + char error[100]; + regerror(err, &xen_cap_rec, error, sizeof(error)); + regfree(&xen_cap_rec); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to compile regex %s"), error); + return NULL; + } if (libxl_get_physinfo(ctx, &phy_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.8.1.2
On 03/05/2013 05:43 AM, John Ferlan wrote:
--- src/libxl/libxl_conf.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
1. The virObjectLock() call was unconditional, but Unlock was conditional on vm being valid. Removed the check 2. A call to virDomainEventNewFromObj() isn't guaranteed to return an event - that check needs to be made prior to libxlDomainEventQueue() of the event. Did not add libxlDriverLock/Unlock around the call since some callers already have lock taken 3. Need to initialize fd = -1 in libxlDoDomainSave() since we can jump to cleanup before it's set. 4. Missing break;'s in libxlDomainModifyDeviceFlags() for case LIBXL_DEVICE_UPDATE. The default: case would report an error --- src/libxl/libxl_driver.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 22bd245..35c9e16 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -455,8 +455,7 @@ libxlAutostartDomain(virDomainObjPtr vm, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return ret; } @@ -983,7 +982,8 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, restore_fd < 0 ? VIR_DOMAIN_EVENT_STARTED_BOOTED : VIR_DOMAIN_EVENT_STARTED_RESTORED); - libxlDomainEventQueue(driver, event); + if (event) + libxlDomainEventQueue(driver, event); libxl_domain_config_dispose(&d_config); VIR_FREE(dom_xml); @@ -2085,7 +2085,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainEventPtr event = NULL; char *xml = NULL; uint32_t xml_len; - int fd; + int fd = -1; int ret = -1; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { @@ -3561,6 +3561,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, break; case LIBXL_DEVICE_UPDATE: ret = libxlDomainUpdateDeviceConfig(vmdef, dev); + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown domain modify action %d"), action); @@ -3585,6 +3586,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, break; case LIBXL_DEVICE_UPDATE: ret = libxlDomainUpdateDeviceLive(priv, vm, dev); + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown domain modify action %d"), action); -- 1.8.1.2
On 03/05/2013 05:43 AM, John Ferlan wrote:
1. The virObjectLock() call was unconditional, but Unlock was conditional on vm being valid. Removed the check
2. A call to virDomainEventNewFromObj() isn't guaranteed to return an event - that check needs to be made prior to libxlDomainEventQueue() of the event. Did not add libxlDriverLock/Unlock around the call since some callers already have lock taken
Someday, we should remove the big libxl lock in the same way that danpb just removed the big qemu lock; but that's an independent project.
3. Need to initialize fd = -1 in libxlDoDomainSave() since we can jump to cleanup before it's set.
4. Missing break;'s in libxlDomainModifyDeviceFlags() for case LIBXL_DEVICE_UPDATE. The default: case would report an error
More precisely, the device update fell through to the default case and reported an error even if it had succeeded. At any rate, all of these fixes look correct. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/05/2013 05:43 AM, John Ferlan wrote:
1. The virObjectLock() call was unconditional, but Unlock was conditional on vm being valid. Removed the check
2. A call to virDomainEventNewFromObj() isn't guaranteed to return an event - that check needs to be made prior to libxlDomainEventQueue() of the event. Did not add libxlDriverLock/Unlock around the call since some callers already have lock taken
Someday, we should remove the big libxl lock in the same way that danpb just removed the big qemu lock; but that's an independent project.
Agreed. I added that to the libxl todo list when I saw his excellent work. Jim
Add a Coverity dead_error_condition tag since it's not possible for next to be NULL at that point and thus exit the while loop. If next were NULL the condition to set it would have done the return -1. --- src/storage/storage_backend_sheepdog.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 218284d..7b564bd 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -85,6 +85,8 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool, pool->available = pool->capacity - pool->allocation; return 0; + /* coverity[dead_error_condition] - since we return -1 above we'll + * never get to return -1 below */ } while ((p = next)); return -1; -- 1.8.1.2
On 03/05/2013 05:43 AM, John Ferlan wrote:
Add a Coverity dead_error_condition tag since it's not possible for next to be NULL at that point and thus exit the while loop. If next were NULL the condition to set it would have done the return -1. --- src/storage/storage_backend_sheepdog.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index 218284d..7b564bd 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -85,6 +85,8 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr pool, pool->available = pool->capacity - pool->allocation; return 0;
+ /* coverity[dead_error_condition] - since we return -1 above we'll + * never get to return -1 below */ } while ((p = next));
Instead of adding this fancy comment, why not just change the earlier 'return -1;' lines into 'break;'. Then the end of the function would be reachable, we'd no longer have dead code, we don't need a coverity-specific comment, and other code analyzers will not need similar tricks at a later date. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/05/2013 07:43 AM, John Ferlan wrote:
src/libxl/libxl_conf.c | 11 ++++++++++- src/libxl/libxl_driver.c | 10 ++++++---- src/storage/storage_backend_sheepdog.c | 2 ++ 3 files changed, 18 insertions(+), 5 deletions(-)
Changed sheepdog to use break and removed Coverity comments (now where was my brain on that). pushed the changes John
participants (3)
-
Eric Blake -
Jim Fehlig -
John Ferlan