Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

2010-10-13 Thread Daniel Veillard 
On Wed, Oct 13, 2010 at 10:57:17AM +0530, Nikunj A. Dadhania wrote:
 On Tue, 12 Oct 2010 17:51:54 +0200, Daniel Veillard veill...@redhat.com 
 wrote:
  On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
   From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
   
   V4:
   * prototype change: add unsigned int flags
   
   Driver interface for getting memory parameters, eg. hard_limit, 
   soft_limit and
   swap_hard_limit.
  
   +qemuReportError(VIR_ERR_INVALID_ARG,
   +%s, _(Invalid parameter count));
   +goto cleanup;
   +}
  
okay, this mean the application must always call with 0 first to get
the exact value or this will break, fine but probably need to be made
more clear from the description in libvirt.c  TODO
  
 Sure, I will take care of updating the api desc in libvirt.c, I haven't used
 word always there.
 
   +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 
   0) {
   +qemuReportError(VIR_ERR_INTERNAL_ERROR,
   +_(cannot find cgroup for domain %s), 
   vm-def-name);
   +goto cleanup;
   +}
   +
   +for (i = 0; i  *nparams; i++) {
   +virMemoryParameterPtr param = params[i];
   +val = 0;
   +param-value.ul = 0;
   +param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
   +
   +switch(i) {
   +case 0: /* fill memory hard limit here */
   +rc = virCgroupGetMemoryHardLimit(group, val);
   +if (rc != 0) {
   +virReportSystemError(-rc, %s,
   + _(unable to get memory hard 
   limit));
   +continue;
   +}
   +if (virStrcpyStatic(param-field, 
   VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) {
   +qemuReportError(VIR_ERR_INTERNAL_ERROR,
   +%s, _(Field memory hard limit too 
   long for destination));
   +continue;
   +}
   +param-value.ul = val;
   +break;
   +
   +case 1: /* fill memory soft limit here */
   +rc = virCgroupGetMemorySoftLimit(group, val);
   +if (rc != 0) {
   +virReportSystemError(-rc, %s,
   + _(unable to get memory soft 
   limit));
   +continue;
   +}
   +if (virStrcpyStatic(param-field, 
   VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) {
   +qemuReportError(VIR_ERR_INTERNAL_ERROR,
   +%s, _(Field memory soft limit too 
   long for destination));
   +continue;
   +}
   +param-value.ul = val;
   +break;
   +   
   +case 2: /* fill swap hard limit here */
   +rc = virCgroupGetSwapHardLimit(group, val);
   +if (rc != 0) {
   +virReportSystemError(-rc, %s,
   + _(unable to get swap hard limit));
   +continue;
   +}
   +if (virStrcpyStatic(param-field, 
   VIR_DOMAIN_SWAP_HARD_LIMIT) == NULL) {
   +qemuReportError(VIR_ERR_INTERNAL_ERROR,
   +%s, _(Field swap hard limit too long 
   for destination));
   +continue;
   +}
   +param-value.ul = val;
   +break;
   +
   +default:
   +break;
   +/* should not hit here */
   +}
   +}
  
Okay, I'm not sure we actually need a loop here, but it may help
refactoring...
 I guess this is related to my previous thinking, if nparams 
 QEMU_NB_MEM_PARAM, fill only till nparams and return. But with the change of
 the logic, I think loop may not be required now.

  I think keeping the loop is fine at this point, even if not strictly
necessary.

  I'm still having a problem with the code ignoring any error occuring in
  the loop, and fixing this in the same way. If there is an error the
  application *must* learn about it instead of trusting uninitialized
  memory as being data !
  Maybe a memset is in order actually before entering that loop to avoid
  edge case problems... TODO too
 
 By TODO you mean the error handling, right?  

  the error handling is done, I fixed those before commiting. Please
  fetch the new git tree before trying to make any further patches.

 I am taking care of setting the values to zero currently, and it does not tell
 the application whether to use this value or not.  One option could be adding
 VIR_DOMAIN_MEMORY_INVALID in virMemoryParameterType and setting it in the
 beginning of the loop. Comments?

That would be one way for the user when getting back an error to find
out if there were still useful values. It makes the application more
complex, and using that call is already too complex IMHO.
If we fail in the loop, it's a internal error

Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

2010-10-12 Thread Daniel Veillard 
On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
 From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 
 V4:
 * prototype change: add unsigned int flags
 
 Driver interface for getting memory parameters, eg. hard_limit, soft_limit and
 swap_hard_limit.
 
 Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
 ---
  src/qemu/qemu_driver.c |  120 
 
  1 files changed, 119 insertions(+), 1 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 471db39..8eaa762 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9458,6 +9458,124 @@ cleanup:
  return ret;
  }

  same things about crashing if the arguments are invalid

 +if ((*nparams) == 0) {
 +/* Current number of memory parameters supported by cgroups is 3
 + * FIXME: Magic number, need to see where should this go
 + */
 +*nparams = 3;
 +ret = 0;
 +goto cleanup;
 +}
 +
 +if ((*nparams) != 3) {

  using QEMU_NB_MEM_PARAM instead of the raw 3 value c.f. previous patch
  comment

 +qemuReportError(VIR_ERR_INVALID_ARG,
 +%s, _(Invalid parameter count));
 +goto cleanup;
 +}

  okay, this mean the application must always call with 0 first to get
  the exact value or this will break, fine but probably need to be made
  more clear from the description in libvirt.c  TODO

 +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +_(cannot find cgroup for domain %s), 
 vm-def-name);
 +goto cleanup;
 +}
 +
 +for (i = 0; i  *nparams; i++) {
 +virMemoryParameterPtr param = params[i];
 +val = 0;
 +param-value.ul = 0;
 +param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
 +
 +switch(i) {
 +case 0: /* fill memory hard limit here */
 +rc = virCgroupGetMemoryHardLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get memory hard limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_HARD_LIMIT) 
 == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field memory hard limit too long 
 for destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +
 +case 1: /* fill memory soft limit here */
 +rc = virCgroupGetMemorySoftLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get memory soft limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) 
 == NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field memory soft limit too long 
 for destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +   
 +case 2: /* fill swap hard limit here */
 +rc = virCgroupGetSwapHardLimit(group, val);
 +if (rc != 0) {
 +virReportSystemError(-rc, %s,
 + _(unable to get swap hard limit));
 +continue;
 +}
 +if (virStrcpyStatic(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT) == 
 NULL) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR,
 +%s, _(Field swap hard limit too long for 
 destination));
 +continue;
 +}
 +param-value.ul = val;
 +break;
 +
 +default:
 +break;
 +/* should not hit here */
 +}
 +}

  Okay, I'm not sure we actually need a loop here, but it may help
  refactoring...
