11:21 a.m.
<BLURB/>
Martin Kletzander (2):
tests: Create privileged driver config in qemuxml2argvtest
qemu: Reject unsupported tuning in session mode
src/qemu/qemu_command.c | 32 +++++++++++++++
src/qemu/qemu_driver.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++-
tests/qemuxml2argvtest.c | 2 +-
3 files changed, 135 insertions(+), 2 deletions(-)
--
1.9.0
11:21 a.m.
New subject: [libvirt] [PATCH 1/2] tests: Create privileged driver config in qemuxml2argvtest
This is actaully a proper setting since we're not checking
session-mode related XMLs.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
tests/qemuxml2argvtest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index af7194f..5d6a64b 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -500,7 +500,7 @@ mymain(void)
return EXIT_FAILURE;
}
- driver.config = virQEMUDriverConfigNew(false);
+ driver.config = virQEMUDriverConfigNew(true);
VIR_FREE(driver.config->spiceListen);
VIR_FREE(driver.config->vncListen);
--
1.9.0
1:19 p.m.
New subject: [libvirt] [PATCH 1/2] tests: Create privileged driver config in qemuxml2argvtest
On 03/03/2014 10:21 AM, Martin Kletzander wrote:
This is actaully a proper setting since we're not checking
s/actaully/actually/
session-mode related XMLs.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
tests/qemuxml2argvtest.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index af7194f..5d6a64b 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -500,7 +500,7 @@ mymain(void)
return EXIT_FAILURE;
}
- driver.config = virQEMUDriverConfigNew(false);
+ driver.config = virQEMUDriverConfigNew(true);
VIR_FREE(driver.config->spiceListen);
VIR_FREE(driver.config->vncListen);
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:04 a.m.
New subject: [libvirt] [PATCH 1/2] tests: Create privileged driver config in qemuxml2argvtest
On Mon, Mar 03, 2014 at 12:19:07PM -0700, Eric Blake wrote:
On 03/03/2014 10:21 AM, Martin Kletzander wrote:
> This is actaully a proper setting since we're not checking
s/actaully/actually/
> session-mode related XMLs.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> tests/qemuxml2argvtest.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Thanks, pushed.
Martin
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index af7194f..5d6a64b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -500,7 +500,7 @@ mymain(void)
> return EXIT_FAILURE;
> }
>
> - driver.config = virQEMUDriverConfigNew(false);
> + driver.config = virQEMUDriverConfigNew(true);
> VIR_FREE(driver.config->spiceListen);
> VIR_FREE(driver.config->vncListen);
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:21 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Reject unsupported tuning in session mode
When domain is started with setting that cannot be done, i.e. those
that require cgroups, there is no error reported and it succeeds
without any message whatsoever.
When setting with API, virsh, an error is reported, but only due to
the fact that no cgroups are mounted (priv->cgroup == NULL).
Given the above it seems reasonable to reject such unsupported
settings.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Notes:
Few questions came to my mind while writing the commit message:
1) Would helper function (macro) be preferred so the code looks
cleaner?
2) Do we want to allow reading of some of these settings through an
API? This would however require our cgroup handling to be
reworked.
3) Would new error type (for session-unsupported settings) be any
good or it doesn't make sense to create one just for these added
messages plus few older ones (just guessing the amount)?
src/qemu/qemu_command.c | 32 +++++++++++++++
src/qemu/qemu_driver.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 134 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9ee84a0..4b628d0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7763,6 +7763,38 @@ qemuBuildCommandLine(virConnectPtr conn,
emulator = def->emulator;
+ if (!cfg->privileged) {
+ /* If we have no cgroups than we can have no tunings that
+ * require them */
+
+ if (def->mem.hard_limit || def->mem.soft_limit ||
+ def->mem.min_guarantee || def->mem.swap_hard_limit) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session
mode"));
+ goto error;
+ }
+
+ if (def->blkio.weight || def->blkio.ndevices) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto error;
+ }
+
+ if (def->cputune.shares || def->cputune.period ||
+ def->cputune.quota || def->cputune.emulator_period ||
+ def->cputune.emulator_quota) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto error;
+ }
+
+ if (def->cputune.nvcpupin || def->cputune.emulatorpin || def->cpumask)
{
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU pinning is not available in session mode"));
+ goto error;
+ }
+ }
+
for (i = 0; i < def->ngraphics; ++i) {
switch (def->graphics[i]->type) {
case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e04a328..29a77d8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4320,6 +4320,12 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
if (virDomainPinVcpuFlagsEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU pinning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -4493,6 +4499,7 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
unsigned char *cpumap;
bool pinned;
virCapsPtr caps = NULL;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -4503,6 +4510,13 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom,
if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU pinning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -4560,6 +4574,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -4594,6 +4609,12 @@ qemuDomainPinEmulator(virDomainPtr dom,
if (virDomainPinEmulatorEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU pinning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -4738,6 +4759,7 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
virBitmapPtr cpumask = NULL;
bool pinned;
virCapsPtr caps = NULL;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -4748,6 +4770,13 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom,
if (virDomainGetEmulatorPinInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU pinning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -4796,6 +4825,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -7434,6 +7464,8 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
char *ret = NULL;
virDomainObjPtr vm = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverPtr driver = dom->conn->privateData;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (!(vm = qemuDomObjFromDomain(dom)))
goto cleanup;
@@ -7443,6 +7475,12 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
/* Domain not running, thus no cgroups - return defaults */
if (!virDomainObjIsActive(vm)) {
if (nparams)
@@ -7469,6 +7507,7 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
cleanup:
if (vm)
virObjectUnlock(vm);
+ virObjectUnref(cfg);
return ret;
}
@@ -7683,6 +7722,13 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
if (virDomainSetBlkioParametersEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -7863,6 +7909,7 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
int ret = -1;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -7881,6 +7928,13 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
if (virDomainGetBlkioParametersEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8269,6 +8323,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -8316,6 +8371,12 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
if (virDomainSetMemoryParametersEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8423,6 +8484,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
int ret = -1;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -8439,6 +8501,13 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
if (virDomainGetMemoryParametersEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -8550,6 +8619,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -8975,6 +9045,13 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags)
< 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -9210,6 +9287,7 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
virDomainDefPtr persistentDef;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -9226,6 +9304,13 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom,
if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("CPU tuning is not available in session mode"));
+ goto cleanup;
+ }
+
if (*nparams > 1)
cpu_bw_status = virCgroupSupportsCpuBW(priv->cgroup);
@@ -9320,6 +9405,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
@@ -15520,11 +15606,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
priv = vm->privateData;
- cfg = virQEMUDriverGetConfig(driver);
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto endjob;
@@ -15667,6 +15759,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
int ret = -1;
size_t i;
virCapsPtr caps = NULL;
+ virQEMUDriverConfigPtr cfg = NULL;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -15681,6 +15774,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
if (virDomainGetBlockIoTuneEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ cfg = virQEMUDriverGetConfig(driver);
+ if (!cfg->privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Block I/O tuning is not available in session
mode"));
+ goto cleanup;
+ }
+
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
@@ -15783,6 +15883,7 @@ cleanup:
if (vm)
virObjectUnlock(vm);
virObjectUnref(caps);
+ virObjectUnref(cfg);
return ret;
}
--
1.9.0
7:44 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Reject unsupported tuning in session mode
On 03/03/2014 10:21 AM, Martin Kletzander wrote:
When domain is started with setting that cannot be done, i.e. those
that require cgroups, there is no error reported and it succeeds
without any message whatsoever.
When setting with API, virsh, an error is reported, but only due to
the fact that no cgroups are mounted (priv->cgroup == NULL).
Given the above it seems reasonable to reject such unsupported
settings.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
Notes:
Few questions came to my mind while writing the commit message:
1) Would helper function (macro) be preferred so the code looks
cleaner?
What macro do you have in mind?
2) Do we want to allow reading of some of these settings through an
API? This would however require our cgroup handling to be
reworked.
3) Would new error type (for session-unsupported settings) be any
good or it doesn't make sense to create one just for these added
messages plus few older ones (just guessing the amount)?
Looks like your use of VIR_ERR_CONFIG_UNSUPPORTED was reasonable.
+ if (!cfg->privileged) {
+ /* If we have no cgroups than we can have no tunings that
+ * require them */
+
+ if (def->mem.hard_limit || def->mem.soft_limit ||
+ def->mem.min_guarantee || def->mem.swap_hard_limit) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Memory tuning is not available in session
mode"));
+ goto error;
+ }
+
Or maybe we should think about some day adding a daemon that accepts RPC
commands for manipulating cgroups on behalf of a session client. But
that would be a later and bigger patch.
Seems reasonable to me, but it might be nice to also get Dan's opinion
on this one.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:13 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Reject unsupported tuning in session mode
On Tue, Mar 04, 2014 at 06:44:01AM -0700, Eric Blake wrote:
On 03/03/2014 10:21 AM, Martin Kletzander wrote:
> When domain is started with setting that cannot be done, i.e. those
> that require cgroups, there is no error reported and it succeeds
> without any message whatsoever.
>
> When setting with API, virsh, an error is reported, but only due to
> the fact that no cgroups are mounted (priv->cgroup == NULL).
>
> Given the above it seems reasonable to reject such unsupported
> settings.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
>
> Notes:
> Few questions came to my mind while writing the commit message:
>
> 1) Would helper function (macro) be preferred so the code looks
> cleaner?
What macro do you have in mind?
I haven't thought that through, just from top of my head:
#define SESSION_UNSUPP(what) if (!cfg->privileged) { \
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s %s", \
what, _("is not available in sesison mode")); \
goto cleanup; \
}
because the code repeats a lot, but it seems like it would not be that
readable. Separate function would do as well.
>
> 2) Do we want to allow reading of some of these settings through an
> API? This would however require our cgroup handling to be
> reworked.
>
> 3) Would new error type (for session-unsupported settings) be any
> good or it doesn't make sense to create one just for these added
> messages plus few older ones (just guessing the amount)?
Looks like your use of VIR_ERR_CONFIG_UNSUPPORTED was reasonable.
> + if (!cfg->privileged) {
> + /* If we have no cgroups than we can have no tunings that
> + * require them */
> +
> + if (def->mem.hard_limit || def->mem.soft_limit ||
> + def->mem.min_guarantee || def->mem.swap_hard_limit) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Memory tuning is not available in session
mode"));
> + goto error;
> + }
> +
Or maybe we should think about some day adding a daemon that accepts RPC
commands for manipulating cgroups on behalf of a session client. But
that would be a later and bigger patch.
I was just trying to output a better error message and forgot to add
the BZ that started it:
https://bugzilla.redhat.com/show_bug.cgi?id=1023366
Seems reasonable to me, but it might be nice to also get Dan's
opinion
on this one.
Anyway, I'll have a second version which allows at least cpu pinning
as setaffinity works and is used when no cgroups are available.
Martin
10:52 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Reject unsupported tuning in session mode
On 03/04/2014 07:13 AM, Martin Kletzander wrote:
>> 1) Would helper function (macro) be preferred so the
code looks
>> cleaner?
>
> What macro do you have in mind?
>
I haven't thought that through, just from top of my head:
#define SESSION_UNSUPP(what) if (!cfg->privileged) { \
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s %s", \
what, _("is not available in sesison mode")); \
That's not very nice to translators. At this point, what you started
with is better than what a macro would give.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3916
days inactive
3917
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Martin Kletzander