At Thu, 10 Apr 2014 17:49:07 +0200,
Spaces after comma.phate867@gmail.com wrote:
>
> From: Pasquale Di Rienzo <phate867@gmail.com>
>
> -Added the getNodeCpuStat binding to Libvirt class
> -Added virNodeCPUStats and CPUStat classes to support this binding
> -Added the method getCPUStats to Connect class
> ---
> AUTHORS | 1 +
> src/main/java/org/libvirt/CPUStat.java | 57 ++++++++++++++++++++++
> src/main/java/org/libvirt/Connect.java | 53 ++++++++++++++++++++
> src/main/java/org/libvirt/jna/Libvirt.java | 5 ++
> src/main/java/org/libvirt/jna/virNodeCPUStats.java | 19 ++++++++
> 5 files changed, 135 insertions(+)
> create mode 100644 src/main/java/org/libvirt/CPUStat.java
> create mode 100644 src/main/java/org/libvirt/jna/virNodeCPUStats.java
>
> diff --git a/AUTHORS b/AUTHORS
> index 07809bf..77139e5 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -16,3 +16,4 @@ Andrea Sansottera <andrea.sansottera@gmail.com>
> Stefan Majer <stefan.majer@gmail.com>
> Wido den Hollander <wido@widodh.nl>
> Eric Blake <eblake@redhat.com>
> +Pasquale Di Rienzo <phate867@gmail.com>
> diff --git a/src/main/java/org/libvirt/CPUStat.java b/src/main/java/org/libvirt/CPUStat.java
> new file mode 100644
> index 0000000..527049c
> --- /dev/null
> +++ b/src/main/java/org/libvirt/CPUStat.java
> @@ -0,0 +1,57 @@
> +package org.libvirt;
> +
> +import java.nio.charset.Charset;
> +import org.libvirt.jna.virNodeCPUStats;
> +import org.libvirt.jna.virTypedParameter;
> +
> +/**
> + * This class holds a cpu time.
> + * The tag attribute is a string of either "kernel","user","idle","iowait"
But, just mentioning the possible tags in the documentation is not
good enough. We should provide constants (preferably type safe enums)
to the user.
Furthermore, grepping through the code, there seem to be some
other constants besides the ones you have mentioned:
#define VIR_NODE_CPU_STATS_KERNEL "kernel"
#define VIR_NODE_CPU_STATS_USER "user"
#define VIR_NODE_CPU_STATS_IDLE "idle"
#define VIR_NODE_CPU_STATS_IOWAIT "iowait"
#define VIR_NODE_CPU_STATS_INTR "intr"
#define VIR_NODE_CPU_STATS_UTILIZATION "utilization"
>From a users perspective, I think it would be easier to provide an API
like:
NodeCPUStatistics stats = connection.getNodeCPUStatistics(cpuNum);
stats.kernelTime()
stats.userTime()
...
where those methods return a special value if the time is not present,
e.g. "null".
For upwards compatibility we might provide a getter having a String
parameter to retrieve yet unknown stats. But, I don't anticipate any
additions to the available node CPU stats that often or even at all.
FWIW, I'd prefer not use use a special parameter value to retrieve
the total node CPU stats, but use a telling method name, like
getTotalCPUStatistics().
Wrong indentation. Don't use tabs for indentation.
> + * while the value attribute is the actual time value
> + * @author Pasquale Di Rienzo
> + *
> + */
> +public class CPUStat {
> + public String tag;
> + public long value;
> +
Just use the Native.toString(byte[] b, String enc) method for this
> + private String createStringFromBytes(byte[] b){
> + Charset ch = Charset.forName("UTF-8");
> + int i = 0;
> + while ((i<b.length) && (b[i]!=0))
> + i++;
> +
> + return new String(b,0,i,ch);
> + }
conversion instead of open coding it here.
I'll look into making such a method available in the Library class.
> + public CPUStat(virNodeCPUStats stat){
Make this constructor package-private.
What's virTypedParameter? You don't define that class in this patch.
> + tag = createStringFromBytes(stat.field);
> + value = stat.value.longValue();
> + }
> +
> + public CPUStat(virTypedParameter stat){
> + tag = createStringFromBytes(stat.field);
> + value = stat.value.l;
> + }
As per my comment for the last review, remove the flags parameter.
> + public String getTag() {
> + return tag;
> + }
> +
> + public long getValue() {
> + return value;
> + }
> +
> + public void setTag(String tag) {
> + this.tag = tag;
> + }
> +
> + public void setValue(long val) {
> + this.value = val;
> + }
> +
> + @Override
> + public String toString() {
> + return String.format("tag:%s%nval:%d%n", tag, value);
> + }
> +}
> diff --git a/src/main/java/org/libvirt/Connect.java b/src/main/java/org/libvirt/Connect.java
> index fedc60e..d8a4ce2 100644
> --- a/src/main/java/org/libvirt/Connect.java
> +++ b/src/main/java/org/libvirt/Connect.java
> @@ -2,6 +2,8 @@ package org.libvirt;
>
> import java.util.UUID;
>
> +import org.libvirt.CPUStat;
> +import org.libvirt.LibvirtException;
> import org.libvirt.jna.ConnectionPointer;
> import org.libvirt.jna.DevicePointer;
> import org.libvirt.jna.DomainPointer;
> @@ -14,6 +16,7 @@ import org.libvirt.jna.StoragePoolPointer;
> import org.libvirt.jna.StorageVolPointer;
> import org.libvirt.jna.StreamPointer;
> import org.libvirt.jna.virConnectAuth;
> +import org.libvirt.jna.virNodeCPUStats;
> import org.libvirt.jna.virNodeInfo;
>
> import static org.libvirt.Library.libvirt;
> @@ -23,6 +26,7 @@ import static org.libvirt.ErrorHandler.processErrorIfZero;
> import com.sun.jna.Memory;
> import com.sun.jna.NativeLong;
> import com.sun.jna.Pointer;
> +import com.sun.jna.ptr.IntByReference;
> import com.sun.jna.ptr.LongByReference;
>
> /**
> @@ -207,6 +211,55 @@ public class Connect {
> }
> return processError(success);
> }
> +
> + /**
> + * This function returns statistics about the cpu, that is the time
> + * each cpu is spending in kernel time, user time, io time and idle time.
> + * Each CPUStat object refers to a particular time.
> + *
> + * Note that not all these stats are granted to be retrieved.
> + *
> + * @param the number of the cpu you want to retrieve stats from.
> + * passing -1 will make this function retrieve a mean value
> + * from all cpus the system has.
> + *
> + * @param some flags
Insert a space after //
> + * @return a cpustat object for each cpu
> + * @throws LibvirtException
> + */
> + public CPUStat[] getCPUStats(int cpuNumber,long flags) throws LibvirtException{
> + CPUStat[] stats = null;
> +
> + IntByReference nParams = new IntByReference();
> +
> + //according to libvirt reference you call this function once passing
> + //null as param paramether to get the actual stats (kernel,user,io,idle) number into the
> + //nParams reference. Generally this number would be 4, but some systems
> + //may not give all 4 times, so it is always good to call it.
s/paramethers/parameters/
The check is unnecessary since processError will throw an exception if
> + int result = libvirt.virNodeGetCPUStats(
> + VCP, cpuNumber, null, nParams, flags);
> +
> + processError(result);
> +
> + if(result == 0){//dunno if this check is actually needed
result == -1.
s/paramethers/parameters/
> + //this time we create an array to fit the number of paramethers
Likewise.
> + virNodeCPUStats[] params = new virNodeCPUStats[nParams.getValue()];
> + //and we pass it to the function
> + result = libvirt.virNodeGetCPUStats(VCP, cpuNumber , params, nParams, flags);
> + processError(result);
> +
> + //finally we parse the result in an user friendly class which does
> + //not expose libvirt's internal paramethers
> + if(result >= 0){
Space between "if" and "(" and between ")" and "{"
Spaces after "for" and after semicolon
> + stats = new CPUStat[params.length];
> + for(int i=0;i<params.length;i++)
Lots of trailing spaces here.
> + stats[i] = new CPUStat(params[i]);
> + }
> + }
> +
> + return stats;
> + }
>
Replace the tab with spaces. flags is an "unsigned int" in C, so it
> /**
> * Compares the given CPU description with the host CPU
> diff --git a/src/main/java/org/libvirt/jna/Libvirt.java b/src/main/java/org/libvirt/jna/Libvirt.java
> index 0e4c9fc..658299f 100644
> --- a/src/main/java/org/libvirt/jna/Libvirt.java
> +++ b/src/main/java/org/libvirt/jna/Libvirt.java
> @@ -1,5 +1,8 @@
> package org.libvirt.jna;
>
> +import org.libvirt.jna.ConnectionPointer;
> +import org.libvirt.jna.virNodeCPUStats;
> +
> import com.sun.jna.Callback;
> import com.sun.jna.Library;
> import com.sun.jna.Native;
> @@ -267,6 +270,8 @@ public interface Libvirt extends Library {
> int virNetworkUndefine(NetworkPointer virConnectPtr);
>
> // Node functions
> + int virNodeGetCPUStats(ConnectionPointer virConnectPtr, int cpuNum,
> + virNodeCPUStats[] stats,IntByReference nparams, long flags);
must be an "int" in Java.
It would be better to declare a constant instead of using magic numbers
> int virNodeGetInfo(ConnectionPointer virConnectPtr, virNodeInfo virNodeInfo);
> int virNodeGetCellsFreeMemory(ConnectionPointer virConnectPtr, LongByReference freeMems, int startCell,
> int maxCells);
> diff --git a/src/main/java/org/libvirt/jna/virNodeCPUStats.java b/src/main/java/org/libvirt/jna/virNodeCPUStats.java
> new file mode 100644
> index 0000000..a8f2dca
> --- /dev/null
> +++ b/src/main/java/org/libvirt/jna/virNodeCPUStats.java
> @@ -0,0 +1,19 @@
> +package org.libvirt.jna;
> +
> +import java.util.Arrays;
> +import java.util.List;
> +
> +import com.sun.jna.NativeLong;
> +import com.sun.jna.Structure;
> +
> +public class virNodeCPUStats extends Structure{
> + public byte[] field = new byte[80];
here => NODE_CPU_STATS_FIELD_LENGTH
> + public NativeLong value ;
This is an "unsigned long long" in C, so it must be a long, not
NativeLong here.
Spurious space after opening paren.
> + private static final List fields = Arrays.asList( "field", "value");
Use a generic type to avoid compiler warnings (cf. 94ec3dc0b78b)
Likewise.
> + @Override
> + protected List getFieldOrder() {
> + return fields;
> + }
> +}
Regards,
Claudio