I'm still having a problem with the code ignoring any error occuring in
the loop, and fixing this in the same way. If there is an error the
application *must* learn about it instead of trusting uninitialized
memory as being data !
Maybe a memset is in order actually before entering that loop to avoid
edge case problems... TODO too

 +ret = 0;
 +
 +cleanup:
 +if (group)
 +virCgroupFree(group);
 +if (vm)
 +virDomainObjUnlock(vm);
 +qemuDriverUnlock(driver);
 +return ret;
 +}
 +
  static int qemuSetSchedulerParameters(virDomainPtr dom,
virSchedParameterPtr params,
int nparams)
 @@ -12804,7 +12922,7 @@ static virDriver qemuDriver = {
  qemuDomainSnapshotDelete, /* domainSnapshotDelete */
  qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */

Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

2010-10-12 Thread Nikunj A. Dadhania 
On Tue, 12 Oct 2010 17:51:54 +0200, Daniel Veillard veill...@redhat.com wrote:
 On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
  From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
  
  V4:
  * prototype change: add unsigned int flags
  
  Driver interface for getting memory parameters, eg. hard_limit, soft_limit 
  and
  swap_hard_limit.
 
  +qemuReportError(VIR_ERR_INVALID_ARG,
  +%s, _(Invalid parameter count));
  +goto cleanup;
  +}
 
   okay, this mean the application must always call with 0 first to get
   the exact value or this will break, fine but probably need to be made
   more clear from the description in libvirt.c  TODO
 
Sure, I will take care of updating the api desc in libvirt.c, I haven't used
word always there.

  +if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) 
  {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +_(cannot find cgroup for domain %s), 
  vm-def-name);
  +goto cleanup;
  +}
  +
  +for (i = 0; i  *nparams; i++) {
  +virMemoryParameterPtr param = params[i];
  +val = 0;
  +param-value.ul = 0;
  +param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
  +
  +switch(i) {
  +case 0: /* fill memory hard limit here */
  +rc = virCgroupGetMemoryHardLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get memory hard limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, 
  VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field memory hard limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +
  +case 1: /* fill memory soft limit here */
  +rc = virCgroupGetMemorySoftLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get memory soft limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, 
  VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field memory soft limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +   
  +case 2: /* fill swap hard limit here */
  +rc = virCgroupGetSwapHardLimit(group, val);
  +if (rc != 0) {
  +virReportSystemError(-rc, %s,
  + _(unable to get swap hard limit));
  +continue;
  +}
  +if (virStrcpyStatic(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT) 
  == NULL) {
  +qemuReportError(VIR_ERR_INTERNAL_ERROR,
  +%s, _(Field swap hard limit too long 
  for destination));
  +continue;
  +}
  +param-value.ul = val;
  +break;
  +
  +default:
  +break;
  +/* should not hit here */
  +}
  +}
 
   Okay, I'm not sure we actually need a loop here, but it may help
   refactoring...
