ISSUE: 06/106/1
authorAlessandro Boch <aboch@cisco.com>
Thu, 4 Apr 2013 17:25:14 +0000 (10:25 -0700)
committerAlessandro Boch <aboch@cisco.com>
Thu, 4 Apr 2013 17:25:14 +0000 (10:25 -0700)
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>
opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/FlowConverter.java
opendaylight/protocol_plugins/openflow/src/main/java/org/opendaylight/controller/protocol_plugin/openflow/internal/PortConverter.java
opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/action/ActionType.java
opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/match/MatchType.java
opendaylight/sal/api/src/main/java/org/opendaylight/controller/sal/utils/NetUtils.java
opendaylight/sal/api/src/test/java/org/opendaylight/controller/sal/action/ActionTest.java
opendaylight/web/troubleshoot/src/main/java/org/opendaylight/controller/troubleshoot/web/Troubleshoot.java

index 407535f..7e89cd0 100644 (file)
@@ -172,12 +172,17 @@ public class FlowConverter {
                 }
             }
             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 dscp = (byte)((int)tos << 2);
                 if (!isIPv6) {
-                    ofMatch.setNetworkTypeOfService(tos);
+                    ofMatch.setNetworkTypeOfService(dscp);
                     wildcards &= ~OFMatch.OFPFW_NW_TOS;
                 } else {
-                    ((V6Match) ofMatch).setNetworkTypeOfService(tos, (byte) 0);
+                    ((V6Match) ofMatch).setNetworkTypeOfService(dscp, (byte) 0);
                 }
             }
             if (match.isPresent(MatchType.NW_PROTO)) {
@@ -420,7 +425,7 @@ public class FlowConverter {
      * @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();
@@ -523,8 +528,10 @@ public class FlowConverter {
                                                         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
@@ -584,8 +591,10 @@ public class FlowConverter {
                                 .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
@@ -678,14 +687,12 @@ public class FlowConverter {
                     } 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();
-                        int intPort = (port < 0) ? (port.intValue() & 0x7FFF | 0x8000)
-                                : port;
+                        int intPort = NetUtils.getUnsignedShort(port);
                         salAction = new SetTpDst(intPort);
                     }
                     salActionList.add(salAction);
@@ -696,4 +703,5 @@ public class FlowConverter {
         }
         return flow;
     }
+
 }
\ No newline at end of file
index 8de7693..61d6730 100644 (file)
@@ -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.utils.NetUtils;
 import org.opendaylight.controller.sal.utils.NodeConnectorCreator;
 
 /**
@@ -24,14 +25,15 @@ import org.opendaylight.controller.sal.utils.NodeConnectorCreator;
  *
  */
 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
-        int unsignedOFPort = (ofPort & 0x7FFF) | 0x8000;
+        int unsignedOFPort = NetUtils.getUnsignedShort(ofPort);
 
         if (unsignedOFPort > maxOFPhysicalPort) {
             if (ofPort == OFPort.OFPP_LOCAL.getValue()) {
index 6ff233b..d9dc0c7 100644 (file)
@@ -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(
-            "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;
index 96ddbf4..93e5a58 100644 (file)
@@ -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.utils.NetUtils;
 
 /**
  * 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
-    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), 
@@ -203,19 +204,18 @@ public enum MatchType {
                        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_TOS:
+                       return ((Integer) NetUtils.getUnsignedByte((Byte)value))
+                                       .toString();
                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;
                }
index 703d49a..8628bd4 100644 (file)
@@ -338,4 +338,29 @@ public abstract class NetUtils {
         }
         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);
+       }
 }
index eeddc0b..954e07b 100644 (file)
@@ -163,9 +163,12 @@ public class ActionTest {
         action = new SetNwTos(0xf);
         Assert.assertTrue(action.isValid());
 
-        action = new SetNwTos(0xff);
+        action = new SetNwTos(0x3f);
         Assert.assertTrue(action.isValid());
 
+        action = new SetNwTos(0x40);
+        Assert.assertFalse(action.isValid());
+        
         action = new SetNwTos(0xff1);
         Assert.assertFalse(action.isValid());
 
index c2d9614..a4083d1 100644 (file)
@@ -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.NetUtils;
 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 (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());
-            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(), "*");
         }