That is if we would need to read/process these values too instead of just
using them.
2014-04-15 10:36 GMT+02:00 Pasquale Dir <phate...@gmail.com>:
> Working for the other corrections but I disegree with these ones:
>
> 1. This is an "unsigned long long" in C, so it must be a long, not
> NativeLong here.
> 2. flags is an "unsigned int" in C, so it
> must be an "int" in Java
>
> As in java types are signed I'd avoid to allocate the exact same memory,
> on the contrary I would allocate more space so that an high positive value
> for an unsigned C type would stay an high positive value in java (who would
> make it a negative value if space is not enough).
>
>
> 2014-04-14 14:22 GMT+02:00 Claudio Bley <cb...@av-test.de>:
>
> At Thu, 10 Apr 2014 17:49:07 +0200,
>> phate...@gmail.com wrote:
>> >
>> > From: Pasquale Di Rienzo <phate...@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.sansott...@gmail.com>
>> > Stefan Majer <stefan.ma...@gmail.com>
>> > Wido den Hollander <w...@widodh.nl>
>> > Eric Blake <ebl...@redhat.com>
>> > +Pasquale Di Rienzo <phate...@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
>>
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