I guess this is related to my previous thinking, if nparams 
QEMU_NB_MEM_PARAM, fill only till nparams and return. But with the change of
the logic, I think loop may not be required now.

 I'm still having a problem with the code ignoring any error occuring in
 the loop, and fixing this in the same way. If there is an error the
 application *must* learn about it instead of trusting uninitialized
 memory as being data !
 Maybe a memset is in order actually before entering that loop to avoid
 edge case problems... TODO too

By TODO you mean the error handling, right?  

I am taking care of setting the values to zero currently, and it does not tell
the application whether to use this value or not.  One option could be adding
VIR_DOMAIN_MEMORY_INVALID in virMemoryParameterType and setting it in the
beginning of the loop. Comments?

Nikunj

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

[libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu

2010-10-08 Thread Nikunj A. Dadhania 
From: Nikunj A. Dadhania nik...@linux.vnet.ibm.com

V4:
* prototype change: add unsigned int flags

Driver interface for getting memory parameters, eg. hard_limit, soft_limit and
swap_hard_limit.

Signed-off-by: Nikunj A. Dadhania nik...@linux.vnet.ibm.com
---
 src/qemu/qemu_driver.c |  120 
 1 files changed, 119 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 471db39..8eaa762 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9458,6 +9458,124 @@ cleanup:
 return ret;
 }
 
+static int qemuDomainGetMemoryParameters(virDomainPtr dom,
+ virMemoryParameterPtr params,
+ int *nparams, 
+ unsigned int flags ATTRIBUTE_UNUSED)
+{
+struct qemud_driver *driver = dom-conn-privateData;
+int i;
+virCgroupPtr group = NULL;
+virDomainObjPtr vm = NULL;
+unsigned long val;
+int ret = -1;
+int rc;
+
+qemuDriverLock(driver);
+
+if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
+qemuReportError(VIR_ERR_NO_SUPPORT,
+__FUNCTION__);
+goto cleanup;
+}
+
+vm = virDomainFindByUUID(driver-domains, dom-uuid);
+
+if (vm == NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(No such domain %s), dom-uuid);
+goto cleanup;
+}
+
+if ((*nparams) == 0) {
+/* Current number of memory parameters supported by cgroups is 3
+ * FIXME: Magic number, need to see where should this go
+ */
+*nparams = 3;
+ret = 0;
+goto cleanup;
+}
+
+if ((*nparams) != 3) {
+qemuReportError(VIR_ERR_INVALID_ARG,
+%s, _(Invalid parameter count));
+goto cleanup;
+}
+
+if (virCgroupForDomain(driver-cgroup, vm-def-name, group, 0) != 0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+_(cannot find cgroup for domain %s), vm-def-name);
+goto cleanup;
+}
+
+for (i = 0; i  *nparams; i++) {
+virMemoryParameterPtr param = params[i];
+val = 0;
+param-value.ul = 0;
+param-type = VIR_DOMAIN_MEMORY_FIELD_ULLONG;
+
+switch(i) {
+case 0: /* fill memory hard limit here */
+rc = virCgroupGetMemoryHardLimit(group, val);
+if (rc != 0) {
+virReportSystemError(-rc, %s,
+ _(unable to get memory hard limit));
+continue;
+}
+if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_HARD_LIMIT) == 
NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+%s, _(Field memory hard limit too long for 
destination));
+continue;
+}
+param-value.ul = val;
+break;
+
+case 1: /* fill memory soft limit here */
+rc = virCgroupGetMemorySoftLimit(group, val);
+if (rc != 0) {
+virReportSystemError(-rc, %s,
+ _(unable to get memory soft limit));
+continue;
+}
+if (virStrcpyStatic(param-field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) == 
NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+%s, _(Field memory soft limit too long for 
destination));
+continue;
+}
+param-value.ul = val;
+break;
+   
+case 2: /* fill swap hard limit here */
+rc = virCgroupGetSwapHardLimit(group, val);
+if (rc != 0) {
+virReportSystemError(-rc, %s,
+ _(unable to get swap hard limit));
+continue;
+}
+if (virStrcpyStatic(param-field, VIR_DOMAIN_SWAP_HARD_LIMIT) == 
NULL) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR,
+%s, _(Field swap hard limit too long for 
destination));
+continue;
+}
+param-value.ul = val;
+break;
+
+default:
+break;
+/* should not hit here */
+}
+}
+ret = 0;
+
+cleanup:
+if (group)
+virCgroupFree(group);
+if (vm)
+virDomainObjUnlock(vm);
+qemuDriverUnlock(driver);
+return ret;
+}
+
 static int qemuSetSchedulerParameters(virDomainPtr dom,
   virSchedParameterPtr params,
   int nparams)
@@ -12804,7 +12922,7 @@ static virDriver qemuDriver = {
 qemuDomainSnapshotDelete, /* domainSnapshotDelete */
 qemuDomainMonitorCommand, /* qemuDomainMonitorCommand */
 qemuDomainSetMemoryParameters, /* domainSetMemoryParameters */
-NULL,