[libvirt] [PATCH] [libvirt-java] Fix Array IndexOutOfBoundsException for unknown error codes
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
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
participants (2)
-
Claudio Bley -
Eric Blake