From b7a34b47e39a9fe25617c9e536817858e58b2558 Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Thu, 4 Apr 2013 10:25:14 -0700 Subject: [PATCH] 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 --- .../openflow/internal/FlowConverter.java | 30 ++++++++++++------- .../openflow/internal/PortConverter.java | 6 ++-- .../controller/sal/action/ActionType.java | 2 +- .../controller/sal/match/MatchType.java | 18 +++++------ .../controller/sal/utils/NetUtils.java | 25 ++++++++++++++++ .../controller/sal/action/ActionTest.java | 5 +++- .../troubleshoot/web/Troubleshoot.java | 19 ++++-------- 7 files changed, 67 insertions(+), 38 deletions(-) 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 407535f5ce..7e89cd0a42 100644 --- 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)) { + /* + * 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 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 8de7693961..61d6730042 100644 --- 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.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()) { 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 6ff233b69a..d9dc0c7fee 100644 --- 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( - "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; 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 96ddbf47f4..93e5a5874c 100644 --- 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.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; } 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 703d49a9c3..8628bd4813 100644 --- 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; } + + /* + * 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 eeddc0b9b0..954e07b16a 100644 --- 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(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()); 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 c2d9614879..a4083d1624 100644 --- 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.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(), "*"); } -- 2.36.6