At Sat, 5 Apr 2014 21:01:31 +0200,
phate867@xxxxxxxxx wrote:
>
> From: pasquale <pasquale@pasquale-VPCEB1A4E>
>
> -Added the following bindings:
> --virDomainGetCpuStats
> --virNodeGetCpuStats
> --virDomainShutdownFlags
> --virDomainDestroyFlags
>
> -Added following classes
> --org/libvirt/CPUStatistic giving cpu stats in user friendly form
> --org/libvirt/jna/VirUnion binding
> --org/libvirt/jna/virDomainCPUStats type
> --org/libvirt/jna/virNodeCPUStats type
> --org/libvirt/jna/virTypedParameter biding
You're mixing a lot of different things in this commit.
We want to have all the changes in small logical steps, not as a big
patch blob.
> ---
> src/main/java/org/libvirt/CPUStatistic.java | 61 +++
> src/main/java/org/libvirt/Connect.java | 137 +++++-
> src/main/java/org/libvirt/Device.java | 57 ++-
> src/main/java/org/libvirt/Domain.java | 501 +++++++++++++++------
> .../java/org/libvirt/DomainInterfaceStats.java | 2 +-
> src/main/java/org/libvirt/DomainSnapshot.java | 31 +-
> src/main/java/org/libvirt/Error.java | 409 ++++++-----------
> src/main/java/org/libvirt/ErrorHandler.java | 54 +--
> src/main/java/org/libvirt/Interface.java | 65 +--
> src/main/java/org/libvirt/Library.java | 4 +-
> src/main/java/org/libvirt/Network.java | 61 ++-
> src/main/java/org/libvirt/NetworkFilter.java | 43 +-
> src/main/java/org/libvirt/Secret.java | 67 ++-
> src/main/java/org/libvirt/StoragePool.java | 106 +++--
> src/main/java/org/libvirt/StorageVol.java | 68 +--
> src/main/java/org/libvirt/Stream.java | 71 ++-
> src/main/java/org/libvirt/jna/Libvirt.java | 42 +-
> src/main/java/org/libvirt/jna/VirUnion.java | 14 +
> src/main/java/org/libvirt/jna/virConnectAuth.java | 4 +-
> .../java/org/libvirt/jna/virConnectCredential.java | 4 +-
> .../java/org/libvirt/jna/virDomainBlockInfo.java | 4 +-
> .../java/org/libvirt/jna/virDomainBlockStats.java | 4 +-
> .../java/org/libvirt/jna/virDomainCPUStats.java | 21 +
> src/main/java/org/libvirt/jna/virDomainInfo.java | 4 +-
> .../org/libvirt/jna/virDomainInterfaceStats.java | 4 +-
> .../java/org/libvirt/jna/virDomainJobInfo.java | 4 +-
> .../java/org/libvirt/jna/virDomainMemoryStats.java | 4 +-
> src/main/java/org/libvirt/jna/virError.java | 4 +-
> src/main/java/org/libvirt/jna/virNodeCPUStats.java | 19 +
> src/main/java/org/libvirt/jna/virNodeInfo.java | 4 +-
> .../java/org/libvirt/jna/virSchedParameter.java | 4 +-
> .../java/org/libvirt/jna/virStoragePoolInfo.java | 4 +-
> .../java/org/libvirt/jna/virStorageVolInfo.java | 4 +-
> .../java/org/libvirt/jna/virTypedParameter.java | 22 +
> src/main/java/org/libvirt/jna/virVcpuInfo.java | 4 +-
> 35 files changed, 1194 insertions(+), 717 deletions(-)
> create mode 100644 src/main/java/org/libvirt/CPUStatistic.java
> create mode 100644 src/main/java/org/libvirt/jna/VirUnion.java
> create mode 100644 src/main/java/org/libvirt/jna/virDomainCPUStats.java
> create mode 100644 src/main/java/org/libvirt/jna/virNodeCPUStats.java
> create mode 100644 src/main/java/org/libvirt/jna/virTypedParameter.java
>
> diff --git a/src/main/java/org/libvirt/CPUStatistic.java b/src/main/java/org/libvirt/CPUStatistic.java
> new file mode 100644
> index 0000000..4cd6f09
> --- /dev/null
> +++ b/src/main/java/org/libvirt/CPUStatistic.java
> @@ -0,0 +1,61 @@
> +package org.libvirt;
> +
> +import java.nio.charset.Charset;
> +import org.libvirt.jna.virNodeCPUStats;
> +import org.libvirt.jna.virTypedParameter;
> +
> +public class CPUStatistic {
> + public String tag;
> + public long val;
Wrong indentation.
> + //valid for host cpu stats
> + public static final int KERNEL_H = 0;
> + public static final int USER_H = 1;
> + public static final int IDLE_H = 2;
> + public static final int IOWAIT_H = 3;
> +
> + //valid for guest cpu stats
> + public static final int CPU_D = 0;
> + public static final int USER_D = 1;
> + public static final int SYSTEM_D = 2;
What are those constants used for? We prefere to use enums, in general.
> + private String createStringFromBytes(byte[] b){
There's already a utility function in the Library class doing this
conversion.
> + public CPUStatistic(virNodeCPUStats stat){
> + tag = createStringFromBytes(stat.tag);
> + val = stat.val;
> + }
This constructor should not be public, as it exposes a private
implementation detail (the parameter).
> + public CPUStatistic(virTypedParameter stat){
Dito.
> diff --git a/src/main/java/org/libvirt/Connect.java b/src/main/java/org/libvirt/Connect.java
> index fedc60e..07285f9 100644
> --- a/src/main/java/org/libvirt/Connect.java
> +++ b/src/main/java/org/libvirt/Connect.java
> @@ -14,15 +14,15 @@ 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;
> -import static org.libvirt.ErrorHandler.processError;
> -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;
>
> /**
> @@ -31,7 +31,7 @@ import com.sun.jna.ptr.LongByReference;
> *
> * @author stoty
> */
> -public class Connect {
> +public class Connect implements AutoCloseable{
>
> /**
> * Get the version of a connection.
> @@ -82,6 +82,7 @@ public class Connect {
> */
> public static void setErrorCallback(Libvirt.VirErrorCallback callback) throws LibvirtException {
> Libvirt.INSTANCE.virSetErrorFunc(null, callback);
> + ErrorHandler.processError(Libvirt.INSTANCE);
> }
>
> /**
> @@ -121,6 +122,7 @@ public class Connect {
> VCP = libvirt.virConnectOpen(uri);
> // Check for an error
> processError(VCP);
> + ErrorHandler.processError(Libvirt.INSTANCE);
NACK to this change as that is what has been committed lastly in an
effort to clean up the error handling. Why did you change it back? Did
you perhaps based your changes on another commit and forgot to rebase?
> @@ -195,7 +199,7 @@ public class Connect {
> * @throws LibvirtException
> * @return number of remaining references (>= 0)
> */
> - public int close() throws LibvirtException {
> + public void close() throws LibvirtException {
When you change a return type of some method, you're changing the API,
which usually is a problem because it breaks compatibility with former
versions. So, we cannot simply do this unless there're good reasons
for the breakage.
> int success = 0;
> if (VCP != null) {
> success = libvirt.virConnectClose(VCP);
> @@ -205,7 +209,8 @@ public class Connect {
> // it's called with a null virConnectPointer
> VCP = null;
> }
> - return processError(success);
> +
> + processError(success);
Um, since you removed the static imports of
org.libvirt.ErrorHandler.processError this cannot even compile.
I guess you really forgot to rebase. You should do a "git fetch"
followed by a "git pull --rebase" before sending out patches (see
http://libvirt.org/hacking.html#patches).
> diff --git a/src/main/java/org/libvirt/Domain.java b/src/main/java/org/libvirt/Domain.java
> index a217733..b515b4b 100644
> --- a/src/main/java/org/libvirt/Domain.java
> +++ b/src/main/java/org/libvirt/Domain.java
> @@ -3,7 +3,6 @@ package org.libvirt;
> import org.libvirt.jna.DomainPointer;
> import org.libvirt.jna.DomainSnapshotPointer;
> import org.libvirt.jna.Libvirt;
> -import org.libvirt.jna.SizeT;
> import org.libvirt.jna.virDomainBlockInfo;
> import org.libvirt.jna.virDomainBlockStats;
> import org.libvirt.jna.virDomainInfo;
> @@ -11,10 +10,10 @@ import org.libvirt.jna.virDomainInterfaceStats;
> import org.libvirt.jna.virDomainJobInfo;
> import org.libvirt.jna.virDomainMemoryStats;
> import org.libvirt.jna.virSchedParameter;
> +import org.libvirt.jna.virTypedParameter;
> import org.libvirt.jna.virVcpuInfo;
> +
> import static org.libvirt.Library.libvirt;
> -import static org.libvirt.ErrorHandler.processError;
> -import static org.libvirt.ErrorHandler.processErrorIfZero;
>
> import com.sun.jna.Native;
> import com.sun.jna.NativeLong;
> @@ -33,25 +32,25 @@ public class Domain {
> public static final int BYTES = 1;
> }
>
> - static final class CreateFlags {
> - static final int VIR_DOMAIN_NONE = 0;
> - static final int VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE = (1 << 0); /* Restore or alter
> + public static final class CreateFlags {
NACK to this change as I'm planning on using an enum for those
flags. (That would need to be a commit on its own, anyway.)
> @@ -475,6 +483,41 @@ public class Connect {
> Library.free(ptr);
> }
> }
> +
> + /**
> + * This functions gives a % of the load on the host cpu system.
> + * In the case of a multicore processor it gives a load average.
> + * In order to do so it makes two sequential measurements, according
> + * to the interval parameter.
> + *
> + *
> + * @param interval the interval,in ms, between the two measurations.
> + * @return
> + * @throws LibvirtException
> + */
> + public float getCPUOverallUsage(long interval) throws LibvirtException{
> + float result = 0;
> + CPUStatistic[] st1 = getCPUStats(-1, 0);
> + try {
> + Thread.sleep(interval);
I don't think a Thread sleep is a viable thing to do in a library
function.
> + CPUStatistic[] st2 = getCPUStats(-1, 0);
> +
> + for(int i=0;i<st2.length;i++)
> + st2[i].val -= st1[i].val;
Is there a guarantee that the values will be delivered in the same
order so that you always subtract the corresponding values?
> + long sum = 0;
> + for(int i=0;i<st2.length;i++)
> + sum +=st2[i].val;
> +
> + result = 100.0f - (st2[CPUStatistic.IDLE_H].val*100.0f/sum);
You're relying on a specific ordering of the st2 array here too. What
happens if there are more or less statistic values returned by
getCPUStats? That's not going to work.
> @@ -1130,6 +1181,65 @@ public class Connect {
> }
>
> /**
> + * call the error handling logic. Should be called after every libvirt call
> + *
> + * @throws LibvirtException
> + */
> + protected void processError() throws LibvirtException {
> + ErrorHandler.processError(libvirt);
> + }
> +
> + public CPUStatistic[] getCPUStats(int number,long flags) throws LibvirtException{
Since the virNodeGetCPUStats function does not support any flags, just
drop the flags parameter for now. The function can be overloaded later
if upstream actually introduces a flag.
> + CPUStatistic[] stats = null;
Wrong indentation. Use 4 spaces per indentation level, no tabs.
> + IntByReference nParams = new IntByReference();
> +
> + int result = libvirt.virNodeGetCPUStats(
> + VCP, number, null, nParams, flags);
> + processError();
Just use processError(result) here.
I'm stopping the review here because of the problems
mentioned. Looking forward for a v2, though.
Please use '[java]' in your subject in order to indicate the project
you send patches for. Use
"git config format.subjectprefix 'java] [PATCH'" in your local
repository to let git automatically generate this for you.
Regards,
Claudio
--
BSc (Comp) Claudio Bley - Principal Software Engineer
AV-TEST GmbH, Klewitzstr. 7, 39112 Magdeburg, Germany
Phone: +49 391 6075460, Fax: +49 391 6075469
Web: <http://www.av-test.org>
* https://twitter.com/avtestorg * https://facebook.com/avtestorg *
* https://plus.google.com/100383867141221115206/ *
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
Our services shall be effected on the basis of the General Terms
and Conditions of AV-TEST GmbH, which are accessible under
<http://www.av-test.org/en/av-test/terms-and-conditions/> or
obtainable upon request.
Unsere Leistungen erfolgen auf der Grundlage der Allgemeinen
Geschäftsbedingungen der AV-TEST GmbH, die unter
<http://www.av-test.org/av-test/agb/> abrufbar sind oder auf
Anfrage übersandt werden.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list