Fix bug in Static Route configuration path 83/883/2
authorAlessandro Boch <aboch@cisco.com>
Thu, 15 Aug 2013 16:36:38 +0000 (09:36 -0700)
committerGerrit Code Review <gerrit@opendaylight.org>
Sat, 17 Aug 2013 04:12:40 +0000 (04:12 +0000)
ISSUE: Multiple equivalent static route configurations with different names not detected
CHANGE: In addStaticRoute() perform proper equality check on existing StaticRoute objects
        (using available StaticRoute compareTo() method) instead of checking on StaticRouteConfig
        objects which can never be positive as same name config is detected at the beginning of method

Change-Id: I51d2e1a641f612f5e79eb11063a868739a275209
Signed-off-by: Alessandro Boch <aboch@cisco.com>
opendaylight/forwarding/staticrouting/src/main/java/org/opendaylight/controller/forwarding/staticrouting/internal/StaticRoutingImplementation.java
opendaylight/forwarding/staticrouting/src/test/java/org/opendaylight/controller/forwarding/staticrouting/StaticRouteTest.java

index ec6da38..eac854c 100644 (file)
@@ -97,6 +97,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         }
     }
 
         }
     }
 
+    @Override
     public ConcurrentMap<String, StaticRouteConfig> getStaticRouteConfigs() {
         return staticRouteConfigs;
     }
     public ConcurrentMap<String, StaticRouteConfig> getStaticRouteConfigs() {
         return staticRouteConfigs;
     }
@@ -258,11 +259,13 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
     }
 
     private void notifyHostUpdate(HostNodeConnector host, boolean added) {
     }
 
     private void notifyHostUpdate(HostNodeConnector host, boolean added) {
-        if (host == null)
+        if (host == null) {
             return;
             return;
+        }
         for (StaticRoute s : staticRoutes.values()) {
         for (StaticRoute s : staticRoutes.values()) {
-            if (s.getType() == StaticRoute.NextHopType.SWITCHPORT)
+            if (s.getType() == StaticRoute.NextHopType.SWITCHPORT) {
                 continue;
                 continue;
+            }
             if (s.getNextHopAddress().equals(host.getNetworkAddress())) {
                 if (added) {
                     s.setHost(host);
             if (s.getNextHopAddress().equals(host.getNetworkAddress())) {
                 if (added) {
                     s.setHost(host);
@@ -285,8 +288,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
     }
 
     public boolean isIPv4AddressValid(String cidr) {
     }
 
     public boolean isIPv4AddressValid(String cidr) {
-        if (cidr == null)
+        if (cidr == null) {
             return false;
             return false;
+        }
 
         String values[] = cidr.split("/");
         Pattern ipv4Pattern = Pattern
 
         String values[] = cidr.split("/");
         Pattern ipv4Pattern = Pattern
@@ -322,6 +326,7 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         return 0;
     }
 
         return 0;
     }
 
+    @Override
     public StaticRoute getBestMatchStaticRoute(InetAddress ipAddress) {
         ByteBuffer bblongestPrefix = null;
         try {
     public StaticRoute getBestMatchStaticRoute(InetAddress ipAddress) {
         ByteBuffer bblongestPrefix = null;
         try {
@@ -349,10 +354,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         return longestPrefixRoute;
     }
 
         return longestPrefixRoute;
     }
 
+    @Override
     public Status addStaticRoute(StaticRouteConfig config) {
     public Status addStaticRoute(StaticRouteConfig config) {
-        Status status;
-
-        status = config.isValid();
+        Status status = config.isValid();
         if (!status.isSuccess()) {
             return status;
         }
         if (!status.isSuccess()) {
             return status;
         }
@@ -361,22 +365,29 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
                                 "A valid Static Route configuration with this name " +
                                                 "already exists. Please use a different name");
         }
                                 "A valid Static Route configuration with this name " +
                                                 "already exists. Please use a different name");
         }
-        for (StaticRouteConfig s : staticRouteConfigs.values()) {
-            if (s.equals(config)) {
+
+        // Update database
+        StaticRoute sRoute = new StaticRoute(config);
+
+        for (Map.Entry<String, StaticRoute> entry : staticRoutes.entrySet()) {
+            if (entry.getValue().compareTo(sRoute) == 0) {
                 return new Status(StatusCode.CONFLICT,
                 return new Status(StatusCode.CONFLICT,
-                                "This conflicts with an existing Static Route " +
-                                        "Configuration. Please check the configuration " +
-                                                "and try again");
+                        "This conflicts with an existing Static Route " +
+                                "Configuration. Please check the configuration " +
+                                        "and try again");
             }
         }
             }
         }
+        staticRoutes.put(config.getName(), sRoute);
 
 
+        // Update config databse
         staticRouteConfigs.put(config.getName(), config);
         staticRouteConfigs.put(config.getName(), config);
-        StaticRoute sRoute = new StaticRoute(config);
-        staticRoutes.put(config.getName(), sRoute);
+
+        // Notify
         checkAndUpdateListeners(sRoute, true);
         return status;
     }
 
         checkAndUpdateListeners(sRoute, true);
         return status;
     }
 
+    @Override
     public Status removeStaticRoute(String name) {
         staticRouteConfigs.remove(name);
         StaticRoute sRoute = staticRoutes.remove(name);
     public Status removeStaticRoute(String name) {
         staticRouteConfigs.remove(name);
         StaticRoute sRoute = staticRoutes.remove(name);
@@ -424,8 +435,9 @@ public class StaticRoutingImplementation implements IfNewHostNotify,
         allocateCaches();
         retrieveCaches();
         this.executor = Executors.newFixedThreadPool(1);
         allocateCaches();
         retrieveCaches();
         this.executor = Executors.newFixedThreadPool(1);
-        if (staticRouteConfigs.isEmpty())
+        if (staticRouteConfigs.isEmpty()) {
             loadConfiguration();
             loadConfiguration();
+        }
 
         /*
          *  Slow probe to identify any gateway that might have silently appeared
 
         /*
          *  Slow probe to identify any gateway that might have silently appeared
index 4a497a0..25b2dac 100644 (file)
@@ -83,9 +83,9 @@ public class StaticRouteTest {
         Assert.assertFalse(staticRoute1.equals(staticRoute3));
         Assert.assertFalse(staticRoute1.equals(staticRoute4));
 
         Assert.assertFalse(staticRoute1.equals(staticRoute3));
         Assert.assertFalse(staticRoute1.equals(staticRoute4));
 
-        Assert.assertTrue(staticRoute1.compareTo(staticRoute2) == 0 ? true : false);
-        Assert.assertFalse(staticRoute1.compareTo(staticRoute3) == 0 ? true : false);
-        Assert.assertTrue(staticRoute1.compareTo(staticRoute4) == 0 ? true : false);
+        Assert.assertTrue(staticRoute1.compareTo(staticRoute2) == 0);
+        Assert.assertFalse(staticRoute1.compareTo(staticRoute3) == 0);
+        Assert.assertTrue(staticRoute1.compareTo(staticRoute4) == 0);
 
         }
 
 
         }