[libvirt] [PATCHv2 3/6] virNodeGetCPUTime: Implement public API

[Minoru Usui]

On Tue, 19 Apr 2011 11:53:31 +0100
"Daniel P. Berrange" <berrange at redhat.com> wrote:

> On Fri, Apr 08, 2011 at 08:35:12PM +0900, Minoru Usui wrote:
> > virNodeGetCPUTime: Implement public API
> > 
> > Signed-off-by: Minoru Usui <usui at mxm.nes.nec.co.jp>
> > ---
> >  src/libvirt.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 70 insertions(+), 0 deletions(-)
> > 
> > diff --git a/src/libvirt.c b/src/libvirt.c
> > index 8be18d4..a65bfc6 100644
> > --- a/src/libvirt.c
> > +++ b/src/libvirt.c
> > @@ -4260,6 +4260,76 @@ error:
> >  }
> >  
> >  /**
> > + * virNodeGetCpuTime:
> > + * @stats: nr_stats-sized array of stat structures (returned)
> > + * @nr_stats: number of cpu time statistics requested of the node.
> > + * @flags: unused, always pass 0
> > + *
> > + * This function provides cpu time statistics of the node.
> > + *
> > + * Up to 'nr_stats' elements of 'stats' will be populated with cpu time statistics
> > + * of the node.  Only statistics supported by the driver, and this version of
> > + * libvirt will be returned.
> > + *
> > + * 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: The number of stats provided or -1 in case of failure.
> > + */
> > +int virNodeGetCpuTime (virConnectPtr conn, virNodeCpuTimePtr stats,
> > +                          unsigned int nr_stats, unsigned int flags)
> > +{
> > +    unsigned long nr_stats_ret = 0;
> 
> This ought to have been an 'int' to match the function return
> type, and better to declare it later in the block where it was
> first used.

Ouch. I'll change it.

> > +
> > +    VIR_DEBUG("conn=%p, stats=%p, nr_stats=%u", conn, stats, nr_stats);
> > +
> > +    virResetLastError();
> > +
> > +    if (!VIR_IS_CONNECT (conn)) {
> > +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> > +        virDispatchError(NULL);
> > +        return 0;
> > +    }
> > +
> > +    if (flags != 0) {
> > +        virLibConnError(VIR_ERR_INVALID_ARG, _("flags must be zero"));
> > +        goto error;
> > +    }
> 
> Instead of this, use the standard macro we have:
> 
>    virCheckFlags(flags, 0);

OK. I'll change it.

> > +    if (!stats || nr_stats == 0)
> > +        return 0;
> > +
> > +    if (nr_stats > VIR_NODE_CPU_TIME_NR)
> > +        nr_stats = VIR_NODE_CPU_TIME_NR;
> 
> If we're following the pattern of virDomainGetMemoryParameters/BlkioParameters
> then we should have passed in a pointer to nr_stats. It would be allowed to
> be zero, in which case the hypervisor would initialize it with the required
> size for the number of parameters.
> 
> eg, the signature would be
> 
> int
> virNodeGetCpuParameters(virConnectPtr conn,
>                         virNodeCpuParameterPtr params,
>                         int *nparams, unsigned int flags)
> 
> and then check
> 
>     if ((nparams == NULL) || (*nparams < 0)) {
>         virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>         goto error;
>     }

I'll rebase these patches, if it's better to be based on 
virDomainGetMemoryParameters()/BlkioParameters()?

> > +
> > +    if (conn->driver->nodeGetCpuTime) {
> > +        nr_stats_ret = conn->driver->nodeGetCpuTime (conn, stats, nr_stats, flags);
> > +        if (nr_stats_ret == -1)
> > +            goto error;
> > +        return nr_stats_ret;
> > +    }
> > +
> > +    virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> > +
> > +error:
> > +    virDispatchError(conn);
> > +    return -1;
> > +}
> > +
> > +/**
> 
> 
> 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 :|


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