[libvirt] [PATCH] build: add compiler attributes to virUUIDParse
Coverity complained that most, but not all, clients of virUUIDParse were checking for errors. Silence those coverity warnings by explicitly marking the cases where we trust the input, and fixing one instance that really should have been checking. In particular, this silences about half of the 46 warnings I saw on my most recent Coverity analysis run. * src/util/uuid.h (virUUIDParse): Enforce rules. * src/util/uuid.c (virUUIDParse): Drop impossible check; at least Coverity will detect if we break rules and pass NULL. * src/xenapi/xenapi_driver.c (xenapiDomainCreateXML) (xenapiDomainLookupByID, xenapiDomainLookupByName) (xenapiDomainDefineXML): Ignore return when we trust data source. * src/vbox/vbox_tmpl.c (nsIDtoChar, vboxIIDToUUID_v3_x) (vboxCallbackOnMachineStateChange) (vboxCallbackOnMachineRegistered, vboxStoragePoolLookupByName): Likewise. * src/node_device/node_device_hal.c (gather_system_cap): Likewise. * src/xenxs/xen_sxpr.c (xenParseSxpr): Check for errors. --- This one's big enough that I'll wait for ACK before pushing. src/node_device/node_device_hal.c | 3 ++- src/util/uuid.c | 5 +---- src/util/uuid.h | 3 ++- src/vbox/vbox_tmpl.c | 12 ++++++------ src/xenapi/xenapi_driver.c | 11 ++++++----- src/xenxs/xen_sxpr.c | 3 ++- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 481be97..a028886 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -38,6 +38,7 @@ #include "pci.h" #include "logging.h" #include "node_device_driver.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_NODEDEV @@ -299,7 +300,7 @@ static int gather_system_cap(LibHalContext *ctx, const char *udi, (void)get_str_prop(ctx, udi, "system.hardware.serial", &d->system.hardware.serial); if (get_str_prop(ctx, udi, "system.hardware.uuid", &uuidstr) == 0) { - (void)virUUIDParse(uuidstr, d->system.hardware.uuid); + ignore_value(virUUIDParse(uuidstr, d->system.hardware.uuid)); VIR_FREE(uuidstr); } (void)get_str_prop(ctx, udi, "system.firmware.vendor", diff --git a/src/util/uuid.c b/src/util/uuid.c index b4317df..0df3ebc 100644 --- a/src/util/uuid.c +++ b/src/util/uuid.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007, 2008, 2009, 2010 Red Hat, Inc. + * Copyright (C) 2007-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { const char *cur; int i; - if ((uuidstr == NULL) || (uuid == NULL)) - return(-1); - /* * do a liberal scan allowing '-' and ' ' anywhere between character * pairs, and surrounding whitespace, as long as there are exactly diff --git a/src/util/uuid.h b/src/util/uuid.h index b5d7878..7dbfad5 100644 --- a/src/util/uuid.h +++ b/src/util/uuid.h @@ -32,7 +32,8 @@ int virUUIDIsValid(unsigned char *uuid); int virUUIDGenerate(unsigned char *uuid); int virUUIDParse(const char *uuidstr, - unsigned char *uuid); + unsigned char *uuid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; void virUUIDFormat(const unsigned char *uuid, char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9b674a9..bc19b63 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -300,7 +300,7 @@ static void nsIDtoChar(unsigned char *uuid, const nsID *iid) { } uuidstrdst[VIR_UUID_STRING_BUFLEN-1] = '\0'; - virUUIDParse(uuidstrdst, uuid); + ignore_value(virUUIDParse(uuidstrdst, uuid)); } static void nsIDFromChar(nsID *iid, const unsigned char *uuid) { @@ -339,7 +339,7 @@ static void nsIDFromChar(nsID *iid, const unsigned char *uuid) { } uuidstrdst[VIR_UUID_STRING_BUFLEN-1] = '\0'; - virUUIDParse(uuidstrdst, uuidinterim); + ignore_value(virUUIDParse(uuidstrdst, uuidinterim)); memcpy(iid, uuidinterim, VIR_UUID_BUFLEN); } @@ -511,7 +511,7 @@ vboxIIDToUUID_v3_x(vboxGlobalData *data, vboxIID_v3_x *iid, data->pFuncs->pfnUtf16ToUtf8(iid->value, &utf8); - virUUIDParse(utf8, uuid); + ignore_value(virUUIDParse(utf8, uuid)); data->pFuncs->pfnUtf8Free(utf8); } @@ -6558,7 +6558,7 @@ vboxCallbackOnMachineStateChange(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, unsigned char uuid[VIR_UUID_BUFLEN]; g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); - virUUIDParse(machineIdUtf8, uuid); + ignore_value(virUUIDParse(machineIdUtf8, uuid)); dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); if (dom) { @@ -6686,7 +6686,7 @@ vboxCallbackOnMachineRegistered(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, unsigned char uuid[VIR_UUID_BUFLEN]; g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); - virUUIDParse(machineIdUtf8, uuid); + ignore_value(virUUIDParse(machineIdUtf8, uuid)); dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); if (dom) { @@ -7983,7 +7983,7 @@ static virStoragePoolPtr vboxStoragePoolLookupByName(virConnectPtr conn, const c unsigned char uuid[VIR_UUID_BUFLEN]; const char *uuidstr = "1deff1ff-1481-464f-967f-a50fe8936cc4"; - virUUIDParse(uuidstr, uuid); + ignore_value(virUUIDParse(uuidstr, uuid)); ret = virGetStoragePool(conn, name, uuid); } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 80a706a..3946455 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -40,6 +40,7 @@ #include "xenapi_driver.h" #include "xenapi_driver_private.h" #include "xenapi_utils.h" +#include "ignore-value.h" #define VIR_FROM_THIS VIR_FROM_XENAPI @@ -528,7 +529,7 @@ xenapiDomainCreateXML (virConnectPtr conn, virDomainDefFree(defPtr); if (record) { unsigned char raw_uuid[VIR_UUID_BUFLEN]; - virUUIDParse(record->uuid, raw_uuid); + ignore_value(virUUIDParse(record->uuid, raw_uuid)); if (vm) { if (xen_vm_start(session, vm, false, false)) { domP = virGetDomain(conn, record->name_label, raw_uuid); @@ -574,13 +575,13 @@ xenapiDomainLookupByID (virConnectPtr conn, int id) xen_session_get_this_host(session, &host, session); if (host != NULL && session->ok) { xen_host_get_resident_vms(session, &result, host); - if (result != NULL ) { + if (result != NULL) { for (i = 0; i < result->size; i++) { xen_vm_get_domid(session, &domID, result->contents[i]); if (domID == id) { xen_vm_get_record(session, &record, result->contents[i]); xen_vm_get_uuid(session, &uuid, result->contents[i]); - virUUIDParse(uuid, raw_uuid); + ignore_value(virUUIDParse(uuid, raw_uuid)); domP = virGetDomain(conn, record->name_label, raw_uuid); if (domP) { int64_t domid = -1; @@ -672,7 +673,7 @@ xenapiDomainLookupByName (virConnectPtr conn, vm = vms->contents[0]; xen_vm_get_uuid(session, &uuid, vm); if (uuid!=NULL) { - virUUIDParse(uuid, raw_uuid); + ignore_value(virUUIDParse(uuid, raw_uuid)); domP = virGetDomain(conn, name, raw_uuid); if (domP != NULL) { int64_t domid = -1; @@ -1683,7 +1684,7 @@ xenapiDomainDefineXML (virConnectPtr conn, const char *xml) } if (record != NULL) { unsigned char raw_uuid[VIR_UUID_BUFLEN]; - virUUIDParse(record->uuid, raw_uuid); + ignore_value(virUUIDParse(record->uuid, raw_uuid)); domP = virGetDomain(conn, record->name_label, raw_uuid); if (!domP && !session->ok) xenapiSessionErrorHandler(conn, VIR_ERR_NO_DOMAIN, NULL); diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index f2447f7..d44b0dc 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1094,7 +1094,8 @@ xenParseSxpr(const struct sexpr *root, "%s", _("domain information incomplete, missing name")); goto error; } - virUUIDParse(tmp, def->uuid); + if (virUUIDParse(tmp, def->uuid) < 0) + goto error; if (sexpr_node_copy(root, "domain/description", &def->description) < 0) goto no_memory; -- 1.7.4.4
On Wed, Oct 12, 2011 at 05:27:40PM -0600, Eric Blake wrote:
Coverity complained that most, but not all, clients of virUUIDParse were checking for errors. Silence those coverity warnings by explicitly marking the cases where we trust the input, and fixing one instance that really should have been checking. In particular, this silences about half of the 46 warnings I saw on my most recent Coverity analysis run.
* src/util/uuid.h (virUUIDParse): Enforce rules. * src/util/uuid.c (virUUIDParse): Drop impossible check; at least Coverity will detect if we break rules and pass NULL. * src/xenapi/xenapi_driver.c (xenapiDomainCreateXML) (xenapiDomainLookupByID, xenapiDomainLookupByName) (xenapiDomainDefineXML): Ignore return when we trust data source. * src/vbox/vbox_tmpl.c (nsIDtoChar, vboxIIDToUUID_v3_x) (vboxCallbackOnMachineStateChange) (vboxCallbackOnMachineRegistered, vboxStoragePoolLookupByName): Likewise. * src/node_device/node_device_hal.c (gather_system_cap): Likewise. * src/xenxs/xen_sxpr.c (xenParseSxpr): Check for errors. ---
This one's big enough that I'll wait for ACK before pushing.
src/node_device/node_device_hal.c | 3 ++- src/util/uuid.c | 5 +---- src/util/uuid.h | 3 ++- src/vbox/vbox_tmpl.c | 12 ++++++------ src/xenapi/xenapi_driver.c | 11 ++++++----- src/xenxs/xen_sxpr.c | 3 ++- 6 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 481be97..a028886 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -38,6 +38,7 @@ #include "pci.h" #include "logging.h" #include "node_device_driver.h" +#include "ignore-value.h"
#define VIR_FROM_THIS VIR_FROM_NODEDEV
@@ -299,7 +300,7 @@ static int gather_system_cap(LibHalContext *ctx, const char *udi, (void)get_str_prop(ctx, udi, "system.hardware.serial", &d->system.hardware.serial); if (get_str_prop(ctx, udi, "system.hardware.uuid", &uuidstr) == 0) { - (void)virUUIDParse(uuidstr, d->system.hardware.uuid); + ignore_value(virUUIDParse(uuidstr, d->system.hardware.uuid)); VIR_FREE(uuidstr); } (void)get_str_prop(ctx, udi, "system.firmware.vendor", diff --git a/src/util/uuid.c b/src/util/uuid.c index b4317df..0df3ebc 100644 --- a/src/util/uuid.c +++ b/src/util/uuid.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007, 2008, 2009, 2010 Red Hat, Inc. + * Copyright (C) 2007-2011 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { const char *cur; int i;
- if ((uuidstr == NULL) || (uuid == NULL)) - return(-1); -
ACK, but I'm always a bit distressed at the idea that a perfectly valid runtime check is being replaced by a static one which is compiler specific. Can we keep this chunk without Coverity complaining ?
/* * do a liberal scan allowing '-' and ' ' anywhere between character * pairs, and surrounding whitespace, as long as there are exactly diff --git a/src/util/uuid.h b/src/util/uuid.h index b5d7878..7dbfad5 100644 --- a/src/util/uuid.h +++ b/src/util/uuid.h @@ -32,7 +32,8 @@ int virUUIDIsValid(unsigned char *uuid); int virUUIDGenerate(unsigned char *uuid);
int virUUIDParse(const char *uuidstr, - unsigned char *uuid); + unsigned char *uuid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
void virUUIDFormat(const unsigned char *uuid, char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9b674a9..bc19b63 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -300,7 +300,7 @@ static void nsIDtoChar(unsigned char *uuid, const nsID *iid) { }
uuidstrdst[VIR_UUID_STRING_BUFLEN-1] = '\0'; - virUUIDParse(uuidstrdst, uuid); + ignore_value(virUUIDParse(uuidstrdst, uuid)); }
static void nsIDFromChar(nsID *iid, const unsigned char *uuid) { @@ -339,7 +339,7 @@ static void nsIDFromChar(nsID *iid, const unsigned char *uuid) { }
uuidstrdst[VIR_UUID_STRING_BUFLEN-1] = '\0'; - virUUIDParse(uuidstrdst, uuidinterim); + ignore_value(virUUIDParse(uuidstrdst, uuidinterim)); memcpy(iid, uuidinterim, VIR_UUID_BUFLEN); }
@@ -511,7 +511,7 @@ vboxIIDToUUID_v3_x(vboxGlobalData *data, vboxIID_v3_x *iid,
data->pFuncs->pfnUtf16ToUtf8(iid->value, &utf8);
- virUUIDParse(utf8, uuid); + ignore_value(virUUIDParse(utf8, uuid));
data->pFuncs->pfnUtf8Free(utf8); } @@ -6558,7 +6558,7 @@ vboxCallbackOnMachineStateChange(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, unsigned char uuid[VIR_UUID_BUFLEN];
g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); - virUUIDParse(machineIdUtf8, uuid); + ignore_value(virUUIDParse(machineIdUtf8, uuid));
dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); if (dom) { @@ -6686,7 +6686,7 @@ vboxCallbackOnMachineRegistered(IVirtualBoxCallback *pThis ATTRIBUTE_UNUSED, unsigned char uuid[VIR_UUID_BUFLEN];
g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); - virUUIDParse(machineIdUtf8, uuid); + ignore_value(virUUIDParse(machineIdUtf8, uuid));
dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); if (dom) { @@ -7983,7 +7983,7 @@ static virStoragePoolPtr vboxStoragePoolLookupByName(virConnectPtr conn, const c unsigned char uuid[VIR_UUID_BUFLEN]; const char *uuidstr = "1deff1ff-1481-464f-967f-a50fe8936cc4";
- virUUIDParse(uuidstr, uuid); + ignore_value(virUUIDParse(uuidstr, uuid));
ret = virGetStoragePool(conn, name, uuid); } diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 80a706a..3946455 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -40,6 +40,7 @@ #include "xenapi_driver.h" #include "xenapi_driver_private.h" #include "xenapi_utils.h" +#include "ignore-value.h"
#define VIR_FROM_THIS VIR_FROM_XENAPI
@@ -528,7 +529,7 @@ xenapiDomainCreateXML (virConnectPtr conn, virDomainDefFree(defPtr); if (record) { unsigned char raw_uuid[VIR_UUID_BUFLEN]; - virUUIDParse(record->uuid, raw_uuid); + ignore_value(virUUIDParse(record->uuid, raw_uuid)); if (vm) { if (xen_vm_start(session, vm, false, false)) { domP = virGetDomain(conn, record->name_label, raw_uuid); @@ -574,13 +575,13 @@ xenapiDomainLookupByID (virConnectPtr conn, int id) xen_session_get_this_host(session, &host, session); if (host != NULL && session->ok) { xen_host_get_resident_vms(session, &result, host); - if (result != NULL ) { + if (result != NULL) { for (i = 0; i < result->size; i++) { xen_vm_get_domid(session, &domID, result->contents[i]); if (domID == id) { xen_vm_get_record(session, &record, result->contents[i]); xen_vm_get_uuid(session, &uuid, result->contents[i]); - virUUIDParse(uuid, raw_uuid); + ignore_value(virUUIDParse(uuid, raw_uuid)); domP = virGetDomain(conn, record->name_label, raw_uuid); if (domP) { int64_t domid = -1; @@ -672,7 +673,7 @@ xenapiDomainLookupByName (virConnectPtr conn, vm = vms->contents[0]; xen_vm_get_uuid(session, &uuid, vm); if (uuid!=NULL) { - virUUIDParse(uuid, raw_uuid); + ignore_value(virUUIDParse(uuid, raw_uuid)); domP = virGetDomain(conn, name, raw_uuid); if (domP != NULL) { int64_t domid = -1; @@ -1683,7 +1684,7 @@ xenapiDomainDefineXML (virConnectPtr conn, const char *xml) } if (record != NULL) { unsigned char raw_uuid[VIR_UUID_BUFLEN]; - virUUIDParse(record->uuid, raw_uuid); + ignore_value(virUUIDParse(record->uuid, raw_uuid)); domP = virGetDomain(conn, record->name_label, raw_uuid); if (!domP && !session->ok) xenapiSessionErrorHandler(conn, VIR_ERR_NO_DOMAIN, NULL); diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index f2447f7..d44b0dc 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1094,7 +1094,8 @@ xenParseSxpr(const struct sexpr *root, "%s", _("domain information incomplete, missing name")); goto error; } - virUUIDParse(tmp, def->uuid); + if (virUUIDParse(tmp, def->uuid) < 0) + goto error;
if (sexpr_node_copy(root, "domain/description", &def->description) < 0) goto no_memory;
But fine, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 10/13/2011 02:55 AM, Daniel Veillard wrote:
On Wed, Oct 12, 2011 at 05:27:40PM -0600, Eric Blake wrote:
Coverity complained that most, but not all, clients of virUUIDParse were checking for errors. Silence those coverity warnings by explicitly marking the cases where we trust the input, and fixing one instance that really should have been checking. In particular, this silences about half of the 46 warnings I saw on my most recent Coverity analysis run.
@@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { const char *cur; int i;
- if ((uuidstr == NULL) || (uuid == NULL)) - return(-1); -
ACK, but I'm always a bit distressed at the idea that a perfectly valid runtime check is being replaced by a static one which is compiler specific. Can we keep this chunk without Coverity complaining ?
Unfortunately, no. Keeping this code will make Coverity warn about dead code (the ATTRIBUTE_NONNULL implies that the check for NULL will always fail). All callers were already passing valid pointers; directly passing NULL will make gcc warn, and there's an open BZ against gcc to make it someday do the same analysis as Coverity to warn about any other obvious passing of NULL. But in spite of gcc's weakness, both clang and coverity catch a lot more cases of passing unintentional NULL, and get run frequently enough that I'm not too worried about dropping the argument check (it's an internal function, so we should be calling it correctly in the first place).
But fine, ACK
I've gone ahead and pushed this. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 13, 2011 at 12:24:02PM -0600, Eric Blake wrote:
On 10/13/2011 02:55 AM, Daniel Veillard wrote:
On Wed, Oct 12, 2011 at 05:27:40PM -0600, Eric Blake wrote:
@@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { const char *cur; int i;
- if ((uuidstr == NULL) || (uuid == NULL)) - return(-1); -
ACK, but I'm always a bit distressed at the idea that a perfectly valid runtime check is being replaced by a static one which is compiler specific. Can we keep this chunk without Coverity complaining ?
Unfortunately, no. Keeping this code will make Coverity warn about dead code (the ATTRIBUTE_NONNULL implies that the check for NULL will always fail). All callers were already passing valid pointers; directly passing NULL will make gcc warn, and there's an open BZ against gcc to make it someday do the same analysis as Coverity to warn about any other obvious passing of NULL. But in spite of gcc's weakness, both clang and coverity catch a lot more cases of passing unintentional NULL, and get run frequently enough that I'm not too worried about dropping the argument check (it's an internal function, so we should be calling it correctly in the first place).
Okay an example scenario I have in mind is someone developping a driver for a OS where gcc is not the compiler (say ppc64 lpar for AIX and using xlc as a example) then even if the code is internal it would never be compiled with gcc, and one would require Coverity to actually find the problem, internal function or not. To be honnest I dislike the idea that we must rely on a proprietary code in the future to make sure the code is correct, and even if gcc was adding some support for this, this is still not an absolute guarantee... Still the ACK stands, but I prefer to not completely generalize a static only check approach to our code base, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Oct 13, 2011 at 12:24:02PM -0600, Eric Blake wrote:
On 10/13/2011 02:55 AM, Daniel Veillard wrote:
On Wed, Oct 12, 2011 at 05:27:40PM -0600, Eric Blake wrote:
Coverity complained that most, but not all, clients of virUUIDParse were checking for errors. Silence those coverity warnings by explicitly marking the cases where we trust the input, and fixing one instance that really should have been checking. In particular, this silences about half of the 46 warnings I saw on my most recent Coverity analysis run.
@@ -129,9 +129,6 @@ virUUIDParse(const char *uuidstr, unsigned char *uuid) { const char *cur; int i;
- if ((uuidstr == NULL) || (uuid == NULL)) - return(-1); -
ACK, but I'm always a bit distressed at the idea that a perfectly valid runtime check is being replaced by a static one which is compiler specific. Can we keep this chunk without Coverity complaining ?
Unfortunately, no. Keeping this code will make Coverity warn about dead code (the ATTRIBUTE_NONNULL implies that the check for NULL will always fail). All callers were already passing valid pointers; directly passing NULL will make gcc warn, and there's an open BZ against gcc to make it someday do the same analysis as Coverity to warn about any other obvious passing of NULL. But in spite of gcc's weakness, both clang and coverity catch a lot more cases of passing unintentional NULL, and get run frequently enough that I'm not too worried about dropping the argument check (it's an internal function, so we should be calling it correctly in the first place).
Also note that if you have ATTRIBUTE_NONNULL, then GCC will actually *remove* those two lines during optimization phase anyway. So by leaving them you are giving yourself a misleading view of reality. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake