Fix Match intersection logic 87/987/1
authorAlessandro Boch <aboch@cisco.com>
Sat, 24 Aug 2013 02:43:32 +0000 (19:43 -0700)
committerAlessandro Boch <aboch@cisco.com>
Sat, 24 Aug 2013 02:43:58 +0000 (19:43 -0700)
    - Change in Match.java: Empty match represents all packets (universal set) while empty set is the null match.
                        So, in case of no intersection, return a null Match.
                            intersect() and getIntersection() optimized in case of universal and empty sets
    - Changes in MatchTest.java to junit the above changes
    - Change in FlowEntry.java: Match.mergewithFilter() is now returning a new Match instance

Change-Id: Idd3ff80d2fbcc11c3f83757268806596b7b2fc50
Signed-off-by: Alessandro Boch <aboch@cisco.com>
opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntry.java
opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/Match.java
opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/match/MatchTest.java

index e8c5f64..83106a3 100644 (file)
@@ -168,14 +168,13 @@ public class FlowEntry implements Cloneable, Serializable {
     public FlowEntry mergeWith(ContainerFlow containerFlow) {
         Match myMatch = flow.getMatch();
 
-        // Based on this flow direction, rearrange the match
-        Match match = containerFlow.getMatch();
+        Match filter = containerFlow.getMatch();
 
         // Merge
-        myMatch.mergeWithFilter(match);
+        Match merge = myMatch.mergeWithFilter(filter);
 
         // Replace this Flow's match with merged version
-        flow.setMatch(myMatch);
+        flow.setMatch(merge);
 
         return this;
     }
index 7a94d05..30c25af 100644 (file)
@@ -287,12 +287,13 @@ public class Match implements Cloneable, Serializable {
      * The result is the match object representing the intersection of the set
      * of packets described by this match with the set of packets described by
      * the filter match. If the intersection of the two sets is empty, the
-     * return match will be empty as well.
+     * return match will be null.
      *
      * @param filter
      *            the match with which attempting the merge
      * @return a new Match object describing the set of packets represented by
-     *         the intersection of this and the filter matches.
+     *         the intersection of this and the filter matches. null if the
+     *         intersection is empty.
      */
     public Match mergeWithFilter(Match filter) {
         return this.getIntersection(filter);
@@ -301,55 +302,70 @@ public class Match implements Cloneable, Serializable {
     /**
      * Return the match representing the intersection of the set of packets
      * described by this match with the set of packets described by the other
-     * match. Such as m.intersect(m) == m and m.intersect(o) == o where o is an
-     * empty match.
+     * match. Such as m.getIntersection(m) == m, m.getIntersection(u) == m and
+     * m.getIntersection(o) == o where u is an empty match (universal set, all
+     * packets) and o is the null match (empty set).
      *
      * @param other
      *            the match with which computing the intersection
      * @return a new Match object representing the intersection of the set of
      *         packets described by this match with the set of packets described
-     *         by the other match.
+     *         by the other match. null when the intersection is the empty set.
      */
     public Match getIntersection(Match other) {
+        // If no intersection, return the empty set
+        if (!this.intersetcs(other)) {
+            return null;
+        }
+        // Check if any of the two is the universal match
+        if (this.getMatches() == 0) {
+            return other.clone();
+        }
+        if (other.getMatches() == 0) {
+            return this.clone();
+        }
+        // Derive the intersection
         Match intersection = new Match();
-        if (this.intersetcs(other)) {
-            for (MatchType type : MatchType.values()) {
-                if (this.isAny(type) && other.isAny(type)) {
-                    continue;
-                }
-                if (this.isAny(type)) {
-                    intersection.setField(other.getField(type).clone());
-                    continue;
-                } else if (other.isAny(type)) {
-                    intersection.setField(this.getField(type).clone());
-                    continue;
-                }
-                // Either they are equal or it is about IP address
-                switch (type) {
-                // When it is about IP address, take the wider prefix address between the twos
-                case NW_SRC:
-                case NW_DST:
-                    MatchField thisField = this.getField(type);
-                    MatchField otherField = other.getField(type);
-                    InetAddress thisAddress = (InetAddress) thisField.getValue();
-                    InetAddress otherAddress = (InetAddress) otherField.getValue();
-                    InetAddress thisMask = (InetAddress) thisField.getMask();
-                    InetAddress otherMask = (InetAddress) otherField.getMask();
-
-                    int thisMaskLen = (thisMask == null) ? ((thisAddress instanceof Inet4Address) ? 32 : 128)
-                            : NetUtils.getSubnetMaskLength(thisMask);
-                    int otherMaskLen = (otherMask == null) ? ((otherAddress instanceof Inet4Address) ? 32 : 128)
-                            : NetUtils.getSubnetMaskLength(otherMask);
-                    if (otherMaskLen < thisMaskLen) {
-                        intersection.setField(new MatchField(type, NetUtils.getSubnetPrefix(otherAddress, otherMaskLen), otherMask));
-                    } else {
-                        intersection.setField(new MatchField(type, NetUtils.getSubnetPrefix(thisAddress, thisMaskLen), thisMask));
-                    }
-                    break;
-                default:
-                    // this and other match field are equal for this type, pick this match field
-                    intersection.setField(this.getField(type).clone());
+        for (MatchType type : MatchType.values()) {
+            if (this.isAny(type) && other.isAny(type)) {
+                continue;
+            }
+            if (this.isAny(type)) {
+                intersection.setField(other.getField(type).clone());
+                continue;
+            } else if (other.isAny(type)) {
+                intersection.setField(this.getField(type).clone());
+                continue;
+            }
+            // Either they are equal or it is about IP address
+            switch (type) {
+            // When it is about IP address, take the wider prefix address
+            // between the twos
+            case NW_SRC:
+            case NW_DST:
+                MatchField thisField = this.getField(type);
+                MatchField otherField = other.getField(type);
+                InetAddress thisAddress = (InetAddress) thisField.getValue();
+                InetAddress otherAddress = (InetAddress) otherField.getValue();
+                InetAddress thisMask = (InetAddress) thisField.getMask();
+                InetAddress otherMask = (InetAddress) otherField.getMask();
+
+                int thisMaskLen = (thisMask == null) ? ((thisAddress instanceof Inet4Address) ? 32 : 128) : NetUtils
+                        .getSubnetMaskLength(thisMask);
+                int otherMaskLen = (otherMask == null) ? ((otherAddress instanceof Inet4Address) ? 32 : 128) : NetUtils
+                        .getSubnetMaskLength(otherMask);
+                if (otherMaskLen < thisMaskLen) {
+                    intersection.setField(new MatchField(type, NetUtils.getSubnetPrefix(otherAddress, otherMaskLen),
+                            otherMask));
+                } else {
+                    intersection.setField(new MatchField(type, NetUtils.getSubnetPrefix(thisAddress, thisMaskLen),
+                            thisMask));
                 }
+                break;
+            default:
+                // this and other match field are equal for this type, pick this
+                // match field
+                intersection.setField(this.getField(type).clone());
             }
         }
         return intersection;
@@ -372,10 +388,14 @@ public class Match implements Cloneable, Serializable {
      *         is non empty
      */
     public boolean intersetcs(Match other) {
-        if (this.getMatches() == 0 || other.getMatches() == 0) {
-            // No intersection with the empty set
+        // No intersection with the empty set
+        if (other == null) {
             return false;
         }
+        // Always intersection with the universal set
+        if (this.getMatches() == 0 || other.getMatches() == 0) {
+            return true;
+        }
         // Iterate through the MatchType defined in the filter
         for (MatchType type : MatchType.values()) {
             if (this.isAny(type) || other.isAny(type)) {
index 5dcb898..f3c7a95 100644 (file)
@@ -565,8 +565,9 @@ public class MatchTest {
         Assert.assertTrue(((InetAddress)i.getField(MatchType.NW_SRC).getValue()).equals(ip2));
         Assert.assertTrue(((InetAddress)i.getField(MatchType.NW_SRC).getMask()).equals(ipm2));
 
+        // Empty set
         i = m2.getIntersection(m3);
-        Assert.assertTrue(i.getMatches() == 0);
+        Assert.assertNull(i);
 
         Match m4 = new Match();
         m4.setField(MatchType.DL_TYPE, ethType);
@@ -608,9 +609,12 @@ public class MatchTest {
         Assert.assertTrue(m6.intersetcs(m6));
         Assert.assertTrue(m6.getIntersection(m6).equals(m6));
 
-        // Empty match, represents the empty set
-        Match o = new Match();
-        Assert.assertTrue(m6.getIntersection(o).equals(o));
-        Assert.assertTrue(o.getIntersection(m6).equals(o));
+        // Empty match, represents the universal set (all packets)
+        Match u = new Match();
+        Assert.assertTrue(m6.getIntersection(u).equals(m6));
+        Assert.assertTrue(u.getIntersection(m6).equals(m6));
+
+        // No intersection with null match, empty set
+        Assert.assertNull(m6.getIntersection(null));
     }
 }