Re: [PATCH] qemu: numatune/domiftune no support in session mode

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Sep 03, 2014 at 05:00:15PM +0200, Erik Skultety wrote:
> Tuning NUMA or network interface parameters require root
> privileges, thus an attempt to set some of these parameters in
> session mode should be invalid followed by an error. As an example might
> be memory tuning which raises an error in such case. This patch
> provides similar behavior for numatune and domiftune.
> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
> ---
> src/qemu/qemu_command.c | 33 ++++++++++++++++++++++++++++++++-
> src/qemu/qemu_driver.c  | 20 ++++++++++++++++++++
> 2 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c84c7c3..c021080 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7443,7 +7443,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>     emulator = def->emulator;
>
>     if (!cfg->privileged) {
> -        /* If we have no cgroups than we can have no tunings that
> +        /* If we have no cgroups then we can have no tunings that

Good catch, I (almost) always type something else than what I want :)

>          * require them */
>
>         if (def->mem.hard_limit || def->mem.soft_limit ||
> @@ -7466,6 +7466,37 @@ qemuBuildCommandLine(virConnectPtr conn,
>                            _("CPU tuning is not available in session mode"));
>             goto error;
>         }
> +
> +        if (virDomainNumatuneGetNodeset(def->numatune, NULL, -1) ||
> +            virDomainNumatuneGetMode(def->numatune, -1)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("NUMA tuning is not available in session mode"));
> +            goto error;
> +        }
> +

This is not entirely true.  We are using cgroups to restrict the nodes
used, but we are _also_ using libnuma functions to set the binding and
modes, etc.

I guess this should be skipped while starting and forbidden when being
changed live (we can no longer use those functions then).

> +        virDomainNetDefPtr *nets = def->nets;
> +        size_t nnets = def->nnets;
> +        for (i = 0; i < nnets; i++) {
> +            if (nets[i]->bandwidth) {
> +                if (nets[i]->bandwidth->in &&
> +                    (nets[i]->bandwidth->in->average ||
> +                     nets[i]->bandwidth->in->peak ||
> +                     nets[i]->bandwidth->in->burst)) {
> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("Network bandwidth tuning is not available in session mode"));
> +                    goto error;
> +                }
> +
> +                if (nets[i]->bandwidth->out &&
> +                    (nets[i]->bandwidth->out->average ||
> +                     nets[i]->bandwidth->out->peak ||
> +                     nets[i]->bandwidth->out->burst)) {

You're not checking ->floor attribute and I think it's not very
different, or is it?

> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                                   _("Network bandwidth tuning is not available in session mode"));
> +                    goto error;
> +                }
> +            }
> +        }
>     }
>
>     for (i = 0; i < def->ngraphics; ++i) {
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 239a300..b46e12f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8779,6 +8779,12 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
>     if (virDomainSetNumaParametersEnsureACL(dom->conn, vm->def, flags) < 0)
>         goto cleanup;
>
> +    if (!cfg->privileged) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("NUMA tuning is not available in session mode"));
> +        goto cleanup;
> +    }
> +

This error should be only emitted when the api is supposed to change
it on a live domain.  Otherwise it's just setting an XML which the
user can set in any other way, so this wouldn't help.  And as written
above, it should work when starting the domain.

>     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>         goto cleanup;
>
> @@ -8870,6 +8876,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>     size_t i;
>     virDomainObjPtr vm = NULL;
>     virDomainDefPtr persistentDef = NULL;
> +    virQEMUDriverConfigPtr cfg = NULL;
>     char *nodeset = NULL;
>     int ret = -1;
>     virCapsPtr caps = NULL;
> @@ -8888,10 +8895,17 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
>         return -1;
>
>     priv = vm->privateData;
> +    cfg = virQEMUDriverGetConfig(driver);

You need to do virObjectUnref(cfg); in the cleanup phase, it's
reference-counted.

>     if (virDomainGetNumaParametersEnsureACL(dom->conn, vm->def) < 0)
>         goto cleanup;
>
> +    if (!cfg->privileged) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("NUMA tuning is not available in session mode"));
> +        goto cleanup;
> +    }
> +

Why aren't we allowing this?  It's a getter only.

>     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>         goto cleanup;
>
> @@ -9889,6 +9903,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
>     if (virDomainSetInterfaceParametersEnsureACL(dom->conn, vm->def, flags) < 0)
>         goto cleanup;
>
> +    if (!cfg->privileged) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("Network bandwidth tuning is not available in session mode"));
> +        goto cleanup;
> +    }
> +

This too should be true for live only.

>     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>         goto cleanup;
>
> --
> 1.9.3
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list