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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list