<div dir="ltr">That is if we would need to read/process these values too instead of just using them.</div><div class="gmail_extra"> <div class="gmail_quote">2014-04-15 10:36 GMT+02:00 Pasquale Dir <<a href="mailto:phate867@gmail.com" target="_blank">phate867@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Working for the other corrections but I disegree with these ones:<div> </div><div>1. This is an "unsigned long long" in C, so it must be a long, not</div> NativeLong here.<div>2. flags is an "unsigned int" in C, so it</div> <div class=""> must be an "int" in Java<div> </div></div><div>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).</div> </div><div class="gmail_extra"> <div class="gmail_quote">2014-04-14 14:22 GMT+02:00 Claudio Bley <<a href="mailto:cbley@av-test.de" target="_blank">cbley@av-test.de</a>>:<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> At Thu, 10 Apr 2014 17:49:07 +0200, <div><div><a href="mailto:phate867@gmail.com" target="_blank">phate867@gmail.com</a> wrote: > > From: Pasquale Di Rienzo <<a href="mailto:phate867@gmail.com" target="_blank">phate867@gmail.com</a>> > > -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 <<a href="mailto:andrea.sansottera@gmail.com" target="_blank">andrea.sansottera@gmail.com</a>> > Stefan Majer <<a href="mailto:stefan.majer@gmail.com" target="_blank">stefan.majer@gmail.com</a>> > Wido den Hollander <<a href="mailto:wido@widodh.nl" target="_blank">wido@widodh.nl</a>> > Eric Blake <<a href="mailto:eblake@redhat.com" target="_blank">eblake@redhat.com</a>> > +Pasquale Di Rienzo <<a href="mailto:phate867@gmail.com" target="_blank">phate867@gmail.com</a>> > 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" </div></div>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(). <div> > + * while the value attribute is the actual time value > + * @author Pasquale Di Rienzo > + * > + */ > +public class CPUStat { > + public String tag; > + public long value; > + </div>Wrong indentation. Don't use tabs for indentation. <div> > + 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); > + } </div>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. <div> > + tag = createStringFromBytes(stat.field); > + value = stat.value.longValue(); > + } > + > + public CPUStat(virTypedParameter stat){ > + tag = createStringFromBytes(stat.field); > + value = stat.value.l; > + } </div>What's virTypedParameter? You don't define that class in this patch. <div><div> > + 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 </div></div>As per my comment for the last review, remove the flags parameter. <div> > + * @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. </div>Insert a space after // s/paramethers/parameters/ <div> > + int result = libvirt.virNodeGetCPUStats( > + VCP, cpuNumber, null, nParams, flags); > + > + processError(result); > + > + if(result == 0){//dunno if this check is actually needed </div>The check is unnecessary since processError will throw an exception if result == -1. <div> > + //this time we create an array to fit the number of paramethers </div>s/paramethers/parameters/ <div> > + 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 </div>Likewise. > + if(result >= 0){ Space between "if" and "(" and between ")" and "{" <div> > + stats = new CPUStat[params.length]; > + for(int i=0;i<params.length;i++) </div>Spaces after "for" and after semicolon <div> > + stats[i] = new CPUStat(params[i]); > + } > + } > + > + return stats; > + } > </div>Lots of trailing spaces here. <div> > /** > * 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); </div>Replace the tab with spaces. flags is an "unsigned int" in C, so it must be an "int" in Java. <div> > 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]; </div>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. <div> > + private static final List fields = Arrays.asList( "field", "value"); </div>Spurious space after opening paren. Use a generic type to avoid compiler warnings (cf. 94ec3dc0b78b) <div> > + @Override > + protected List getFieldOrder() { > + return fields; > + } > +} </div>Likewise. Regards, Claudio </blockquote></div></div></div> </div> </blockquote></div> </div>