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 <nikunj(a)linux.vnet.ibm.com&gt; > > 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

> + 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