Fix Match intersection logic

    - 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>

diff --git a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntry.java b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntry.java
index e8c5f648fa69c4c7d3c9f5f4846daf73a8e78341..83106a391cb7bdc00ff0911fa703e7dee1c1c2f4 100644 (file)
--- a/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntry.java
+++ b/opendaylight/forwardingrulesmanager/api/src/main/java/org/opendaylight/controller/forwardingrulesmanager/FlowEntry.java
@@ -168,14 +168,13 @@ public class FlowEntry implements Cloneable, Serializable {
     public FlowEntry mergeWith(ContainerFlow containerFlow) {
         Match myMatch = flow.getMatch();
 
     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
 
         // Merge

-        myMatch.mergeWithFilter(match);
+        Match merge = myMatch.mergeWithFilter(filter);

 
         // Replace this Flow's match with merged version
 
         // Replace this Flow's match with merged version

-        flow.setMatch(myMatch);
+        flow.setMatch(merge);

 
         return this;
     }
 
         return this;
     }

diff --git a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/Match.java b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/Match.java
index 7a94d05bf10990b62a24ebb17d0d16c1f6778ad5..30c25af57f688539c7bf569aa4ecccf0f804af26 100644 (file)
--- a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/Match.java
+++ b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/Match.java
@@ -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
      * 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
      *
      * @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);
      */
     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
     /**
      * 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
      *
      * @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) {
      */
     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();
         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;
             }
         }
         return intersection;


@@ -372,10 +388,14 @@ public class Match implements Cloneable, Serializable {
      *         is non empty
      */
     public boolean intersetcs(Match other) {
      *         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;
         }
             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)) {
         // Iterate through the MatchType defined in the filter
         for (MatchType type : MatchType.values()) {
             if (this.isAny(type) || other.isAny(type)) {

diff --git a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/match/MatchTest.java b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/match/MatchTest.java
index 5dcb898dfbb9db5e4420e0cafa06b8977979d8af..f3c7a95b0ebcdfb44a5de81e8a5509572827d2f6 100644 (file)
--- a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/match/MatchTest.java
+++ b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/match/MatchTest.java
@@ -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));
 
         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);
         i = m2.getIntersection(m3);

-        Assert.assertTrue(i.getMatches() == 0);
+        Assert.assertNull(i);

 
         Match m4 = new Match();
         m4.setField(MatchType.DL_TYPE, ethType);
 
         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));
 
         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));

     }
 }
     }
 }