[libvirt] [PATCHv2 07/14] Fix vmdef usage while in monitor in BlockStat* APIs

[John Ferlan]

<No commit message>?

Seems we're fixing two issues here

#1. The ExitMonitor issue

#2. Somehow the disk->info.alias becomes NULL and thus we're making a
local copy to avoid that.

On 01/07/2015 10:42 AM, Ján Tomko wrote:
> ---
>  src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1275ba4..30c9017 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10114,6 +10114,7 @@ qemuDomainBlockStats(virDomainPtr dom,
>      virDomainObjPtr vm;
>      virDomainDiskDefPtr disk = NULL;
>      qemuDomainObjPrivatePtr priv;
> +    char *diskAlias = NULL;
>  
>      if (!*path) {
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> @@ -10149,11 +10150,14 @@ qemuDomainBlockStats(virDomainPtr dom,
>          goto endjob;
>      }
>  
> +    if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
> +        goto endjob;
> +
>      priv = vm->privateData;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
>      ret = qemuMonitorGetBlockStatsInfo(priv->mon,
> -                                       disk->info.alias,
> +                                       diskAlias,
>                                         &stats->rd_req,
>                                         &stats->rd_bytes,
>                                         NULL,
> @@ -10163,13 +10167,15 @@ qemuDomainBlockStats(virDomainPtr dom,
>                                         NULL,
>                                         NULL,
>                                         &stats->errs);
> -    qemuDomainObjExitMonitor(driver, vm);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        ret = -1;
>  
>   endjob:
>      qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
>      qemuDomObjEndAPI(&vm);
> +    VIR_FREE(diskAlias);
>      return ret;
>  }
>  
> @@ -10185,11 +10191,11 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>      int idx;
>      int tmp, ret = -1;
>      virDomainObjPtr vm;
> -    virDomainDiskDefPtr disk = NULL;
>      qemuDomainObjPrivatePtr priv;
>      long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times;
>      long long wr_total_times, flush_req, flush_total_times, errs;
>      virTypedParameterPtr param;
> +    char *diskAlias = NULL;
>  
>      virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
>  
> @@ -10218,6 +10224,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>      }
>  
>      if (*nparams != 0) {
> +        virDomainDiskDefPtr disk = NULL;
>          if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
>              virReportError(VIR_ERR_INVALID_ARG,
>                             _("invalid path: %s"), path);
> @@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>                              disk->dst);
>               goto endjob;
>          }
> +        if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
> +            goto endjob;

so diskAlias is only set if *nparams != 0, but...

>      }
>  
>      priv = vm->privateData;
> @@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>      ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
>  
>      if (tmp == 0 || ret < 0) {
> -        qemuDomainObjExitMonitor(driver, vm);
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));

Can tmp == 0 and ret == 0?  Thus when go to endjob and eventually exit
we don't a failure?  Does it matter? I suppose for consistency it does.

>          goto endjob;
>      }
>  
>      ret = qemuMonitorGetBlockStatsInfo(priv->mon,
> -                                       disk->info.alias,
> +                                       diskAlias,

...used here regardless of *nparams...  Can we get here with diskAlias
== NULL?

>                                         &rd_req,
>                                         &rd_bytes,
>                                         &rd_total_times,
> @@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>                                         &flush_total_times,
>                                         &errs);
>  
> -    qemuDomainObjExitMonitor(driver, vm);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        ret = -1;
>  
>      if (ret < 0)
>          goto endjob;
> @@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>      qemuDomainObjEndJob(driver, vm);
>  
>   cleanup:
> +    VIR_FREE(diskAlias);
>      qemuDomObjEndAPI(&vm);
>      return ret;
>  }
> @@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>          qemuDomainObjEnterMonitor(driver, vm);
>          ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
>                                              &info, supportMaxOptions);
> -        qemuDomainObjExitMonitor(driver, vm);
> +        if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +            goto endjob;

Potentially leaving ret = 0 from the IoThrottle call... Should just set
ret = -1 (liek done in qemuDomainBlockStatsFlags above)


ACK with a commit message and the issues resolved.

John

>          if (ret < 0)
>              goto endjob;
>          vm->def->disks[idx]->blkdeviotune = info;
>