[libvirt] [PATCH 2/2] qemu: Fix DEATHCODE error in qemuDomainGetBlockIoTune.

[Matthias Gatto]

On Wed, Nov 12, 2014 at 1:27 PM, John Ferlan <jferlan at redhat.com> 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 at outscale.com>
>> ---
>>  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.