[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] build: add compiler attributes to virUUIDParse



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]