[libvirt] [PATCH 2/2] Use virDomainDiskByName where appropriate

[John Ferlan]

On 05/21/2015 05:42 AM, Jiri Denemark wrote:
> Most virDomainDiskIndexByName callers do not care about the index; what
> they really want is a disk def pointer.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/libxl/libxl_driver.c  |  4 +--
>  src/lxc/lxc_driver.c      | 10 +++-----
>  src/qemu/qemu_agent.c     |  9 ++++---
>  src/qemu/qemu_blockjob.c  |  5 ++--
>  src/qemu/qemu_driver.c    | 62 ++++++++++++++++++-----------------------------
>  src/qemu/qemu_migration.c |  6 ++---
>  src/qemu/qemu_process.c   | 23 ++++--------------
>  7 files changed, 42 insertions(+), 77 deletions(-)
> 

Since Jan asked - I ran this against Coverity...
...

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 31cc2ee..2ac72a4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8559,13 +8559,11 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
>      switch ((virDomainDeviceType) dev->type) {
>      case VIR_DOMAIN_DEVICE_DISK:
>          disk = dev->data.disk;
> -        pos = virDomainDiskIndexByName(vmdef, disk->dst, false);
> -        if (pos < 0) {
> +        if (!(orig = virDomainDiskByName(vmdef, disk->dst, false))) {
>              virReportError(VIR_ERR_INVALID_ARG,
>                             _("target %s doesn't exist."), disk->dst);
>              return -1;
>          }
> -        orig = vmdef->disks[pos];
>          if (!(orig->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
>              !(orig->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)) {
>              virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -11163,7 +11161,7 @@ qemuDomainBlockResize(virDomainPtr dom,
>      virQEMUDriverPtr driver = dom->conn->privateData;
>      virDomainObjPtr vm;
>      qemuDomainObjPrivatePtr priv;
> -    int ret = -1, idx;
> +    int ret = -1;
>      char *device = NULL;
>      virDomainDiskDefPtr disk = NULL;
>  
> @@ -11203,12 +11201,11 @@ qemuDomainBlockResize(virDomainPtr dom,
>          goto endjob;
>      }
>  
> -    if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
> +    if (!(disk = virDomainDiskByName(vm->def, path, false)) < 0) {


(1) Event result_independent_of_operands: 	"!(disk =
virDomainDiskByName(vm->def, path, false /* 0 */)) < 0" is always false
regardless of the values of its operands. This occurs as the logical
operand of if.


That led to a second complaint which won't happen if the " < 0" is removed.

>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("invalid path: %s"), path);
>          goto endjob;
>      }
> -    disk = vm->def->disks[idx];
>  
>      /* qcow2 and qed must be sized on 512 byte blocks/sectors,
>       * so adjust size if necessary to round up.

second issue was here since disk could be accessed...

And yes, the sa_assert(persistent_def) that Jan asked about can be
safely removed. Coverity also doesn't complain if the
sa_assert(conf_disk) is removed. I'll have to check if a few of those
previous false positives just like this could be removed... Although for
this case it could be because conf_disk is populated and accessed within
the same function for the same condition, so it's much easier for
Coverity to track.

John