BUG-3129 NPE in flowHash

 - tableId and priority are mandatory
 - flowCookie will fallback to default (0) value

Change-Id: I9bf082178847544f1672b294b5461ea192e6b16f
Signed-off-by: Martin Bobak <mbobak@cisco.com>

diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/OFConstants.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/OFConstants.java
index b0de6ddf2fe1326d9e266d52aada18ed9deb9afd..9c9ee5d252989f4a0af452b086d63265a98c1924 100644 (file)
--- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/OFConstants.java
+++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/OFConstants.java
@@ -8,6 +8,7 @@
 package org.opendaylight.openflowplugin.api;
 
 import java.math.BigInteger;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.flow.types.rev131026.FlowCookie;
 
 /**
  * OFP related constants
@@ -46,8 +47,10 @@ public final class OFConstants {
     public static final Long OFPG_ALL = 0xfffffffcL;
     /** Refers to all queues configured at the specified port. */
     public static final Long OFPQ_ALL = ANY;
+    /** default cookie */
     public static final BigInteger DEFAULT_COOKIE = BigInteger.ZERO;
     public static final BigInteger DEFAULT_COOKIE_MASK = BigInteger.ZERO;
+    public static final FlowCookie DEFAULT_FLOW_COOKIE = new FlowCookie(DEFAULT_COOKIE);
     /** indicates that no buffering should be applied and the whole packet is to be
      *  sent to the controller. */
     public static final Long OFP_NO_BUFFER = 0xffffffffL;

diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactory.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactory.java
index 9747d014409ea774bd0fd9f3616431c627bee453..9eec7ea72342e8cafbbd0a6a87c02f0614c60ce0 100644 (file)
--- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactory.java
+++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactory.java
@@ -8,9 +8,12 @@
 
 package org.opendaylight.openflowplugin.impl.registry.flow;
 
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.common.primitives.Longs;
 import java.math.BigInteger;
 import java.util.Objects;
+import org.opendaylight.openflowplugin.api.OFConstants;
 import org.opendaylight.openflowplugin.api.openflow.device.DeviceContext;
 import org.opendaylight.openflowplugin.api.openflow.registry.flow.FlowHash;
 import org.opendaylight.openflowplugin.impl.util.HashUtil;
@@ -47,9 +50,9 @@ public class FlowHashFactory {
         public FlowHashDto(final long flowHash, final Flow flow) {
             this.flowHash = flowHash;
             this.intHashCode = Longs.hashCode(flowHash);
-            tableId = flow.getTableId();
-            priority = flow.getPriority();
-            cookie = flow.getCookie().getValue();
+            tableId = Preconditions.checkNotNull(flow.getTableId(), "flow tableId must not be null");
+            priority = Preconditions.checkNotNull(flow.getPriority(), "flow priority must not be null");
+            cookie = MoreObjects.firstNonNull(flow.getCookie(), OFConstants.DEFAULT_FLOW_COOKIE).getValue();
         }
 
 

diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactoryTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactoryTest.java
index 13210aa328cc3af35d9ccf3b22e5142a31b3f340..6cfdb753124a9b99033d304cf71e824c1a61c31b 100644 (file)
--- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactoryTest.java
+++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/registry/flow/FlowHashFactoryTest.java
@@ -15,9 +15,8 @@ import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
-import junit.framework.Assert;
+import org.junit.Assert;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
@@ -100,7 +99,6 @@ public class FlowHashFactoryTest {
     }
 
     @Test
-    @Ignore
     public void testGetHash2() throws Exception {
         MatchBuilder match1Builder = new MatchBuilder().setLayer3Match(new Ipv4MatchBuilder()
                 .setIpv4Destination(new Ipv4Prefix("10.0.1.157/32")).build());
@@ -126,6 +124,43 @@ public class FlowHashFactoryTest {
         Assert.assertNotSame(flow1Hash, flow2Hash);
     }
 
+    @Test
+    public void testGetHashNPE() throws Exception {
+        MatchBuilder match1Builder = new MatchBuilder().setLayer3Match(new Ipv4MatchBuilder()
+                .setIpv4Destination(new Ipv4Prefix("10.0.1.157/32")).build());
+        FlowBuilder flow1Builder = new FlowBuilder()
+                .setCookie(new FlowCookie(BigInteger.valueOf(483)))
+                .setMatch(match1Builder.build())
+                .setPriority(2)
+                .setTableId((short) 0);
+
+        FlowBuilder fb1 = new FlowBuilder(flow1Builder.build());
+        fb1.setTableId(null);
+        try {
+            FlowHashFactory.create(fb1.build(), deviceContext);
+            Assert.fail("hash creation should have failed because of NPE");
+        } catch (Exception e) {
+            // expected
+            Assert.assertEquals("flow tableId must not be null", e.getMessage());
+        }
+
+        FlowBuilder fb2 = new FlowBuilder(flow1Builder.build());
+        fb2.setPriority(null);
+        try {
+            FlowHashFactory.create(fb2.build(), deviceContext);
+            Assert.fail("hash creation should have failed because of NPE");
+        } catch (Exception e) {
+            // expected
+            Assert.assertEquals("flow priority must not be null", e.getMessage());
+        }
+
+        FlowBuilder fb3 = new FlowBuilder(flow1Builder.build());
+        fb3.setCookie(null);
+        FlowHash flowHash = FlowHashFactory.create(fb3.build(), deviceContext);
+        Assert.assertNotNull(flowHash.getCookie());
+        Assert.assertEquals(OFConstants.DEFAULT_COOKIE, flowHash.getCookie());
+    }
+
     @Test
     public void testGetHash() throws Exception {
         FlowsStatisticsUpdate flowStats = FLOWS_STATISTICS_UPDATE_BUILDER.build();