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

Re: [libvirt] [PATCH v7 4/7] qemu: Add bps_max and friends qemu driver




On 11/10/2014 10:19 AM, Matthias Gatto wrote:
> Add support for bps_max and friends in the driver part.
> In the part checking if a qemu is running, check if the running binary
> support bps_max, if not print an error message, if yes add it to
> "info" variable
> 
> This patch follow therse patchs: http://www.redhat.com/archives/libvir-list/2014-November/msg00271.html
> I've fix the syntax error and the nparams detection problem
> 
> Signed-off-by: Matthias Gatto <matthias gatto outscale com>
> ---
>  include/libvirt/libvirt-domain.h |  56 +++++++++++
>  src/qemu/qemu_driver.c           | 197 ++++++++++++++++++++++++++++++++++++---
>  src/qemu/qemu_monitor.c          |  10 +-
>  src/qemu/qemu_monitor.h          |   6 +-
>  src/qemu/qemu_monitor_json.c     |  11 ++-
>  src/qemu/qemu_monitor_json.h     |   6 +-
>  tests/qemumonitorjsontest.c      |   4 +-
>  7 files changed, 264 insertions(+), 26 deletions(-)
> 

Coverity is unhappy here too - DEADCODE...

<...snip...>

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5eccacc..242b30e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c

<...snip...>

> @@ -16753,13 +16870,14 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>  {
>      virQEMUDriverPtr driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
> -    qemuDomainObjPrivatePtr priv;
> +    qemuDomainObjPrivatePtr priv = NULL;
>      virDomainDefPtr persistentDef = NULL;
>      virDomainBlockIoTuneInfo reply;
>      char *device = NULL;
>      int ret = -1;
>      size_t i;
>      virCapsPtr caps = NULL;

(1) Event assignment: 	Assigning: "supportMaxOptions" = "true".
Also see events: 

> +    bool supportMaxOptions = true;
>  
>      virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>                    VIR_DOMAIN_AFFECT_CONFIG |
> @@ -16777,13 +16895,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>      if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>          goto cleanup;
>  
> -    if ((*nparams) == 0) {
> -        /* Current number of parameters supported by QEMU Block I/O Throttling */
> -        *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
> -        ret = 0;
> -        goto cleanup;
> -    }
> -
>      if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>          goto cleanup;
>  
> @@ -16791,6 +16902,18 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>                                          &persistentDef) < 0)
>          goto endjob;
>  
> +    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;
> +        goto endjob;

^^^^
The only way supportMaxOptions can change (be false) is in this block; 
however, the "goto endjob;" here jumps around the later check regarding
where Coverity complains about DEADCODE

Less sure how to handle in this case, but my gut says the (!supportMaxOptions &&
check below is unnecessary.  

THoughts?

John
> +    }
> +
>      device = qemuDiskPathToAlias(vm, disk, NULL);
>      if (!device) {
>          goto endjob;
> @@ -16799,7 +16922,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>          priv = vm->privateData;
>          qemuDomainObjEnterMonitor(driver, vm);
> -        ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply);
> +        ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions);
>          qemuDomainObjExitMonitor(driver, vm);
>          if (ret < 0)
>              goto endjob;
> @@ -16816,7 +16939,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>          reply = persistentDef->disks[idx]->blkdeviotune;
>      }
>  
> -    for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM && i < *nparams; i++) {
> +    for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX && i < *nparams; i++) {
>          virTypedParameterPtr param = &params[i];
>  
>          switch (i) {
> @@ -16862,14 +16985,64 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>                                          reply.write_iops_sec) < 0)
>                  goto endjob;
>              break;
> +        case 6:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.total_bytes_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 7:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.read_bytes_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 8:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.write_bytes_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 9:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.total_iops_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 10:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.read_iops_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 11:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.write_iops_sec_max) < 0)
> +                goto endjob;
> +            break;
> +        case 12:
> +            if (virTypedParameterAssign(param,
> +                                        VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC,
> +                                        VIR_TYPED_PARAM_ULLONG,
> +                                        reply.size_iops_sec) < 0)
> +                goto endjob;
>          /* coverity[dead_error_begin] */
>          default:
>              break;
>          }
>      }
>  

17144	

(2) Event const: 	At condition "supportMaxOptions", the value of "supportMaxOptions" must be equal to 1.
(3) Event dead_error_condition: 	The condition "!supportMaxOptions" cannot be true.
(4) Event dead_error_line: 	Execution cannot reach the expression "*nparams > 6" inside this statement: "if (!supportMaxOptions && *...".
Also see events: 	[assignment]

17145	    if (!supportMaxOptions && *nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM)


> -    if (*nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM)
> +    if (!supportMaxOptions && *nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM)
>          *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
> +    else if (*nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX)
> +        *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX;
>      ret = 0;
>  
>   endjob:
<...snip...>




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