[libvirt] [PATCHv5 3/6] virNodeGetCPUTimeParameters: Implement public API

[Minoru Usui]

On Fri, 20 May 2011 11:35:20 +0100
"Daniel P. Berrange" <berrange at redhat.com> wrote:

> On Tue, May 17, 2011 at 04:02:02PM +0900, Minoru Usui wrote:
> > virNodeGetCPUTime: Implement public API
> > 
> > Signed-off-by: Minoru Usui <usui at mxm.nes.nec.co.jp>
> > ---
> >  src/libvirt.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 87 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/libvirt.c b/src/libvirt.c
> > index 5a5439d..6828e1a 100644
> > --- a/src/libvirt.c
> > +++ b/src/libvirt.c
> > @@ -4930,6 +4930,93 @@ error:
> >  }
> >  
> >  /**
> > + * virNodeGetCPUTime:
> > + * @conn: a connection object
> > + * @params: pointer to node cpu time parameter objects
> > + * @nparams: number of node cpu time parameter (this value should be same or
> > + *          less than the number of parameters supported)
> > + * @flags: currently unused, for future extension. always pass 0.
> > + *
> > + * This function provides cpu time statistics of the node.
> > + * The @params array will be filled with the values equal to the number of
> > + * parameters suggested by @nparams
> > + *
> > + * As the value of @nparams is dynamic, call the API setting @nparams to 0 and
> > + * @params as NULL, the API returns the number of parameters supported by the
> > + * HV by updating @nparams on SUCCESS. The caller should then allocate @params
> > + * array, i.e. (sizeof(@virCPUTimeParameter) * @nparams) bytes and call
> > + * the API again.
> > + *
> > + * Here is the sample code snippet:
> > + *
> > + * if ((virNodeGetCPUTimeParameters(conn, NULL, &nparams, 0) == 0) &&
> > + *     (nparams != 0)) {
> > + *     params = vshMalloc(ctl, sizeof(virCPUTimeParameter) * nparams);
> > + *     memset(params, 0, sizeof(virCPUTimeParameter) * nparams);
> > + *     if (virNodeGetCPUTimeParameters(conn, params, &nparams, 0)) {
> > + *         vshError(ctl, "%s", _("Unable to get node cpu time parameters"));
> > + *         goto error;
> > + *     }
> > + * }
> > + *
> > + * This function requires privileged access to the hypervisor. This function
> > + * expects the caller to allocate the @params.
> > + *
> > + * CPU time Statistics:
> > + *
> > + * VIR_NODE_CPU_TIME_KERNEL:
> > + *     The cumulative CPU time which spends by kernel,
> > + *     when the node booting up.(nanoseconds)
> > + * VIR_NODE_CPU_TIME_USER:
> > + *     The cumulative CPU time which spends by user processes,
> > + *     when the node booting up.(nanoseconds)
> > + * VIR_NODE_CPU_TIME_IDLE:
> > + *     The cumulative idle CPU time, when the node booting up.(nanoseconds)
> > + * VIR_NODE_CPU_TIME_IOWAIT:
> > + *     The cumulative I/O wait CPU time, when the node booting up.(nanoseconds)
> > + * VIR_NODE_CPU_TIME_UTILIZATION:
> > + *     The CPU utilization. The usage value is in percent and 100%
> > + *     represents all CPUs on the server.
> > + *
> > + * Returns -1 in case of error, 0 in case of success.
> > + */
> > +int virNodeGetCPUTimeParameters (virConnectPtr conn,
> > +                                 virCPUTimeParameterPtr params,
> > +                                 int *nparams, unsigned int flags)
> > +{
> > +    VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=%u",
> > +              conn, params, (nparams) ? *nparams : -1, flags);
> 
> '(nparams)' is redundant, just use  'nparams'

OK.
I'll change it.

> > +
> > +    virResetLastError();
> > +
> > +    if (!VIR_IS_CONNECT(conn)) {
> > +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> > +        virDispatchError(NULL);
> > +        return -1;
> > +    }
> > +
> > +    virCheckFlags(flags, 0);
> 
> The virCheckFlags call should only be in the per-hypervisor
> method implementations. This entry point needs to allow
> any/all flags to be passed through, since it does not know
> which the hypervisor will considered appropriate. So just
> delete this line

OK.
I'll fix it.

> > +
> > +    if ((nparams == NULL) || (*nparams < 0)) {
> > +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto error;
> > +    }
> > +
> > +    if (conn->driver->nodeGetCPUTimeParameters) {
> > +        int ret;
> > +        ret = conn->driver->nodeGetCPUTimeParameters (conn, params, nparams, flags);
> > +        if (ret < 0)
> > +            goto error;
> > +        return ret;
> > +    }
> > +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> > +
> > +error:
> > +    virDispatchError(conn);
> > +    return -1;
> > +}
> > +
> > +/**
> >   * virNodeGetFreeMemory:
> >   * @conn: pointer to the hypervisor connection
> >   *
> 
> The comment says this requires privileged access to the hypervisor, but
> the code is not checking for whether the readonly flag is set. IMHO this
> api is safe for readonly usage, so perhaps the comment needs changing
> to say it does not require privileged access.

OK. 
I'll change it.

-- 
Minoru Usui <usui at mxm.nes.nec.co.jp>