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