[libvirt] [PATCH 22/34] qemu: monitor: Remove weird return values from qemuMonitorSetCPU

[John Ferlan]

On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Let the function report errors internally and change it to return
> standard return codes.
> ---
>  src/qemu/qemu_driver.c       | 22 ++++------------------
>  src/qemu/qemu_monitor_json.c |  4 ----
>  src/qemu/qemu_monitor_text.c | 22 +++++++++++-----------
>  3 files changed, 15 insertions(+), 33 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 49fdd63..9011b2d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4698,7 +4698,6 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      size_t i;
> -    int rc = 1;
>      int ret = -1;
>      int oldvcpus = virDomainDefGetVCpus(vm->def);
>      int vcpus = oldvcpus;
> @@ -4712,10 +4711,7 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
> 
>      for (i = vcpus; i < nvcpus; i++) {
>          /* Online new CPU */
> -        rc = qemuMonitorSetCPU(priv->mon, i, true);
> -        if (rc == 0)
> -            goto unsupported;
> -        if (rc < 0)
> +        if (qemuMonitorSetCPU(priv->mon, i, true) < 0)
>              goto exit_monitor;
> 
>          vcpus++;
> @@ -4795,14 +4791,11 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>      if (virDomainObjIsActive(vm) &&
>          virDomainDefSetVCpus(vm->def, vcpus) < 0)
>          ret = -1;
> -    virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
> +    virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", ret == 0);
>      if (cgroup_vcpu)
>          virCgroupFree(&cgroup_vcpu);
>      return ret;
> 
> - unsupported:
> -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                   _("cannot change vcpu count of this domain"));
>   exit_monitor:
>      ignore_value(qemuDomainObjExitMonitor(driver, vm));
>      goto cleanup;
> @@ -4816,7 +4809,6 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      size_t i;
> -    int rc = 1;
>      int ret = -1;
>      int oldvcpus = virDomainDefGetVCpus(vm->def);
>      int vcpus = oldvcpus;
> @@ -4827,10 +4819,7 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> 
>      for (i = vcpus - 1; i >= nvcpus; i--) {
>          /* Offline old CPU */
> -        rc = qemuMonitorSetCPU(priv->mon, i, false);
> -        if (rc == 0)
> -            goto unsupported;
> -        if (rc < 0)
> +        if (qemuMonitorSetCPU(priv->mon, i, false) < 0)
>              goto exit_monitor;
> 
>          vcpus--;
> @@ -4889,12 +4878,9 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>      if (virDomainObjIsActive(vm) &&
>          virDomainDefSetVCpus(vm->def, vcpus) < 0)
>          ret = -1;
> -    virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", rc == 1);
> +    virDomainAuditVcpu(vm, oldvcpus, nvcpus, "update", ret == 0);
>      return ret;
> 
> - unsupported:
> -    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                   _("cannot change vcpu count of this domain"));
>   exit_monitor:
>      ignore_value(qemuDomainObjExitMonitor(driver, vm));
>      goto cleanup;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 86b8c7b..50d6f62 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c

Need to adjust comments here...  Probably could move the comments to
qemuMonitorSetCPU just so it doesn't cause chase into second level to
know what function returns.

> @@ -2188,10 +2188,6 @@ int qemuMonitorJSONSetCPU(qemuMonitorPtr mon,
>      else
>          ret = qemuMonitorJSONCheckError(cmd, reply);
> 
> -    /* this function has non-standard return values, so adapt it */
> -    if (ret == 0)
> -        ret = 1;
> -
>   cleanup:
>      virJSONValueFree(cmd);
>      virJSONValueFree(reply);
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index f44da20..fd38d01 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -1137,8 +1137,7 @@ qemuMonitorTextSetBalloon(qemuMonitorPtr mon,
> 
> 
>  /*
> - * Returns: 0 if CPU hotplug not supported, +1 if CPU hotplug worked
> - * or -1 on failure
> + * Returns: 0 if CPU modification was successful or -1 on failure
>   */

Could copy/move the comment to qemuMonitorSetCPU

ACK - as long as JSON function comments modified.

John

>  int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool online)
>  {
> @@ -1149,22 +1148,23 @@ int qemuMonitorTextSetCPU(qemuMonitorPtr mon, int cpu, bool online)
>      if (virAsprintf(&cmd, "cpu_set %d %s", cpu, online ? "online" : "offline") < 0)
>          return -1;
> 
> -    if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) {
> -        VIR_FREE(cmd);
> -        return -1;
> -    }
> -    VIR_FREE(cmd);
> +    if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0)
> +        goto cleanup;
> 
>      /* If the command failed qemu prints: 'unknown command'
>       * No message is printed on success it seems */
>      if (strstr(reply, "unknown command:")) {
> -        /* Don't set error - it is expected CPU onlining fails on many qemu - caller will handle */
> -        ret = 0;
> -    } else {
> -        ret = 1;
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("cannot change vcpu count of this domain"));
> +        goto cleanup;
>      }
> 
> +    ret = 0;
> +
> + cleanup:
>      VIR_FREE(reply);
> +    VIR_FREE(cmd);
> +
>      return ret;
>  }
>