Instruction related fixes
- Fixed PushHeaderInstructions bug, where half-baked Ethernet instace was used only to hold ethernetType. (ONOS-987) Change-Id: I330a862c8a18206250befbd4e22ee6d189beed83
Showing
8 changed files
with
32 additions
and
46 deletions
| ... | @@ -194,7 +194,7 @@ public final class Instructions { | ... | @@ -194,7 +194,7 @@ public final class Instructions { |
| 194 | */ | 194 | */ |
| 195 | public static Instruction pushMpls() { | 195 | public static Instruction pushMpls() { |
| 196 | return new PushHeaderInstructions(L2SubType.MPLS_PUSH, | 196 | return new PushHeaderInstructions(L2SubType.MPLS_PUSH, |
| 197 | - new Ethernet().setEtherType(Ethernet.MPLS_UNICAST)); | 197 | + Ethernet.MPLS_UNICAST); |
| 198 | } | 198 | } |
| 199 | 199 | ||
| 200 | /** | 200 | /** |
| ... | @@ -203,7 +203,7 @@ public final class Instructions { | ... | @@ -203,7 +203,7 @@ public final class Instructions { |
| 203 | */ | 203 | */ |
| 204 | public static Instruction popMpls() { | 204 | public static Instruction popMpls() { |
| 205 | return new PushHeaderInstructions(L2SubType.MPLS_POP, | 205 | return new PushHeaderInstructions(L2SubType.MPLS_POP, |
| 206 | - new Ethernet().setEtherType(Ethernet.MPLS_UNICAST)); | 206 | + Ethernet.MPLS_UNICAST); |
| 207 | } | 207 | } |
| 208 | 208 | ||
| 209 | /** | 209 | /** |
| ... | @@ -214,8 +214,7 @@ public final class Instructions { | ... | @@ -214,8 +214,7 @@ public final class Instructions { |
| 214 | */ | 214 | */ |
| 215 | public static Instruction popMpls(Short etherType) { | 215 | public static Instruction popMpls(Short etherType) { |
| 216 | checkNotNull(etherType, "Ethernet type cannot be null"); | 216 | checkNotNull(etherType, "Ethernet type cannot be null"); |
| 217 | - return new PushHeaderInstructions(L2SubType.MPLS_POP, | 217 | + return new PushHeaderInstructions(L2SubType.MPLS_POP, etherType); |
| 218 | - new Ethernet().setEtherType(etherType)); | ||
| 219 | } | 218 | } |
| 220 | 219 | ||
| 221 | /* | 220 | /* | ... | ... |
| ... | @@ -19,7 +19,6 @@ import static com.google.common.base.MoreObjects.toStringHelper; | ... | @@ -19,7 +19,6 @@ import static com.google.common.base.MoreObjects.toStringHelper; |
| 19 | 19 | ||
| 20 | import java.util.Objects; | 20 | import java.util.Objects; |
| 21 | 21 | ||
| 22 | -import org.onlab.packet.Ethernet; | ||
| 23 | import org.onlab.packet.MacAddress; | 22 | import org.onlab.packet.MacAddress; |
| 24 | import org.onlab.packet.VlanId; | 23 | import org.onlab.packet.VlanId; |
| 25 | 24 | ||
| ... | @@ -140,15 +139,15 @@ public abstract class L2ModificationInstruction implements Instruction { | ... | @@ -140,15 +139,15 @@ public abstract class L2ModificationInstruction implements Instruction { |
| 140 | L2ModificationInstruction { | 139 | L2ModificationInstruction { |
| 141 | 140 | ||
| 142 | private final L2SubType subtype; | 141 | private final L2SubType subtype; |
| 143 | - private final Ethernet ethernetType; | 142 | + private final short ethernetType; // uint16_t |
| 144 | 143 | ||
| 145 | - public PushHeaderInstructions(L2SubType subType, Ethernet ethernetType) { | 144 | + PushHeaderInstructions(L2SubType subType, short ethernetType) { |
| 146 | this.subtype = subType; | 145 | this.subtype = subType; |
| 147 | this.ethernetType = ethernetType; | 146 | this.ethernetType = ethernetType; |
| 148 | } | 147 | } |
| 149 | 148 | ||
| 150 | - public Ethernet ethernetType() { | 149 | + public int ethernetType() { |
| 151 | - return ethernetType; | 150 | + return Short.toUnsignedInt(ethernetType); |
| 152 | } | 151 | } |
| 153 | 152 | ||
| 154 | @Override | 153 | @Override |
| ... | @@ -158,12 +157,14 @@ public abstract class L2ModificationInstruction implements Instruction { | ... | @@ -158,12 +157,14 @@ public abstract class L2ModificationInstruction implements Instruction { |
| 158 | 157 | ||
| 159 | @Override | 158 | @Override |
| 160 | public String toString() { | 159 | public String toString() { |
| 161 | - return toStringHelper(subtype().toString()).toString(); | 160 | + return toStringHelper(subtype().toString()) |
| 161 | + .add("ethernetType", String.format("0x%04x", ethernetType())) | ||
| 162 | + .toString(); | ||
| 162 | } | 163 | } |
| 163 | 164 | ||
| 164 | @Override | 165 | @Override |
| 165 | public int hashCode() { | 166 | public int hashCode() { |
| 166 | - return Objects.hash(type(), subtype); | 167 | + return Objects.hash(type(), subtype, ethernetType); |
| 167 | } | 168 | } |
| 168 | 169 | ||
| 169 | @Override | 170 | @Override |
| ... | @@ -173,9 +174,8 @@ public abstract class L2ModificationInstruction implements Instruction { | ... | @@ -173,9 +174,8 @@ public abstract class L2ModificationInstruction implements Instruction { |
| 173 | } | 174 | } |
| 174 | if (obj instanceof PushHeaderInstructions) { | 175 | if (obj instanceof PushHeaderInstructions) { |
| 175 | PushHeaderInstructions that = (PushHeaderInstructions) obj; | 176 | PushHeaderInstructions that = (PushHeaderInstructions) obj; |
| 176 | - return Objects.equals(this.type(), that.type()) && | 177 | + return Objects.equals(subtype, that.subtype) && |
| 177 | - Objects.equals(subtype, that.subtype); | 178 | + Objects.equals(this.ethernetType, that.ethernetType); |
| 178 | - | ||
| 179 | } | 179 | } |
| 180 | return false; | 180 | return false; |
| 181 | } | 181 | } | ... | ... |
| ... | @@ -20,6 +20,7 @@ import java.util.List; | ... | @@ -20,6 +20,7 @@ import java.util.List; |
| 20 | import org.junit.Test; | 20 | import org.junit.Test; |
| 21 | import org.onosproject.net.PortNumber; | 21 | import org.onosproject.net.PortNumber; |
| 22 | import org.onosproject.net.flow.instructions.Instruction; | 22 | import org.onosproject.net.flow.instructions.Instruction; |
| 23 | +import org.onosproject.net.flow.instructions.Instructions; | ||
| 23 | import org.onlab.packet.IpAddress; | 24 | import org.onlab.packet.IpAddress; |
| 24 | import org.onlab.packet.MacAddress; | 25 | import org.onlab.packet.MacAddress; |
| 25 | import org.onlab.packet.VlanId; | 26 | import org.onlab.packet.VlanId; |
| ... | @@ -30,8 +31,6 @@ import static org.hamcrest.CoreMatchers.equalTo; | ... | @@ -30,8 +31,6 @@ import static org.hamcrest.CoreMatchers.equalTo; |
| 30 | import static org.hamcrest.MatcherAssert.assertThat; | 31 | import static org.hamcrest.MatcherAssert.assertThat; |
| 31 | import static org.hamcrest.Matchers.hasSize; | 32 | import static org.hamcrest.Matchers.hasSize; |
| 32 | import static org.hamcrest.Matchers.is; | 33 | import static org.hamcrest.Matchers.is; |
| 33 | -import static org.onosproject.net.flow.instructions.L0ModificationInstruction.L0SubType; | ||
| 34 | -import static org.onosproject.net.flow.instructions.L0ModificationInstruction.ModLambdaInstruction; | ||
| 35 | 34 | ||
| 36 | /** | 35 | /** |
| 37 | * Unit tests for the DefaultTrafficTreatment class. | 36 | * Unit tests for the DefaultTrafficTreatment class. |
| ... | @@ -48,7 +47,7 @@ public class DefaultTrafficTreatmentTest { | ... | @@ -48,7 +47,7 @@ public class DefaultTrafficTreatmentTest { |
| 48 | public void testTreatmentBuilderConstructors() { | 47 | public void testTreatmentBuilderConstructors() { |
| 49 | final TrafficTreatment treatment1 = | 48 | final TrafficTreatment treatment1 = |
| 50 | DefaultTrafficTreatment.builder() | 49 | DefaultTrafficTreatment.builder() |
| 51 | - .add(new ModLambdaInstruction(L0SubType.LAMBDA, (short) 4)) | 50 | + .add(Instructions.modL0Lambda((short) 4)) |
| 52 | .build(); | 51 | .build(); |
| 53 | final TrafficTreatment treatment2 = | 52 | final TrafficTreatment treatment2 = |
| 54 | DefaultTrafficTreatment.builder(treatment1).build(); | 53 | DefaultTrafficTreatment.builder(treatment1).build(); |
| ... | @@ -61,7 +60,7 @@ public class DefaultTrafficTreatmentTest { | ... | @@ -61,7 +60,7 @@ public class DefaultTrafficTreatmentTest { |
| 61 | @Test | 60 | @Test |
| 62 | public void testBuilderMethods() { | 61 | public void testBuilderMethods() { |
| 63 | final Instruction instruction1 = | 62 | final Instruction instruction1 = |
| 64 | - new ModLambdaInstruction(L0SubType.LAMBDA, (short) 4); | 63 | + Instructions.modL0Lambda((short) 4); |
| 65 | 64 | ||
| 66 | final TrafficTreatment.Builder builder1 = | 65 | final TrafficTreatment.Builder builder1 = |
| 67 | DefaultTrafficTreatment.builder() | 66 | DefaultTrafficTreatment.builder() |
| ... | @@ -95,15 +94,15 @@ public class DefaultTrafficTreatmentTest { | ... | @@ -95,15 +94,15 @@ public class DefaultTrafficTreatmentTest { |
| 95 | public void testEquals() { | 94 | public void testEquals() { |
| 96 | final TrafficTreatment treatment1 = | 95 | final TrafficTreatment treatment1 = |
| 97 | DefaultTrafficTreatment.builder() | 96 | DefaultTrafficTreatment.builder() |
| 98 | - .add(new ModLambdaInstruction(L0SubType.LAMBDA, (short) 4)) | 97 | + .add(Instructions.modL0Lambda((short) 4)) |
| 99 | .build(); | 98 | .build(); |
| 100 | final TrafficTreatment sameAsTreatment1 = | 99 | final TrafficTreatment sameAsTreatment1 = |
| 101 | DefaultTrafficTreatment.builder() | 100 | DefaultTrafficTreatment.builder() |
| 102 | - .add(new ModLambdaInstruction(L0SubType.LAMBDA, (short) 4)) | 101 | + .add(Instructions.modL0Lambda((short) 4)) |
| 103 | .build(); | 102 | .build(); |
| 104 | final TrafficTreatment treatment2 = | 103 | final TrafficTreatment treatment2 = |
| 105 | DefaultTrafficTreatment.builder() | 104 | DefaultTrafficTreatment.builder() |
| 106 | - .add(new ModLambdaInstruction(L0SubType.LAMBDA, (short) 2)) | 105 | + .add(Instructions.modL0Lambda((short) 2)) |
| 107 | .build(); | 106 | .build(); |
| 108 | new EqualsTester() | 107 | new EqualsTester() |
| 109 | .addEqualityGroup(treatment1, sameAsTreatment1) | 108 | .addEqualityGroup(treatment1, sameAsTreatment1) | ... | ... |
providers/openflow/flow/src/main/java/org/onosproject/provider/of/flow/impl/FlowModBuilderVer13.java
| ... | @@ -238,12 +238,12 @@ public class FlowModBuilderVer13 extends FlowModBuilder { | ... | @@ -238,12 +238,12 @@ public class FlowModBuilderVer13 extends FlowModBuilder { |
| 238 | PushHeaderInstructions pushHeaderInstructions = | 238 | PushHeaderInstructions pushHeaderInstructions = |
| 239 | (PushHeaderInstructions) l2m; | 239 | (PushHeaderInstructions) l2m; |
| 240 | return factory().actions().pushMpls(EthType.of(pushHeaderInstructions | 240 | return factory().actions().pushMpls(EthType.of(pushHeaderInstructions |
| 241 | - .ethernetType().getEtherType())); | 241 | + .ethernetType())); |
| 242 | case MPLS_POP: | 242 | case MPLS_POP: |
| 243 | PushHeaderInstructions popHeaderInstructions = | 243 | PushHeaderInstructions popHeaderInstructions = |
| 244 | (PushHeaderInstructions) l2m; | 244 | (PushHeaderInstructions) l2m; |
| 245 | return factory().actions().popMpls(EthType.of(popHeaderInstructions | 245 | return factory().actions().popMpls(EthType.of(popHeaderInstructions |
| 246 | - .ethernetType().getEtherType())); | 246 | + .ethernetType())); |
| 247 | case MPLS_LABEL: | 247 | case MPLS_LABEL: |
| 248 | ModMplsLabelInstruction mplsLabel = | 248 | ModMplsLabelInstruction mplsLabel = |
| 249 | (ModMplsLabelInstruction) l2m; | 249 | (ModMplsLabelInstruction) l2m; | ... | ... |
| ... | @@ -255,12 +255,12 @@ public final class GroupModBuilder { | ... | @@ -255,12 +255,12 @@ public final class GroupModBuilder { |
| 255 | L2ModificationInstruction.PushHeaderInstructions pushHeaderInstructions = | 255 | L2ModificationInstruction.PushHeaderInstructions pushHeaderInstructions = |
| 256 | (L2ModificationInstruction.PushHeaderInstructions) l2m; | 256 | (L2ModificationInstruction.PushHeaderInstructions) l2m; |
| 257 | return factory.actions().pushMpls(EthType.of(pushHeaderInstructions | 257 | return factory.actions().pushMpls(EthType.of(pushHeaderInstructions |
| 258 | - .ethernetType().getEtherType())); | 258 | + .ethernetType())); |
| 259 | case MPLS_POP: | 259 | case MPLS_POP: |
| 260 | L2ModificationInstruction.PushHeaderInstructions popHeaderInstructions = | 260 | L2ModificationInstruction.PushHeaderInstructions popHeaderInstructions = |
| 261 | (L2ModificationInstruction.PushHeaderInstructions) l2m; | 261 | (L2ModificationInstruction.PushHeaderInstructions) l2m; |
| 262 | return factory.actions().popMpls(EthType.of(popHeaderInstructions | 262 | return factory.actions().popMpls(EthType.of(popHeaderInstructions |
| 263 | - .ethernetType().getEtherType())); | 263 | + .ethernetType())); |
| 264 | case MPLS_LABEL: | 264 | case MPLS_LABEL: |
| 265 | L2ModificationInstruction.ModMplsLabelInstruction mplsLabel = | 265 | L2ModificationInstruction.ModMplsLabelInstruction mplsLabel = |
| 266 | (L2ModificationInstruction.ModMplsLabelInstruction) l2m; | 266 | (L2ModificationInstruction.ModMplsLabelInstruction) l2m; | ... | ... |
| ... | @@ -15,7 +15,6 @@ | ... | @@ -15,7 +15,6 @@ |
| 15 | */ | 15 | */ |
| 16 | package org.onosproject.codec.impl; | 16 | package org.onosproject.codec.impl; |
| 17 | 17 | ||
| 18 | -import org.onlab.packet.Ethernet; | ||
| 19 | import org.onosproject.codec.CodecContext; | 18 | import org.onosproject.codec.CodecContext; |
| 20 | import org.onosproject.codec.JsonCodec; | 19 | import org.onosproject.codec.JsonCodec; |
| 21 | import org.onosproject.net.flow.instructions.Instruction; | 20 | import org.onosproject.net.flow.instructions.Instruction; |
| ... | @@ -100,11 +99,7 @@ public final class InstructionCodec extends JsonCodec<Instruction> { | ... | @@ -100,11 +99,7 @@ public final class InstructionCodec extends JsonCodec<Instruction> { |
| 100 | final L2ModificationInstruction.PushHeaderInstructions pushHeaderInstructions = | 99 | final L2ModificationInstruction.PushHeaderInstructions pushHeaderInstructions = |
| 101 | (L2ModificationInstruction.PushHeaderInstructions) instruction; | 100 | (L2ModificationInstruction.PushHeaderInstructions) instruction; |
| 102 | 101 | ||
| 103 | - final JsonCodec<Ethernet> ethernetCodec = | 102 | + result.put("ethernetType", pushHeaderInstructions.ethernetType()); |
| 104 | - context.codec(Ethernet.class); | ||
| 105 | - result.set("ethernetType", | ||
| 106 | - ethernetCodec.encode(pushHeaderInstructions.ethernetType(), | ||
| 107 | - context)); | ||
| 108 | break; | 103 | break; |
| 109 | 104 | ||
| 110 | default: | 105 | default: | ... | ... |
| ... | @@ -16,13 +16,11 @@ | ... | @@ -16,13 +16,11 @@ |
| 16 | package org.onosproject.codec.impl; | 16 | package org.onosproject.codec.impl; |
| 17 | 17 | ||
| 18 | import org.hamcrest.Description; | 18 | import org.hamcrest.Description; |
| 19 | -import org.hamcrest.Matcher; | ||
| 20 | import org.hamcrest.TypeSafeDiagnosingMatcher; | 19 | import org.hamcrest.TypeSafeDiagnosingMatcher; |
| 21 | import org.onosproject.net.flow.instructions.Instruction; | 20 | import org.onosproject.net.flow.instructions.Instruction; |
| 22 | 21 | ||
| 23 | import com.fasterxml.jackson.databind.JsonNode; | 22 | import com.fasterxml.jackson.databind.JsonNode; |
| 24 | 23 | ||
| 25 | -import static org.onosproject.codec.impl.EthernetJsonMatcher.matchesEthernet; | ||
| 26 | import static org.onosproject.net.flow.instructions.Instructions.*; | 24 | import static org.onosproject.net.flow.instructions.Instructions.*; |
| 27 | import static org.onosproject.net.flow.instructions.L0ModificationInstruction.*; | 25 | import static org.onosproject.net.flow.instructions.L0ModificationInstruction.*; |
| 28 | import static org.onosproject.net.flow.instructions.L2ModificationInstruction.*; | 26 | import static org.onosproject.net.flow.instructions.L2ModificationInstruction.*; |
| ... | @@ -67,13 +65,11 @@ public final class InstructionJsonMatcher extends TypeSafeDiagnosingMatcher<Json | ... | @@ -67,13 +65,11 @@ public final class InstructionJsonMatcher extends TypeSafeDiagnosingMatcher<Json |
| 67 | return false; | 65 | return false; |
| 68 | } | 66 | } |
| 69 | 67 | ||
| 70 | - final Matcher ethernetMatcher = | 68 | + if (instructionToMatch.ethernetType() != ethJson.asInt()) { |
| 71 | - matchesEthernet(instructionToMatch.ethernetType()); | 69 | + description.appendText("ethernetType was " + ethJson); |
| 72 | - final boolean ethernetMatches = ethernetMatcher.matches(ethJson); | ||
| 73 | - if (!ethernetMatches) { | ||
| 74 | - ethernetMatcher.describeMismatch(ethernetMatcher, description); | ||
| 75 | return false; | 70 | return false; |
| 76 | } | 71 | } |
| 72 | + | ||
| 77 | return true; | 73 | return true; |
| 78 | } | 74 | } |
| 79 | 75 | ... | ... |
| ... | @@ -45,8 +45,7 @@ import org.onosproject.net.flow.TrafficSelector; | ... | @@ -45,8 +45,7 @@ import org.onosproject.net.flow.TrafficSelector; |
| 45 | import org.onosproject.net.flow.TrafficTreatment; | 45 | import org.onosproject.net.flow.TrafficTreatment; |
| 46 | import org.onosproject.net.flow.criteria.Criterion; | 46 | import org.onosproject.net.flow.criteria.Criterion; |
| 47 | import org.onosproject.net.flow.instructions.Instruction; | 47 | import org.onosproject.net.flow.instructions.Instruction; |
| 48 | -import org.onosproject.net.flow.instructions.L0ModificationInstruction; | 48 | +import org.onosproject.net.flow.instructions.Instructions; |
| 49 | - | ||
| 50 | import com.eclipsesource.json.JsonArray; | 49 | import com.eclipsesource.json.JsonArray; |
| 51 | import com.eclipsesource.json.JsonObject; | 50 | import com.eclipsesource.json.JsonObject; |
| 52 | import com.google.common.collect.ImmutableSet; | 51 | import com.google.common.collect.ImmutableSet; |
| ... | @@ -187,6 +186,7 @@ public class FlowsResourceTest extends ResourceTest { | ... | @@ -187,6 +186,7 @@ public class FlowsResourceTest extends ResourceTest { |
| 187 | return false; | 186 | return false; |
| 188 | } | 187 | } |
| 189 | 188 | ||
| 189 | + @Override | ||
| 190 | public Type type() { | 190 | public Type type() { |
| 191 | return Type.DEFAULT; | 191 | return Type.DEFAULT; |
| 192 | } | 192 | } |
| ... | @@ -197,10 +197,8 @@ public class FlowsResourceTest extends ResourceTest { | ... | @@ -197,10 +197,8 @@ public class FlowsResourceTest extends ResourceTest { |
| 197 | */ | 197 | */ |
| 198 | private void setupMockFlows() { | 198 | private void setupMockFlows() { |
| 199 | flow2.treatment = DefaultTrafficTreatment.builder() | 199 | flow2.treatment = DefaultTrafficTreatment.builder() |
| 200 | - .add(new L0ModificationInstruction.ModLambdaInstruction( | 200 | + .add(Instructions.modL0Lambda((short) 4)) |
| 201 | - L0ModificationInstruction.L0SubType.LAMBDA, (short) 4)) | 201 | + .add(Instructions.modL0Lambda((short) 5)) |
| 202 | - .add(new L0ModificationInstruction.ModLambdaInstruction( | ||
| 203 | - L0ModificationInstruction.L0SubType.LAMBDA, (short) 5)) | ||
| 204 | .setEthDst(MacAddress.BROADCAST) | 202 | .setEthDst(MacAddress.BROADCAST) |
| 205 | .build(); | 203 | .build(); |
| 206 | flow2.selector = DefaultTrafficSelector.builder() | 204 | flow2.selector = DefaultTrafficSelector.builder() |
| ... | @@ -208,8 +206,7 @@ public class FlowsResourceTest extends ResourceTest { | ... | @@ -208,8 +206,7 @@ public class FlowsResourceTest extends ResourceTest { |
| 208 | .matchIPProtocol((byte) 9) | 206 | .matchIPProtocol((byte) 9) |
| 209 | .build(); | 207 | .build(); |
| 210 | flow4.treatment = DefaultTrafficTreatment.builder() | 208 | flow4.treatment = DefaultTrafficTreatment.builder() |
| 211 | - .add(new L0ModificationInstruction.ModLambdaInstruction( | 209 | + .add(Instructions.modL0Lambda((short) 6)) |
| 212 | - L0ModificationInstruction.L0SubType.LAMBDA, (short) 6)) | ||
| 213 | .build(); | 210 | .build(); |
| 214 | final Set<FlowEntry> flows1 = new HashSet<>(); | 211 | final Set<FlowEntry> flows1 = new HashSet<>(); |
| 215 | flows1.add(flow1); | 212 | flows1.add(flow1); | ... | ... |
-
Please register or login to post a comment