[libvirt] [java] [PATCH] GetNodeCpuStat binding
Pasquale Dir
phate867 at gmail.com
Tue Apr 15 08:36:02 UTC 2014
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 <cbley at av-test.de>:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140415/ada6eda7/attachment-0001.htm>
More information about the libvir-list
mailing list