ISSUE:

Controller does not format TOS match field as specified by OF1.0 spec.
CHANGE:
- In protocol_plugin when converting SAL Match to OFMatch have TOS bits to be msb and viceversa
- Added utilities functions in NetUtils for deriving unsigned values for primitive
  types byte and short when needed for comparisons and bit shifting operations.
  Made use of them throughout the code.

Change-Id: Ia0505921b46627825c27d18229f1e1b626ce3d7d
Signed-off-by: Alessandro Boch <aboch@cisco.com>

diff --git a/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/FlowConverter.java b/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/FlowConverter.java
index 407535f5cea3cbef03dcbb85121a14c4d553175d..7e89cd0a42658a50d5579dd21649266d99ff6ab1 100644 (file)
--- a/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/FlowConverter.java
+++ b/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/FlowConverter.java
@@ -172,12 +172,17 @@ public class FlowConverter {
                 }
             }
             if (match.isPresent(MatchType.NW_TOS)) {
                 }
             }
             if (match.isPresent(MatchType.NW_TOS)) {

+                /*
+                 *  OF 1.0 switch expects the TOS as the 6 msb in the byte.
+                 *  it is actually the DSCP field followed by a zero ECN
+                 */

                 byte tos = (Byte) match.getField(MatchType.NW_TOS).getValue();
                 byte tos = (Byte) match.getField(MatchType.NW_TOS).getValue();

+                byte dscp = (byte)((int)tos << 2);

                 if (!isIPv6) {
                 if (!isIPv6) {

-                    ofMatch.setNetworkTypeOfService(tos);
+                    ofMatch.setNetworkTypeOfService(dscp);

                     wildcards &= ~OFMatch.OFPFW_NW_TOS;
                 } else {
                     wildcards &= ~OFMatch.OFPFW_NW_TOS;
                 } else {

-                    ((V6Match) ofMatch).setNetworkTypeOfService(tos, (byte) 0);
+                    ((V6Match) ofMatch).setNetworkTypeOfService(dscp, (byte) 0);

                 }
             }
             if (match.isPresent(MatchType.NW_PROTO)) {
                 }
             }
             if (match.isPresent(MatchType.NW_PROTO)) {


@@ -420,7 +425,7 @@ public class FlowConverter {
      * @param port
      * @return
      */
      * @param port
      * @return
      */

-    public OFMessage getOFFlowMod(/*ISwitch sw, */short command, OFPort port) {
+    public OFMessage getOFFlowMod(short command, OFPort port) {

         OFMessage fm = (isIPv6) ? new V6FlowMod() : new OFFlowMod();
         if (this.ofMatch == null) {
             getOFMatch();
         OFMessage fm = (isIPv6) ? new V6FlowMod() : new OFFlowMod();
         if (this.ofMatch == null) {
             getOFMatch();


@@ -523,8 +528,10 @@ public class FlowConverter {
                                                         false));
                     }
                     if (ofMatch.getNetworkTypeOfService() != 0) {
                                                         false));
                     }
                     if (ofMatch.getNetworkTypeOfService() != 0) {

-                        salMatch.setField(MatchType.NW_TOS, ofMatch
-                                .getNetworkTypeOfService());
+                       int dscp = NetUtils.getUnsignedByte(
+                                       ofMatch.getNetworkTypeOfService());
+                       byte tos = (byte)(dscp >> 2);
+                        salMatch.setField(MatchType.NW_TOS, tos);

                     }
                     if (ofMatch.getNetworkProtocol() != 0) {
                         salMatch.setField(MatchType.NW_PROTO, ofMatch
                     }
                     if (ofMatch.getNetworkProtocol() != 0) {
                         salMatch.setField(MatchType.NW_PROTO, ofMatch


@@ -584,8 +591,10 @@ public class FlowConverter {
                                 .getNetworkDestinationMask());
                     }
                     if (v6Match.getNetworkTypeOfService() != 0) {
                                 .getNetworkDestinationMask());
                     }
                     if (v6Match.getNetworkTypeOfService() != 0) {

-                        salMatch.setField(MatchType.NW_TOS, v6Match
-                                .getNetworkTypeOfService());
+                       int dscp = NetUtils.getUnsignedByte(
+                                       v6Match.getNetworkTypeOfService());
+                       byte tos = (byte) (dscp >> 2);
+                        salMatch.setField(MatchType.NW_TOS, tos);

                     }
                     if (v6Match.getNetworkProtocol() != 0) {
                         salMatch.setField(MatchType.NW_PROTO, v6Match
                     }
                     if (v6Match.getNetworkProtocol() != 0) {
                         salMatch.setField(MatchType.NW_PROTO, v6Match


@@ -678,14 +687,12 @@ public class FlowConverter {
                     } else if (ofAction instanceof OFActionTransportLayerSource) {
                         Short port = ((OFActionTransportLayerSource) ofAction)
                                 .getTransportPort();
                     } else if (ofAction instanceof OFActionTransportLayerSource) {
                         Short port = ((OFActionTransportLayerSource) ofAction)
                                 .getTransportPort();

-                        int intPort = (port < 0) ? (port.intValue() & 0x7FFF | 0x8000)
-                                : port;
+                        int intPort = NetUtils.getUnsignedShort(port);

                         salAction = new SetTpSrc(intPort);
                     } else if (ofAction instanceof OFActionTransportLayerDestination) {
                         Short port = ((OFActionTransportLayerDestination) ofAction)
                                 .getTransportPort();
                         salAction = new SetTpSrc(intPort);
                     } else if (ofAction instanceof OFActionTransportLayerDestination) {
                         Short port = ((OFActionTransportLayerDestination) ofAction)
                                 .getTransportPort();

-                        int intPort = (port < 0) ? (port.intValue() & 0x7FFF | 0x8000)
-                                : port;
+                        int intPort = NetUtils.getUnsignedShort(port);

                         salAction = new SetTpDst(intPort);
                     }
                     salActionList.add(salAction);
                         salAction = new SetTpDst(intPort);
                     }
                     salActionList.add(salAction);


@@ -696,4 +703,5 @@ public class FlowConverter {
         }
         return flow;
     }
         }
         return flow;
     }

+

 }
\ No newline at end of file
 }
