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

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



On Thu, May 21, 2015 at 14:18:44 +0200, Jiri Denemark wrote:
> 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 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.
> 
> 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.

Done and pushed, thanks.

Jirka


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