Re: [libvirt] [PATCH v4 07/13] Implement driver interface domainGetMemoryParamters for QEmu
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
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
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
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,