\ No newline at end of file

diff --git a/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/PortConverter.java b/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/PortConverter.java
index 8de7693961ced0f40fd902209e961bd14bd3bfd4..61d6730042e95b8d1e07182330242a8ed2116102 100644 (file)
--- a/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/PortConverter.java
+++ b/opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/PortConverter.java
@@ -14,6 +14,7 @@ import org.openflow.protocol.OFPort;
 import org.opendaylight.controller.sal.core.Node;
 import org.opendaylight.controller.sal.core.NodeConnector;
 import org.opendaylight.controller.sal.core.NodeConnector.NodeConnectorIDType;
 import org.opendaylight.controller.sal.core.Node;
 import org.opendaylight.controller.sal.core.NodeConnector;
 import org.opendaylight.controller.sal.core.NodeConnector.NodeConnectorIDType;

+import org.opendaylight.controller.sal.utils.NetUtils;

 import org.opendaylight.controller.sal.utils.NodeConnectorCreator;
 
 /**
 import org.opendaylight.controller.sal.utils.NodeConnectorCreator;
 
 /**


@@ -24,14 +25,15 @@ import org.opendaylight.controller.sal.utils.NodeConnectorCreator;
  *
  */
 public abstract class PortConverter {
  *
  */
 public abstract class PortConverter {

-    private static final int maxOFPhysicalPort = (OFPort.OFPP_MAX.getValue() & 0x7FFF) | 0x8000;
+    private static final int maxOFPhysicalPort =
+               NetUtils.getUnsignedShort(OFPort.OFPP_MAX.getValue());

 
     /**
      * Converts the Openflow port number to the equivalent NodeConnector.
      */
     public static NodeConnector toNodeConnector(short ofPort, Node node) {
         // Restore original OF unsigned 16 bits value for the comparison
 
     /**
      * Converts the Openflow port number to the equivalent NodeConnector.
      */
     public static NodeConnector toNodeConnector(short ofPort, Node node) {
         // Restore original OF unsigned 16 bits value for the comparison

-        int unsignedOFPort = (ofPort & 0x7FFF) | 0x8000;
+        int unsignedOFPort = NetUtils.getUnsignedShort(ofPort);

 
         if (unsignedOFPort > maxOFPhysicalPort) {
             if (ofPort == OFPort.OFPP_LOCAL.getValue()) {
 
         if (unsignedOFPort > maxOFPhysicalPort) {
             if (ofPort == OFPort.OFPP_LOCAL.getValue()) {

diff --git a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/action/ActionType.java b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/action/ActionType.java
index 6ff233b69a6e1a09966c2f922abd0555020906d5..d9dc0c7fee4714bcfd4470e8d664091e7983cacb 100644 (file)
--- a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/action/ActionType.java
+++ b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/action/ActionType.java
@@ -29,7 +29,7 @@ public enum ActionType {
     PUSH_VLAN("pushVlan", 0, 0xffff), // Push (the max value only takes into account the TCI portion of the 802.1q header)
     SET_DL_TYPE("setDlType", 0, 0xffff), // Set ethertype/length field
     SET_NW_SRC("setNwSrc", 0, 0), SET_NW_DST("setNwDst", 0, 0), SET_NW_TOS(
     PUSH_VLAN("pushVlan", 0, 0xffff), // Push (the max value only takes into account the TCI portion of the 802.1q header)
     SET_DL_TYPE("setDlType", 0, 0xffff), // Set ethertype/length field
     SET_NW_SRC("setNwSrc", 0, 0), SET_NW_DST("setNwDst", 0, 0), SET_NW_TOS(

-            "setNwTos", 0, 0xff), SET_TP_SRC("setTpSrc", 1, 0xffff), SET_TP_DST(
+            "setNwTos", 0, 0x3f), SET_TP_SRC("setTpSrc", 1, 0xffff), SET_TP_DST(

             "setTpDst", 1, 0xffff), SET_NEXT_HOP("setNextHop", 0, 0);
 
     private String id;
             "setTpDst", 1, 0xffff), SET_NEXT_HOP("setNextHop", 0, 0);
 
     private String id;

diff --git a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/MatchType.java b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/MatchType.java
index 96ddbf47f4e97dd12605198fe71b06cc0bc4de44..93e5a5874c64787a5fa63cac604030e2f6b5ce68 100644 (file)
--- a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/MatchType.java
+++ b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/MatchType.java
@@ -13,6 +13,7 @@ import java.net.InetAddress;
 
 import org.opendaylight.controller.sal.core.NodeConnector;
 import org.opendaylight.controller.sal.utils.HexEncode;
 
 import org.opendaylight.controller.sal.core.NodeConnector;
 import org.opendaylight.controller.sal.utils.HexEncode;

+import org.opendaylight.controller.sal.utils.NetUtils;

 
 /**
  * Represents the binding between the id, the value and mask type and the range values
 
 /**
  * Represents the binding between the id, the value and mask type and the range values


@@ -30,7 +31,7 @@ public enum MatchType {
     DL_OUTER_VLAN("dlOuterVlan", 1 << 5, Short.class, 1, 0xfff), 
     DL_OUTER_VLAN_PR("dlOuterVlanPriority", 1 << 6, Short.class, 0, 0x7), 
     DL_TYPE("dlType", 1 << 7, Short.class, 0, 0xffff), // 2 bytes
     DL_OUTER_VLAN("dlOuterVlan", 1 << 5, Short.class, 1, 0xfff), 
     DL_OUTER_VLAN_PR("dlOuterVlanPriority", 1 << 6, Short.class, 0, 0x7), 
     DL_TYPE("dlType", 1 << 7, Short.class, 0, 0xffff), // 2 bytes

-    NW_TOS("nwTOS", 1 << 8, Byte.class, 0, 0xff), // 1 byte
+    NW_TOS("nwTOS", 1 << 8, Byte.class, 0, 0x3f), // 6 bits (DSCP field)

     NW_PROTO("nwProto", 1 << 9, Byte.class, 0, 0xff), // 1 byte
     NW_SRC("nwSrc", 1 << 10, InetAddress.class, 0, 0), 
     NW_DST("nwDst", 1 << 11, InetAddress.class, 0, 0), 
     NW_PROTO("nwProto", 1 << 9, Byte.class, 0, 0xff), // 1 byte
     NW_SRC("nwSrc", 1 << 10, InetAddress.class, 0, 0), 
     NW_DST("nwDst", 1 << 11, InetAddress.class, 0, 0), 


@@ -203,19 +204,18 @@ public enum MatchType {
                        return HexEncode.bytesToHexStringFormat((byte[])value);
                case DL_TYPE:
                case DL_VLAN:
                        return HexEncode.bytesToHexStringFormat((byte[])value);
                case DL_TYPE:
                case DL_VLAN:

-                       if ((Short)value < 0) {
-                               return ((Integer) (((Short)value).intValue() & 0x7FFF | 0x8000)).toString();
-                       }
-                       break;
+                       return ((Integer) NetUtils.getUnsignedShort((Short)value))
+                                       .toString();

                case NW_SRC:
                case NW_DST:
                        return ((InetAddress)value).getHostAddress();
                case NW_SRC:
                case NW_DST:
                        return ((InetAddress)value).getHostAddress();

+               case NW_TOS:
+                       return ((Integer) NetUtils.getUnsignedByte((Byte)value))
+                                       .toString();

                case TP_SRC:
                case TP_DST:
                case TP_SRC:
                case TP_DST:

-                       if ((Short)value < 0) {
-                               return ((Integer) (((Short)value).intValue() & 0x7FFF | 0x8000)).toString();
-                       }
-                       break;
+                       return ((Integer) NetUtils.getUnsignedShort((Short)value))
+                                       .toString();

                default:
                        break;
                }
                default:
                        break;
                }

diff --git a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/utils/NetUtils.java b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/utils/NetUtils.java
index 703d49a9c330ca7f6167c80d23182c9cfeb1d33c..8628bd4813c77cb1f35f4eb2f50d6bc4def95da6 100644 (file)
--- a/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/utils/NetUtils.java
+++ b/opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/utils/NetUtils.java
@@ -338,4 +338,29 @@ public abstract class NetUtils {
         }
         return true;
     }
         }
         return true;
     }

+    
+    /*
+     * Following utilities are useful when you need to 
+     * compare or bit shift java primitive type variable
+     * which are inerently signed
+     */
+    /**
+     * Returns the unsigned value of the passed byte variable
+     * 
+     * @param b        the byte value
+     * @return the int variable containing the unsigned byte value
+     */
+    public static int getUnsignedByte(byte b) {
+               return (b > 0)? (int)b : ((int)b & 0x7F | 0x80);
+       }
+       
+    /**
+     * Return the unsigned value of the passed short variable
+     * 
+     * @param s the short value
+     * @return the int variable containing the unsigned short value
+     */
+       public static int getUnsignedShort(short s) {
+               return (s > 0)? (int)s : ((int)s & 0x7FFF | 0x8000);
+       }

 }
 }

diff --git a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/action/ActionTest.java b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/action/ActionTest.java
index eeddc0b9b0dc33df0424936535907ef651b13d4f..954e07b16a80b31fb8345c1daffe54c6ed4ef055 100644 (file)
--- a/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/action/ActionTest.java
+++ b/opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/action/ActionTest.java
@@ -163,9 +163,12 @@ public class ActionTest {
         action = new SetNwTos(0xf);
         Assert.assertTrue(action.isValid());
 
         action = new SetNwTos(0xf);
         Assert.assertTrue(action.isValid());
 

-        action = new SetNwTos(0xff);
+        action = new SetNwTos(0x3f);

         Assert.assertTrue(action.isValid());
 
         Assert.assertTrue(action.isValid());
 

+        action = new SetNwTos(0x40);
+        Assert.assertFalse(action.isValid());
+        

         action = new SetNwTos(0xff1);
         Assert.assertFalse(action.isValid());
 
         action = new SetNwTos(0xff1);
         Assert.assertFalse(action.isValid());
 

diff --git a/opendaylight/web/troubleshoot/src/main/java/org/opendaylight/controller/troubleshoot/web/Troubleshoot.java b/opendaylight/web/troubleshoot/src/main/java/org/opendaylight/controller/troubleshoot/web/Troubleshoot.java
index c2d96148793efe68a4b2b3a5c3131786668cc4df..a4083d162454938e63ffd53632fe81eb2fe217c4 100644 (file)
--- a/opendaylight/web/troubleshoot/src/main/java/org/opendaylight/controller/troubleshoot/web/Troubleshoot.java
+++ b/opendaylight/web/troubleshoot/src/main/java/org/opendaylight/controller/troubleshoot/web/Troubleshoot.java
@@ -32,6 +32,7 @@ import org.opendaylight.controller.sal.utils.EtherTypes;
 import org.opendaylight.controller.sal.utils.GlobalConstants;
 import org.opendaylight.controller.sal.utils.HexEncode;
 import org.opendaylight.controller.sal.utils.IPProtocols;
 import org.opendaylight.controller.sal.utils.GlobalConstants;
 import org.opendaylight.controller.sal.utils.HexEncode;
 import org.opendaylight.controller.sal.utils.IPProtocols;

+import org.opendaylight.controller.sal.utils.NetUtils;

 import org.opendaylight.controller.sal.utils.ServiceHelper;
 import org.opendaylight.controller.statisticsmanager.IStatisticsManager;
 import org.opendaylight.controller.switchmanager.ISwitchManager;
 import org.opendaylight.controller.sal.utils.ServiceHelper;
 import org.opendaylight.controller.statisticsmanager.IStatisticsManager;
 import org.opendaylight.controller.switchmanager.ISwitchManager;


@@ -288,26 +289,16 @@ public class Troubleshoot implements IOneWeb {
         if (match.isPresent(MatchType.TP_SRC)) {
             Short tpSrc = (Short) (flow.getMatch().getField(MatchType.TP_SRC)
                     .getValue());
         if (match.isPresent(MatchType.TP_SRC)) {
             Short tpSrc = (Short) (flow.getMatch().getField(MatchType.TP_SRC)
                     .getValue());

-            if (tpSrc < 0) {
-                row.put(MatchType.TP_SRC.id(),
-                        ((Integer) (tpSrc.intValue() & 0x7FFF | 0x8000))
-                                .toString());
-            } else {
-                row.put(MatchType.TP_SRC.id(), tpSrc.toString());
-            }
+            row.put(MatchType.TP_SRC.id(),
+                       String.valueOf(NetUtils.getUnsignedShort(tpSrc)));

         } else {
             row.put(MatchType.TP_SRC.id(), "*");
         }
         if (match.isPresent(MatchType.TP_DST)) {
             Short tpDst = (Short) (flow.getMatch().getField(MatchType.TP_DST)
                     .getValue());
         } else {
             row.put(MatchType.TP_SRC.id(), "*");
         }
         if (match.isPresent(MatchType.TP_DST)) {
             Short tpDst = (Short) (flow.getMatch().getField(MatchType.TP_DST)
                     .getValue());

-            if (tpDst < 0) {
-                row.put(MatchType.TP_DST.id(),
-                        ((Integer) (tpDst.intValue() & 0x7FFF | 0x8000))
-                                .toString());
-            } else {
-                row.put(MatchType.TP_DST.id(), tpDst.toString());
-            }
+            row.put(MatchType.TP_DST.id(),
+                       String.valueOf(NetUtils.getUnsignedShort(tpDst)));

         } else {
             row.put(MatchType.TP_DST.id(), "*");
         }
         } else {
             row.put(MatchType.TP_DST.id(), "*");
         }