Re: [libvirt] [PATCHv3] qemu: NUMA/network tuning shouldn't be supported in session mode

2014-10-24 Thread Laine Stump 
On 10/22/2014 03:00 PM, John Ferlan wrote:
 On 10/01/2014 08:57 AM, Erik Skultety wrote:
 Tuning NUMA or network interface parameters require root
 privileges to manage cgroups, thus an attempt to set some of these
 parameters in session mode on a running domain should be invalid
 followed by an error.
 As an example might be memory tuning which raises an error in such case.
 Following behavior in session mode will be present after applying
 this patch:

   Tuning  |  SET  |   GET  |
 --|---||
 NUMA  | shut off only | always |
 Memory| never | never  |
 Interface | never | always |

 Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
 ---
  src/qemu/qemu_command.c | 13 -
  src/qemu/qemu_driver.c  | 35 +--
  2 files changed, 37 insertions(+), 11 deletions(-)


 I was going through some of my list backlog - it seems this was orphaned
 :-)...  Since v3 addressed Mark's comment, I rebased it to top of
 tree... adjusted the title to be just:

 qemu: Disallow NUMA/network tuning for session mode

 adjusted the grammar of the commit message a bit, and pushed

 John


 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index eb72451..4c335dc 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -7671,7 +7671,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
   * require them */
  
  if (def-mem.hard_limit || def-mem.soft_limit ||
 @@ -7694,6 +7694,17 @@ qemuBuildCommandLine(virConnectPtr conn,
 _(CPU tuning is not available in session 
 mode));
  goto error;
  }
 +
 +virDomainNetDefPtr *nets = def-nets;
 +virNetDevBandwidthPtr bandwidth = NULL;
 +size_t nnets = def-nnets;
 +for (i = 0; i  nnets; i++) {
 +if ((bandwidth = virDomainNetGetActualBandwidth(nets[i])) != 
 NULL) {
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
 +_(Network bandwidth tuning is not available in session 
 mode));
 +goto error;
 +}
 +}

John's review mail popped this patch (which I had missed before) up in
my inbox, and I noticed this snippet.

I think this is the wrong place to do this check - all the other bits
being checked inside this if (!cfg-privileged) are, as the comment
suggests, related to cgroups, while bandwidth management isn't (at least
not directly - it's setup with the tc command). I think it would be
better to check for !privileged at the place where the bandwidth is
actually set (although it doesn't have access to cfg, so you would only
be able to look at getuid() - is that adequate?). Doing the check at
that location would assure that it's properly done for *all* situations
when someone tries to set bandwidth, e.g. for an interface hotplug, for
LXC, for virDomainSetInterfaceParameters()...

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

Re: [libvirt] [PATCHv3] qemu: NUMA/network tuning shouldn't be supported in session mode

2014-10-23 Thread Erik Skultety 

Splendid, thank you John :).
Erik

On 10/22/2014 09:00 PM, John Ferlan wrote:


On 10/01/2014 08:57 AM, Erik Skultety wrote:

Tuning NUMA or network interface parameters require root
privileges to manage cgroups, thus an attempt to set some of these
parameters in session mode on a running domain should be invalid
followed by an error.
As an example might be memory tuning which raises an error in such case.
Following behavior in session mode will be present after applying
this patch:

   Tuning  |  SET  |   GET  |
--|---||
NUMA  | shut off only | always |
Memory| never | never  |
Interface | never | always |

Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
---
  src/qemu/qemu_command.c | 13 -
  src/qemu/qemu_driver.c  | 35 +--
  2 files changed, 37 insertions(+), 11 deletions(-)




I was going through some of my list backlog - it seems this was orphaned
:-)...  Since v3 addressed Mark's comment, I rebased it to top of
tree... adjusted the title to be just:

qemu: Disallow NUMA/network tuning for session mode

adjusted the grammar of the commit message a bit, and pushed

John



diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72451..4c335dc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7671,7 +7671,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
   * require them */

  if (def-mem.hard_limit || def-mem.soft_limit ||
@@ -7694,6 +7694,17 @@ qemuBuildCommandLine(virConnectPtr conn,
 _(CPU tuning is not available in session mode));
  goto error;
  }
+
+virDomainNetDefPtr *nets = def-nets;
+virNetDevBandwidthPtr bandwidth = NULL;
+size_t nnets = def-nnets;
+for (i = 0; i  nnets; i++) {
+if ((bandwidth = virDomainNetGetActualBandwidth(nets[i])) != NULL) 
{
+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 6606154..c64d272 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8974,6 +8974,13 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
  persistentDef)  0)
  goto cleanup;

