[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]