On Fri, 20 May 2011 11:35:20 +0100
"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Tue, May 17, 2011 at 04:02:02PM +0900, Minoru Usui wrote:
> > virNodeGetCPUTime: Implement public API
> >
> > Signed-off-by: Minoru Usui <usui@xxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list