[libvirt] [java] [PATCH] GetNodeCpuStat binding

[Claudio Bley]

At Thu, 10 Apr 2014 17:49:07 +0200,
phate867 at gmail.com wrote:
> 
> From: Pasquale Di Rienzo <phate867 at 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 at gmail.com>
>  Stefan Majer <stefan.majer at gmail.com>
>  Wido den Hollander <wido at widodh.nl>
>  Eric Blake <eblake at redhat.com>
> +Pasquale Di Rienzo <phate867 at 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"

Spaces after comma.

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().

> + * while the value attribute is the actual time value
> + * @author Pasquale Di Rienzo
> + *
> + */
> +public class CPUStat {
> +	public String tag;
> +    public long value;
> +    

Wrong indentation. Don't use tabs for indentation.

> +    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);
> +    }

Just use the Native.toString(byte[] b, String enc) method for this
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.

> +    	tag = createStringFromBytes(stat.field);
> +    	value = stat.value.longValue();
> +    }
> +    
> +    public CPUStat(virTypedParameter stat){
> +    	tag = createStringFromBytes(stat.field);
> +		value = stat.value.l;
> +    }

What's virTypedParameter? You don't define that class in this patch.

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

As per my comment for the last review, remove the flags parameter.

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

Insert a space after //

s/paramethers/parameters/

> +    	int result = libvirt.virNodeGetCPUStats(
> +    			VCP, cpuNumber, null, nParams, flags);
> +    	
> +    	processError(result);
> +    	
> +    	if(result == 0){//dunno if this check is actually needed

The check is unnecessary since processError will throw an exception if
result == -1.

> +    		//this time we create an array to fit the number of paramethers

s/paramethers/parameters/

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

Likewise.

> +    		if(result >= 0){

Space between "if" and "(" and between ")" and "{"

> +    			stats = new CPUStat[params.length];
> +    			for(int i=0;i<params.length;i++)

Spaces after "for" and after semicolon

> +					stats[i] = new CPUStat(params[i]);
> +    		}	
> +    	}
> +    	
> +    	return stats;	
> +    }
>  

Lots of trailing spaces here.

>      /**
>       * 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);

Replace the tab with spaces. flags is an "unsigned int" in C, so it
must be an "int" in Java.

>      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];

It would be better to declare a constant instead of using magic numbers
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.

> +    private static final List fields = Arrays.asList( "field", "value");

Spurious space after opening paren.

Use a generic type to avoid compiler warnings (cf. 94ec3dc0b78b)

> +    @Override
> +    protected List getFieldOrder() {
> +        return fields;
> +    }
> +}

Likewise.

Regards,
Claudio