+if (!cfg-privileged 
+flags  VIR_DOMAIN_AFFECT_LIVE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
+   _(NUMA tuning is not available in session mode));
+goto cleanup;
+}
+
  if (flags  VIR_DOMAIN_AFFECT_LIVE) {
  if (!virCgroupHasController(priv-cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
  virReportError(VIR_ERR_OPERATION_INVALID, %s,
@@ -9058,6 +9065,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;
@@ -9076,6 +9084,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  return -1;

  priv = vm-privateData;
+cfg = virQEMUDriverGetConfig(driver);

  if (virDomainGetNumaParametersEnsureACL(dom-conn, vm-def)  0)
  goto cleanup;
@@ -9093,14 +9102,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  goto cleanup;
  }

-if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-if (!virCgroupHasController(priv-cgroup, 
VIR_CGROUP_CONTROLLER_MEMORY)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   %s, _(cgroup memory controller is not mounted));
-goto cleanup;
-}
-}
-
  for (i = 0; i  QEMU_NB_NUMA_PARAM  i  *nparams; i++) {
  virMemoryParameterPtr param = params[i];

@@ -9123,9 +9124,16 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  if (!nodeset)
  goto cleanup;
  } else {
-if (virCgroupGetCpusetMems(priv-cgroup, nodeset)  0)
-goto cleanup;
+if (!virCgroupHasController(priv-cgroup,
+VIR_CGROUP_CONTROLLER_MEMORY) ||
+virCgroupGetCpusetMems(priv-cgroup, nodeset)  0) {
+nodeset = virDomainNumatuneFormatNodeset(vm-def-numatune,
+ NULL, -1);
+if (!nodeset)
+goto cleanup;
+}
  }
+
  if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET,

Re: [libvirt] [PATCHv3] qemu: NUMA/network tuning shouldn't be supported in session mode

2014-10-22 Thread John Ferlan 

On 10/01/2014 08:57 AM, Erik Skultety wrote:
 Tuning NUMA or network interface parameters require root
 privileges to manage cgroups, thus an attempt to set some of these
 parameters in session mode on a running domain should be invalid
 followed by an error.
 As an example might be memory tuning which raises an error in such case.
 Following behavior in session mode will be present after applying
 this patch:
 
   Tuning  |  SET  |   GET  |
 --|---||
 NUMA  | shut off only | always |
 Memory| never | never  |
 Interface | never | always |
 
 Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
 ---
  src/qemu/qemu_command.c | 13 -
  src/qemu/qemu_driver.c  | 35 +--
  2 files changed, 37 insertions(+), 11 deletions(-)
 


I was going through some of my list backlog - it seems this was orphaned
:-)...  Since v3 addressed Mark's comment, I rebased it to top of
tree... adjusted the title to be just:

qemu: Disallow NUMA/network tuning for session mode

adjusted the grammar of the commit message a bit, and pushed

