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

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 17:51:54 +0200, Daniel Veillard <veillard at redhat.com> wrote:
> On Fri, Oct 08, 2010 at 05:45:55PM +0530, Nikunj A. Dadhania wrote:
> > From: Nikunj A. Dadhania <nikunj at 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