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

[Daniel P. Berrange]

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'

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

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


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|