On Wed, Nov 12, 2014 at 1:27 PM, John Ferlan 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
>> ---
>> 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 = qemuMonitorG