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

[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 <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 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 using cgroups, not sure
the user should trust any returned values in that case.

Daniel


-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/