[libvirt] [PATCH 5/7] qemu: Abstract code for the cpu controller setting into a helper

[Daniel P. Berrange]

On Fri, May 17, 2013 at 07:59:35PM +0800, Osier Yang wrote:
> ---
>  src/qemu/qemu_cgroup.c | 63 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 8f84ef9..8a2cc9d 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -671,6 +671,35 @@ cleanup:
>      return ret;
>  }
>  
> +static int
> +qemuSetupCpuCgroup(virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int rc = -1;
> +
> +    if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> +       if (vm->def->cputune.shares) {
> +           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                          _("CPU tuning is not available on this host"));
> +           return -1;
> +       } else {
> +           return 0;
> +       }
> +    }
> +
> +    if (vm->def->cputune.shares) {
> +        rc = virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares);
> +        if (rc != 0) {
> +            virReportSystemError(-rc,
> +                                 _("Unable to set io cpu shares for domain %s"),
> +                                 vm->def->name);
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +

+ blank line

>  int qemuInitCgroup(virQEMUDriverPtr driver,
>                     virDomainObjPtr vm,
>                     bool startup)
> @@ -784,46 +813,30 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
>                      virDomainObjPtr vm,
>                      virBitmapPtr nodemask)
>  {
> -    int rc = -1;
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>  
>      if (qemuInitCgroup(driver, vm, true) < 0)
>          return -1;
>  
>      if (!priv->cgroup)
> -        goto done;
> +        return 0;
>  
>      if (qemuSetupDevicesCgroup(driver, vm) < 0)
> -        goto cleanup;
> +        return -1;
>  
>      if (qemuSetupBlkioCgroup(vm) < 0)
> -        goto cleanup;
> +        return -1;
>  
>      if (qemuSetupMemoryCgroup(vm) < 0)
> -        goto cleanup;
> -
> -    if (vm->def->cputune.shares != 0) {
> -        if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
> -            rc = virCgroupSetCpuShares(priv->cgroup, vm->def->cputune.shares);
> -            if (rc != 0) {
> -                virReportSystemError(-rc,
> -                                     _("Unable to set io cpu shares for domain %s"),
> -                                     vm->def->name);
> -                goto cleanup;
> -            }
> -        } else {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("CPU tuning is not available on this host"));
> -        }
> -    }
> +        return -1;
>  
>      if (qemuSetupCpusetCgroup(vm, nodemask) < 0)
> -        goto cleanup;
> +        return -1;
>  
> -done:
> -    rc = 0;
> -cleanup:
> -    return rc == 0 ? 0 : -1;
> +    if (qemuSetupCpuCgroup(vm) < 0)
> +        return -1;
> +
> +    return 0;
>  }

I don't think you should be replacing all the 'goto cleanup' lines
with 'return -1' here. It is good practice to have a cleanup block,
even if not currently used, since historically we've found that we
need to add such blocks in more often than not.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|