3:21 a.m.
Hi.
I sent a few mails on friday, 6th July, via gmane.org but they haven't
made it to the list yet. As I'm a subscriber now, I'm resending them
directly. Sorry for any duplicates in advance.
I'm using libvirt-java 0.4.7 and libvirt 0.9.8.
When libvirt returns an error code which is not mapped in enum
ErrorNumber, an IndexOutOfBoundsException is thrown.
I realize that the freshly released libvirt-java 0.4.8 supports all
error codes up to libvirt 0.9.12. But that doesn't fix the problem.
Would it be feasible to add a special UNKNOWN enum value?
---
diff --git a/src/main/java/org/libvirt/Error.java b/src/main/java/org/libvirt/Error.java
index 0946030..72bc698 100644
--- a/src/main/java/org/libvirt/Error.java
+++ b/src/main/java/org/libvirt/Error.java
@@ -151,9 +151,12 @@ public class Error implements Serializable {
}
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ if (ErrorNumber.values().length > vError.code)
+ code = ErrorNumber.values()[vError.code];
+ if (ErrorDomain.values().length > vError.domain)
+ domain = ErrorDomain.values()[vError.domain];
+ if (ErrorLevel.values().length > vError.level)
+ level = ErrorLevel.values()[vError.level];
message = vError.message;
str1 = vError.str1;
str2 = vError.str2;
---
Best regards,
Claudio.
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
5:31 a.m.
Hi.
I re-formatted the patch such that "git am" understands it.
Is it OK?
At Mon, 09 Jul 2012 10:21:49 +0200,
Claudio Bley wrote:
I'm using libvirt-java 0.4.7 and libvirt 0.9.8.
When libvirt returns an error code which is not mapped in enum
ErrorNumber, an IndexOutOfBoundsException is thrown.
I realize that the freshly released libvirt-java 0.4.8 supports all
error codes up to libvirt 0.9.12. But that doesn't fix the problem.
Would it be feasible to add a special UNKNOWN enum value?
-- >8 --
Subject: [PATCH] Fix IndexOutOfBoundsException for unknown error codes.
---
src/main/java/org/libvirt/Error.java | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/main/java/org/libvirt/Error.java b/src/main/java/org/libvirt/Error.java
index f185ce0..faf89ff 100644
--- a/src/main/java/org/libvirt/Error.java
+++ b/src/main/java/org/libvirt/Error.java
@@ -186,9 +186,12 @@ public class Error implements Serializable {
}
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ if (ErrorNumber.values().length > vError.code)
+ code = ErrorNumber.values()[vError.code];
+ if (ErrorDomain.values().length > vError.domain)
+ domain = ErrorDomain.values()[vError.domain];
+ if (ErrorLevel.values().length > vError.level)
+ level = ErrorLevel.values()[vError.level];
message = vError.message;
str1 = vError.str1;
str2 = vError.str2;
--
1.7.11.msysgit.0
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
3:56 p.m.
On 07/23/2012 04:31 AM, Claudio Bley wrote:
> When libvirt returns an error code which is not mapped in enum
> ErrorNumber, an IndexOutOfBoundsException is thrown.
>
> I realize that the freshly released libvirt-java 0.4.8 supports all
> error codes up to libvirt 0.9.12. But that doesn't fix the problem.
>
> Would it be feasible to add a special UNKNOWN enum value?
I think that might be better, after all. With your patch...
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ if (ErrorNumber.values().length > vError.code)
+ code = ErrorNumber.values()[vError.code];
the old version crashed, and your version leaves code as null (which is
a strict improvement, but might cause its own NullPointer issue later
on). Having an else branch that sticks in a placeholder would be nicer
to end clients to at least recognize that they are talking to a newer
server, without crashing.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:11 a.m.
At Mon, 23 Jul 2012 14:56:55 -0600,
Eric Blake wrote:
On 07/23/2012 04:31 AM, Claudio Bley wrote:
>> When libvirt returns an error code which is not mapped in enum
>> ErrorNumber, an IndexOutOfBoundsException is thrown.
>>
>> I realize that the freshly released libvirt-java 0.4.8 supports all
>> error codes up to libvirt 0.9.12. But that doesn't fix the problem.
>>
>> Would it be feasible to add a special UNKNOWN enum value?
I think that might be better, after all. With your patch...
> public Error(virError vError) {
> - code = ErrorNumber.values()[vError.code];
> - domain = ErrorDomain.values()[vError.domain];
> - level = ErrorLevel.values()[vError.level];
> + if (ErrorNumber.values().length > vError.code)
> + code = ErrorNumber.values()[vError.code];
the old version crashed, and your version leaves code as null (which is
a strict improvement, but might cause its own NullPointer issue later
on). Having an else branch that sticks in a placeholder would be nicer
to end clients to at least recognize that they are talking to a newer
server, without crashing.
Please have look at the following patch.
-- >8 --
Subject: [PATCH] Fix IndexOutOfBoundsException for unknown error
number/domain/level codes.
Remove default constructor because it does not init the object properly.
Add a special *_UNKNOWN enum value to each enum which is used when the
given enum code is out of bounds.
---
src/main/java/org/libvirt/Error.java | 42 +++++++++++++++++++++++++++++-------
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/src/main/java/org/libvirt/Error.java b/src/main/java/org/libvirt/Error.java
index f185ce0..a345b82 100644
--- a/src/main/java/org/libvirt/Error.java
+++ b/src/main/java/org/libvirt/Error.java
@@ -12,6 +12,20 @@ import org.libvirt.jna.virError;
*/
public class Error implements Serializable {
+ /**
+ * Map an integer to an enum value.
+ *
+ * @return when {@code n < values.length} return n-th item of
+ * {@code values}, otherwise the last item of array
+ * {@code values}.
+ */
+ private static final <T extends Enum<T>> T wrapToEnum(final int n, final
T[] values) {
+ assert(n >= 0 && values.length > 0);
+
+ int idx = Math.min(n, values.length - 1);
+ return values[idx];
+ }
+
public static enum ErrorDomain {
VIR_FROM_NONE, VIR_FROM_XEN, /* Error at Xen hypervisor layer */
VIR_FROM_XEND, /* Error at connection with xend daemon */
@@ -60,6 +74,11 @@ public class Error implements Serializable {
VIR_FROM_URI, /* Error from URI handling */
VIR_FROM_AUTH, /* Error from auth handling */
VIR_FROM_DBUS, /* Error from DBus */
+ VIR_FROM_UNKNOWN; /* unknown error domain (must be the last entry!) */
+
+ protected static final ErrorDomain wrap(int value) {
+ return wrapToEnum(value, values());
+ }
}
public static enum ErrorLevel {
@@ -71,7 +90,13 @@ public class Error implements Serializable {
/**
* An error
*/
- VIR_ERR_ERROR
+ VIR_ERR_ERROR,
+
+ VIR_ERR_UNKNOWN; /* must be the last entry! */
+
+ protected static final ErrorLevel wrap(int value) {
+ return wrapToEnum(value, values());
+ }
}
public static enum ErrorNumber {
@@ -161,6 +186,11 @@ public class Error implements Serializable {
VIR_ERR_MIGRATE_UNSAFE, /* Migration is not safe */
VIR_ERR_OVERFLOW, /* integer overflow */
VIR_ERR_BLOCK_COPY_ACTIVE, /* action prevented by block copy job */
+ VIR_ERR_UNKNOWN; /* unknown error (must be the last entry!) */
+
+ protected static final ErrorNumber wrap(int value) {
+ return wrapToEnum(value, values());
+ }
}
/**
@@ -181,14 +211,10 @@ public class Error implements Serializable {
int int2;
NetworkPointer VNP; /* Deprecated */
- public Error() {
-
- }
-
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ code = code.wrap(vError.code);
+ domain = domain.wrap(vError.domain);
+ level = level.wrap(vError.level);
message = vError.message;
str1 = vError.str1;
str2 = vError.str2;
--
1.7.11.msysgit.0
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
5:57 p.m.
On 08/01/2012 08:11 AM, Claudio Bley wrote:
At Mon, 23 Jul 2012 14:56:55 -0600,
> the old version crashed, and your version leaves code as null
(which is
> a strict improvement, but might cause its own NullPointer issue later
> on). Having an else branch that sticks in a placeholder would be nicer
> to end clients to at least recognize that they are talking to a newer
> server, without crashing.
Please have look at the following patch.
-- >8 --
Subject: [PATCH] Fix IndexOutOfBoundsException for unknown error
number/domain/level codes.
Remove default constructor because it does not init the object properly.
Add a special *_UNKNOWN enum value to each enum which is used when the
given enum code is out of bounds.
+ /**
+ * Map an integer to an enum value.
+ *
+ * @return when {@code n < values.length} return n-th item of
+ * {@code values}, otherwise the last item of array
+ * {@code values}.
+ */
+ private static final <T extends Enum<T>> T wrapToEnum(final int n, final
T[] values) {
Wow, Java has changed since I last programmed in it (I haven't
programmed Java since 1.4 when 'assert' was added, although I was at
least following the development of Java 1.5 at the time enough to
understand this syntax - I guess that would be 8 or so years ago?).
@@ -71,7 +90,13 @@ public class Error implements Serializable {
/**
* An error
*/
- VIR_ERR_ERROR
+ VIR_ERR_ERROR,
+
+ VIR_ERR_UNKNOWN; /* must be the last entry! */
Is this...
+
+ protected static final ErrorLevel wrap(int value) {
+ return wrapToEnum(value, values());
+ }
}
public static enum ErrorNumber {
@@ -161,6 +186,11 @@ public class Error implements Serializable {
VIR_ERR_MIGRATE_UNSAFE, /* Migration is not safe */
VIR_ERR_OVERFLOW, /* integer overflow */
VIR_ERR_BLOCK_COPY_ACTIVE, /* action prevented by block copy job */
+ VIR_ERR_UNKNOWN; /* unknown error (must be the last entry!) */
...and this common use of a name among two different enums going to bite
us? If so, I'd recommend munging the names, maybe:
VIR_ERR_LEVEL_UNKNOWN vs. VIR_ERR_NUMBER_UNKNOWN.
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ code = code.wrap(vError.code);
+ domain = domain.wrap(vError.domain);
+ level = level.wrap(vError.level);
Slick. Depending on your answer to my question on whether a rename
makes sense, I don't mind pushing this or a slightly different v2. When
using 'git send-email', it helps to use '--subject-prefix="libvirt-java
PATCHv2" ' for the repost to make it obvious that it is a revision of an
earlier post.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:55 a.m.
New subject: [libvirt] [PATCH v3] [libvirt-java] Fix Array IndexOutOfBoundsException for unknown error codes
At Wed, 01 Aug 2012 16:57:33 -0600,
Eric Blake wrote:
On 08/01/2012 08:11 AM, Claudio Bley wrote:
> At Mon, 23 Jul 2012 14:56:55 -0600,
>> the old version crashed, and your version leaves code as null (which is
>> a strict improvement, but might cause its own NullPointer issue later
>> on). Having an else branch that sticks in a placeholder would be nicer
>> to end clients to at least recognize that they are talking to a newer
>> server, without crashing.
>
> Please have look at the following patch.
>
> -- >8 --
> Subject: [PATCH] Fix IndexOutOfBoundsException for unknown error
> number/domain/level codes.
>
> Remove default constructor because it does not init the object properly.
>
> Add a special *_UNKNOWN enum value to each enum which is used when the
> given enum code is out of bounds.
> + /**
> + * Map an integer to an enum value.
> + *
> + * @return when {@code n < values.length} return n-th item of
> + * {@code values}, otherwise the last item of array
> + * {@code values}.
> + */
> + private static final <T extends Enum<T>> T wrapToEnum(final int n,
final T[] values) {
Wow, Java has changed since I last programmed in it (I haven't
programmed Java since 1.4 when 'assert' was added, although I was at
least following the development of Java 1.5 at the time enough to
understand this syntax - I guess that would be 8 or so years ago?).
Indeed, they try to shoehorn every kind of feature into the language,
nowadays. ;)
But, as I'm looking at the code now, I notice that it is unnecessarily
complex (originally I was trying to make it more succinct, but Java
wouldn't let me).
I simplified the code, renaming the method / fixing the documentation
and made v3 of the patch.
> @@ -71,7 +90,13 @@ public class Error implements Serializable {
> /**
> * An error
> */
> - VIR_ERR_ERROR
> + VIR_ERR_ERROR,
> +
> + VIR_ERR_UNKNOWN; /* must be the last entry! */
Is this...
> +
> + protected static final ErrorLevel wrap(int value) {
> + return wrapToEnum(value, values());
> + }
> }
>
> public static enum ErrorNumber {
> @@ -161,6 +186,11 @@ public class Error implements Serializable {
> VIR_ERR_MIGRATE_UNSAFE, /* Migration is not safe */
> VIR_ERR_OVERFLOW, /* integer overflow */
> VIR_ERR_BLOCK_COPY_ACTIVE, /* action prevented by block copy job */
> + VIR_ERR_UNKNOWN; /* unknown error (must be the last entry!) */
...and this common use of a name among two different enums going to bite
us?
No, not at all. Enums are type-safe in Java and enums have their own
namespace; one always has to specify the enum name when referring to
an enum constant. (except in case statements where the compiler can
infer the type of the enum constant)
Rather, I would /very/ much like these superfluous prefixes of all enum
constants to be removed from the libvirt-java interface. They're of no
use really, despite adding to the code bloat.
When using 'git send-email', it helps to use
'--subject-prefix="libvirt-java PATCHv2" ' for the repost to make it
obvious that it is a revision of an earlier post.
OK, thanks for the tip.
-- >8 --
Subject: [libvirt-java PATCHv3] Fix IndexOutOfBoundsException for
unknown error number/domain/level
codes.
Remove default constructor because it does not init the object properly.
Add a special *_UNKNOWN enum value to each enum which is used when the
given enum code is out of bounds.
---
src/main/java/org/libvirt/Error.java | 43 +++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 8 deletions(-)
diff --git a/src/main/java/org/libvirt/Error.java b/src/main/java/org/libvirt/Error.java
index f185ce0..e3c3246 100644
--- a/src/main/java/org/libvirt/Error.java
+++ b/src/main/java/org/libvirt/Error.java
@@ -12,6 +12,21 @@ import org.libvirt.jna.virError;
*/
public class Error implements Serializable {
+ /**
+ * Returns the element of the given array at the specified index,
+ * or the last element of the array if the index is not less than
+ * {@code values.length}.
+ *
+ * @return n-th item of {@code values} when {@code n <
+ * values.length}, otherwise the last item of {@code values}.
+ */
+ private static final <T> T safeElementAt(final int n, final T[] values) {
+ assert(n >= 0 && values.length > 0);
+
+ int idx = Math.min(n, values.length - 1);
+ return values[idx];
+ }
+
public static enum ErrorDomain {
VIR_FROM_NONE, VIR_FROM_XEN, /* Error at Xen hypervisor layer */
VIR_FROM_XEND, /* Error at connection with xend daemon */
@@ -60,6 +75,11 @@ public class Error implements Serializable {
VIR_FROM_URI, /* Error from URI handling */
VIR_FROM_AUTH, /* Error from auth handling */
VIR_FROM_DBUS, /* Error from DBus */
+ VIR_FROM_UNKNOWN; /* unknown error domain (must be the last entry!) */
+
+ protected static final ErrorDomain wrap(int value) {
+ return safeElementAt(value, values());
+ }
}
public static enum ErrorLevel {
@@ -71,7 +91,13 @@ public class Error implements Serializable {
/**
* An error
*/
- VIR_ERR_ERROR
+ VIR_ERR_ERROR,
+
+ VIR_ERR_UNKNOWN; /* must be the last entry! */
+
+ protected static final ErrorLevel wrap(int value) {
+ return safeElementAt(value, values());
+ }
}
public static enum ErrorNumber {
@@ -161,6 +187,11 @@ public class Error implements Serializable {
VIR_ERR_MIGRATE_UNSAFE, /* Migration is not safe */
VIR_ERR_OVERFLOW, /* integer overflow */
VIR_ERR_BLOCK_COPY_ACTIVE, /* action prevented by block copy job */
+ VIR_ERR_UNKNOWN; /* unknown error (must be the last entry!) */
+
+ protected static final ErrorNumber wrap(int value) {
+ return safeElementAt(value, values());
+ }
}
/**
@@ -181,14 +212,10 @@ public class Error implements Serializable {
int int2;
NetworkPointer VNP; /* Deprecated */
- public Error() {
-
- }
-
public Error(virError vError) {
- code = ErrorNumber.values()[vError.code];
- domain = ErrorDomain.values()[vError.domain];
- level = ErrorLevel.values()[vError.level];
+ code = code.wrap(vError.code);
+ domain = domain.wrap(vError.domain);
+ level = level.wrap(vError.level);
message = vError.message;
str1 = vError.str1;
str2 = vError.str2;
--
1.7.11.msysgit.0
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
9:39 a.m.
New subject: [libvirt] [PATCH v3] [libvirt-java] Fix Array IndexOutOfBoundsException for unknown error codes
On 08/09/2012 07:55 AM, Claudio Bley wrote:
>> + VIR_ERR_UNKNOWN; /* unknown error (must be the last
entry!) */
>
> ...and this common use of a name among two different enums going to bite
> us?
No, not at all. Enums are type-safe in Java and enums have their own
namespace; one always has to specify the enum name when referring to
an enum constant. (except in case statements where the compiler can
infer the type of the enum constant)
Good.
Rather, I would /very/ much like these superfluous prefixes of all enum
constants to be removed from the libvirt-java interface. They're of no
use really, despite adding to the code bloat.
Indeed - the whole point of bindings in OO languages is to remove the
superfluous prefixes used for namespacing in C when we can instead use
the object oriented namespacing; ErrorDomain.FROM_QEMU reads much better
than ErrorDomain.VIR_FROM_QEMU. A followup patch to alter the
namespacing might be nice (but _then_ you have to worry about
back-compat to existing clients; can you have two enum names, FROM_QEMU
and VIR_FROM_QEMU, that both share the same numeric value?)
-- >8 --
Subject: [libvirt-java PATCHv3] Fix IndexOutOfBoundsException for
unknown error number/domain/level
codes.
'git am' couldn't parse this properly, and I had to amend the commit to
trim out the rest of your message (not a severe issue, but I'm not sure
why things didn't quite go like normal).
Remove default constructor because it does not init the object properly.
Add a special *_UNKNOWN enum value to each enum which is used when the
given enum code is out of bounds.
---
src/main/java/org/libvirt/Error.java | 43 +++++++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 8 deletions(-)
ACK and pushed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:04 a.m.
New subject: [libvirt] [PATCH v3] [libvirt-java] Fix Array IndexOutOfBoundsException for unknown error codes
At Thu, 09 Aug 2012 08:39:39 -0600,
Eric Blake wrote:
> Rather, I would /very/ much like these superfluous prefixes of all enum
> constants to be removed from the libvirt-java interface. They're of no
> use really, despite adding to the code bloat.
Indeed - the whole point of bindings in OO languages is to remove the
superfluous prefixes used for namespacing in C when we can instead use
the object oriented namespacing; ErrorDomain.FROM_QEMU reads much better
than ErrorDomain.VIR_FROM_QEMU. A followup patch to alter the
namespacing might be nice (but _then_ you have to worry about
back-compat to existing clients; can you have two enum names, FROM_QEMU
and VIR_FROM_QEMU, that both share the same numeric value?)
Well, no -- and yes. Enums in Java have an ordinal value automatically
attached to them, but they can have arbitrary other attributes also.
But, another thing I noticed is that quite a few Java methods return
an int when they really should return a boolean (throwing an exception
if the libvirt function failed).
Fixing this would also change the API. So, it's probably a good idea
bumping the minor version number of libvirt-java and just fix it once
and for all times instead of inventing some workaround now?
> -- >8 --
> Subject: [libvirt-java PATCHv3] Fix IndexOutOfBoundsException for
> unknown error number/domain/level
> codes.
'git am' couldn't parse this properly, and I had to amend the commit to
trim out the rest of your message (not a severe issue, but I'm not sure
why things didn't quite go like normal).
Sorry for the inconvenience, maybe it's because I'm on Windows... I'll
try to test it before sending it off next time.
ACK and pushed.
Thanks!
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
4488
days inactive
4519
days old
7 comments
2 participants
participants (2)
-
Claudio Bley -
Eric Blake