On Wed, Nov 12, 2014 at 1:27 PM, John Ferlan <jferlan@xxxxxxxxxx> wrote: > > Need to fix the commit message to something like "Resolve Coverity DEADCODE" (something I just realized for 1/2 which could read Resolve Coverity COPY_PASTE_ERROR" > > Something I can take care of when pushing. > > On 11/12/2014 08:04 AM, Matthias Gatto wrote: >> reported here: http://www.redhat.com/archives/libvir-list/2014-November/msg00327.html >> >> I could have just remove bool supportMaxOptions variable, but >> if I had do this, we could not check anymore if the nparams variable is >> superior to QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX. >> >> Signed-off-by: Matthias Gatto <matthias.gatto@xxxxxxxxxxxx> >> --- >> src/qemu/qemu_driver.c | 1 + >> 1 file changed, 1 insertion(+) >> > > While the following does work... > >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index 56e8430..61c4af6 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -17024,6 +17024,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, >> >> if (flags & VIR_DOMAIN_AFFECT_LIVE) { >> priv = vm->privateData; >> + supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); >> qemuDomainObjEnterMonitor(driver, vm); >> ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); >> qemuDomainObjExitMonitor(driver, vm); >> > > Would perhaps the following be "cleaner": > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 56e8430..5124e56 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -17003,14 +17003,15 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, > &persistentDef) < 0) > goto endjob; > > + /* If the VM is running, we can check if the current VM can use > + * optional parameters or not. We didn't made this check sooner > + * because we need the VM data to do so. */ > + priv = vm->privateData; > + if (flags & VIR_DOMAIN_AFFECT_LIVE) > + supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, > + QEMU_CAPS_DRIVE_IOTUNE_MAX); > + > if ((*nparams) == 0) { > - if (flags & VIR_DOMAIN_AFFECT_LIVE) { > - priv = vm->privateData; > - /* If the VM is running, we can check if the current VM can use > - * optional parameters or not. We didn't made this check sooner > - * because we need the VM data to do so. */ > - supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); > - } > *nparams = supportMaxOptions ? > QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX : QEMU_NB_BLOCK_IO_TUNE_PARAM; > ret = 0; > @@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, > } > > if (flags & VIR_DOMAIN_AFFECT_LIVE) { > - priv = vm->privateData; > qemuDomainObjEnterMonitor(driver, vm); > ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); > qemuDomainObjExitMonitor(driver, vm); > > > The difference being moving the setting of 'priv' to be made > regardless of flags and supportMaxOptions only once prior to > using in either place. The second sentence of your comment is > a bit confusing too - I assume the "VM data" you are referencing > is the vm obtained much earlier. Or are you trying to indicate > that perhaps one of the interceding calls is necessary? > > In any case, does the above look reasonable? > > Tks - > > John Actually it's both: i need vm->privateData; to use virQEMUCapsGet but vm->privateData need qemuDomainObjBeginJob and virDomainLiveConfigHelperMethod. Your proposal is effectively cleaner, but I don't think we need to set priv = vm->privateData if the VM is not running, so i've made this: + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If the VM is running, we can check if the current VM can use + * optional parameters or not. We didn't made this check sooner + * because we need the VM data to do so. */ + priv = vm->privateData; + supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); + } + if ((*nparams) == 0) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - priv = vm->privateData; - /* If the VM is running, we can check if the current VM can use - * optional parameters or not. We didn't made this check sooner - * because we need the VM data to do so. */ - supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX); - } *nparams = supportMaxOptions ? QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX : QEMU_NB_BLOCK_IO_TUNE_PARAM; @@ -17023,7 +17024,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - priv = vm->privateData; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); qemuDomainObjExitMonitor(driver, vm); I send the patch now. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list