John


 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index eb72451..4c335dc 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -7671,7 +7671,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
   * require them */
  
  if (def-mem.hard_limit || def-mem.soft_limit ||
 @@ -7694,6 +7694,17 @@ qemuBuildCommandLine(virConnectPtr conn,
 _(CPU tuning is not available in session mode));
  goto error;
  }
 +
 +virDomainNetDefPtr *nets = def-nets;
 +virNetDevBandwidthPtr bandwidth = NULL;
 +size_t nnets = def-nnets;
 +for (i = 0; i  nnets; i++) {
 +if ((bandwidth = virDomainNetGetActualBandwidth(nets[i])) != 
 NULL) {
 +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 6606154..c64d272 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -8974,6 +8974,13 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
  persistentDef)  0)
  goto cleanup;
  
 +if (!cfg-privileged 
 +flags  VIR_DOMAIN_AFFECT_LIVE) {
 +virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
 +   _(NUMA tuning is not available in session mode));
 +goto cleanup;
 +}
 +
  if (flags  VIR_DOMAIN_AFFECT_LIVE) {
  if (!virCgroupHasController(priv-cgroup, 
 VIR_CGROUP_CONTROLLER_CPUSET)) {
  virReportError(VIR_ERR_OPERATION_INVALID, %s,
 @@ -9058,6 +9065,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;
 @@ -9076,6 +9084,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  return -1;
  
  priv = vm-privateData;
 +cfg = virQEMUDriverGetConfig(driver);
  
  if (virDomainGetNumaParametersEnsureACL(dom-conn, vm-def)  0)
  goto cleanup;
 @@ -9093,14 +9102,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  goto cleanup;
  }
  
 -if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 -if (!virCgroupHasController(priv-cgroup, 
 VIR_CGROUP_CONTROLLER_MEMORY)) {
 -virReportError(VIR_ERR_OPERATION_INVALID,
 -   %s, _(cgroup memory controller is not 
 mounted));
 -goto cleanup;
 -}
 -}
 -
  for (i = 0; i  QEMU_NB_NUMA_PARAM  i  *nparams; i++) {
  virMemoryParameterPtr param = params[i];
  
 @@ -9123,9 +9124,16 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
  if (!nodeset)
  goto cleanup;
  } else {
 -if (virCgroupGetCpusetMems(priv-cgroup, nodeset)  0)
 -goto cleanup;
 +if (!virCgroupHasController(priv-cgroup,
 +VIR_CGROUP_CONTROLLER_MEMORY) ||
 +virCgroupGetCpusetMems(priv-cgroup, nodeset)  0) {
 +nodeset = 
 virDomainNumatuneFormatNodeset(vm-def-numatune,
 + NULL, -1);
 +if (!nodeset)
 +goto cleanup;
 +}
  }
 +
  if (virTypedParameterAssign(param,

[libvirt] [PATCHv3] qemu: NUMA/network tuning shouldn't be supported in session mode

2014-10-01 Thread Erik Skultety 
Tuning NUMA or network interface parameters require root
privileges to manage cgroups, thus an attempt to set some of these
parameters in session mode on a running domain should be invalid
followed by an error.
As an example might be memory tuning which raises an error in such case.
Following behavior in session mode will be present after applying
this patch:

  Tuning  |  SET  |   GET  |
--|---||
NUMA  | shut off only | always |
Memory| never | never  |
Interface | never | always |

Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1126762
---
 src/qemu/qemu_command.c | 13 -
 src/qemu/qemu_driver.c  | 35 +--
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index eb72451..4c335dc 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7671,7 +7671,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
  * require them */
 
 if (def-mem.hard_limit || def-mem.soft_limit ||
@@ -7694,6 +7694,17 @@ qemuBuildCommandLine(virConnectPtr conn,
_(CPU tuning is not available in session mode));
 goto error;
 }
+
+virDomainNetDefPtr *nets = def-nets;
+virNetDevBandwidthPtr bandwidth = NULL;
+size_t nnets = def-nnets;
+for (i = 0; i  nnets; i++) {
+if ((bandwidth = virDomainNetGetActualBandwidth(nets[i])) != NULL) 
{
+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 6606154..c64d272 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8974,6 +8974,13 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
 persistentDef)  0)
 goto cleanup;
 
+if (!cfg-privileged 
+flags  VIR_DOMAIN_AFFECT_LIVE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, %s,
+   _(NUMA tuning is not available in session mode));
+goto cleanup;
+}
+
 if (flags  VIR_DOMAIN_AFFECT_LIVE) {
 if (!virCgroupHasController(priv-cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
 virReportError(VIR_ERR_OPERATION_INVALID, %s,
@@ -9058,6 +9065,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;
@@ -9076,6 +9084,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
 return -1;
 
 priv = vm-privateData;
+cfg = virQEMUDriverGetConfig(driver);
 
 if (virDomainGetNumaParametersEnsureACL(dom-conn, vm-def)  0)
 goto cleanup;
@@ -9093,14 +9102,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
 goto cleanup;
 }
 
-if (flags  VIR_DOMAIN_AFFECT_LIVE) {
-if (!virCgroupHasController(priv-cgroup, 
VIR_CGROUP_CONTROLLER_MEMORY)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   %s, _(cgroup memory controller is not mounted));
-goto cleanup;
-}
-}
-
 for (i = 0; i  QEMU_NB_NUMA_PARAM  i  *nparams; i++) {
 virMemoryParameterPtr param = params[i];
 
@@ -9123,9 +9124,16 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
 if (!nodeset)
 goto cleanup;
 } else {
-if (virCgroupGetCpusetMems(priv-cgroup, nodeset)  0)
-goto cleanup;
+if (!virCgroupHasController(priv-cgroup,
+VIR_CGROUP_CONTROLLER_MEMORY) ||
+virCgroupGetCpusetMems(priv-cgroup, nodeset)  0) {
+nodeset = virDomainNumatuneFormatNodeset(vm-def-numatune,
+ NULL, -1);
+if (!nodeset)
+goto cleanup;
+}
 }
+
 if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET,
 VIR_TYPED_PARAM_STRING, nodeset)  0)
 goto cleanup;
@@ -9150,6 +9158,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
 if (vm)
 virObjectUnlock(vm);
 virObjectUnref(caps);
+virObjectUnref(cfg);
 return ret;
 }
 
@@ -10120,6 +10129,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
 if (virDomainSetInterfaceParametersEnsureACL(dom-conn, vm-def, flags)  
0)
 goto