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

On Thu, May 21, 2015 at 07:42:53 -0400, John Ferlan wrote: > > > 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(a)redhat.com&gt; > > --- > > 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. Oops, I fixed two similar issues and didn't notice this third one. > 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. Great, I'll remove both.