Hi, Daniel. Thank you for your review. On Sun, 10 Apr 2011 12:58:56 +0800 Daniel Veillard <veillard@redhat.com> wrote:
On Fri, Apr 08, 2011 at 08:33:12PM +0900, Minoru Usui wrote:
virNodeGetCPUTime: Expose new API
Signed-off-by: Minoru Usui <usui@mxm.nes.nec.co.jp> --- 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.
OK. I'll change the enum to int.
/** * 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
Thank you for your useful advice. I think the API user would be better to request which values return, too. I'll change the I/F to above 2nd style.
Daniel
-- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
-- Minoru Usui <usui@mxm.nes.nec.co.jp>