Re: [libvirt] [PATCHv2 1/6] virNodeGetCPUTime: Expose new API

On Fri, Apr 08, 2011 at 08:33:12PM +0900, Minoru Usui wrote: > virNodeGetCPUTime: Expose new API > > Signed-off-by: Minoru Usui <usui(a)mxm.nes.nec.co.jp&gt; > --- > include/libvirt/libvirt.h.in | 64 ++++++++++++++++++++++++++++++++++++++++++ > src/libvirt_public.syms | 5 +++ > 2 files changed, 69 insertions(+), 0 deletions(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index bd36015..154c138 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -228,6 +228,57 @@ struct _virNodeInfo { > unsigned int threads;/* number of threads per core */ > }; > > +/** > + * virNodeCpuTime: > + * > + * a virNodeCpuTime is a structure filled by virNodeGetCpuTime() and providing > + * the information for the cpu time of the node. > + */ > + > +/** > + * Cpu Time Statistics Tags: > + */ > +typedef enum { > + /* > + * The cumulative CPU time which spends by kernel, > + * when the node booting up.(in nanoseconds). > + */ > + VIR_NODE_CPU_TIME_KERNEL = 0, > + /* > + * The cumulative CPU time which spends by user processes, > + * when the node booting up.(in nanoseconds). > + */ > + VIR_NODE_CPU_TIME_USER = 1, > + /* > + * The cumulative idle CPU time, > + * when the node booting up.(in nanoseconds). > + */ > + VIR_NODE_CPU_TIME_IDLE = 2, > + /* > + * The cumulative I/O wait CPU time, > + * when the node booting up.(in nanoseconds). > + */ > + VIR_NODE_CPU_TIME_IOWAIT = 3, > + /* > + * The CPU utilization. > + * The usage value is in percent and 100% represents all CPUs on > + * the server. > + */ > + VIR_NODE_CPU_TIME_UTILIZATION = 4, > + > + /* > + * The number of statistics supported by this version of the interface. > + * To add new statistics, add them to the enum and increase this value. > + */ > + VIR_NODE_CPU_TIME_NR = 5, > +} virNodeCpuTimeTags; > + > +typedef struct _virNodeCpuTime virNodeCpuTime; > + > +struct _virNodeCpuTime { > + virNodeCpuTimeTags tag; > + unsigned long long val; > +}; NACK, the size of an enum is not defined by the C language and hence is compiler dependant. We cannot use it as a component of a structure in the API.

> /** > * virDomainSchedParameterType: > @@ -460,6 +511,14 @@ int virDomainMigrateSetMaxSpeed(virDomainPtr domain, > typedef virNodeInfo *virNodeInfoPtr; > > /** > + * virNodeCpuTimePtr: > + * > + * a virNodeCpuTimePtr is a pointer to a virNodeCpuTime structure. > + */ > + > +typedef virNodeCpuTime *virNodeCpuTimePtr; > + > +/** > * virConnectFlags > * > * Flags when opening a connection to a hypervisor > @@ -593,6 +652,11 @@ int virNodeGetInfo (virConnectPtr conn, > virNodeInfoPtr info); > char * virConnectGetCapabilities (virConnectPtr conn); > > +int virNodeGetCpuTime (virConnectPtr conn, > + virNodeCpuTimePtr stats, > + unsigned int nr_stats, > + unsigned int flags); > + I don't understand how the API is suppoed to work. Suppose you want the cumulative CPU time, how do you ask for it ? Seems you can't ! You just ask for a big stucture hoping that on return you may find it within the results. That looks broken to me. Either you make a simple API giving back an unsigned long long and you use the virNodeCpuTimeTags as the flag value int virNodeGetCpuTime(virConnectPtr conn, unsigned long long *time, unsigned int flags); and the flags indicate the value you're interested into, but in that case you have to ask multiple times. Or you make the VIR_NODE_CPU_TIME_* values a bit field, and you pass an array int virNodeGetCpuTime(virConnectPtr conn, unsigned long long *stats, unsigned int nr_stats, unsigned int flags); where flags is the logical or of the values you are interested into, and the implementation put them in order based on the VIR_NODE_CPU_TIME_* values. But in that case you must fail if one of the statistics is not available. But an API where you "go fishing" and you don't know upfront on a successful return if you got the data you're interested into sounds broken to me

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