Committed by
Gerrit Code Review
[CORD-197] Properly handles a flow with empty instruction
Change-Id: Ia465fdc8df1dca7a46249cd4cd8d41faf8461c3a
Showing
7 changed files
with
77 additions
and
15 deletions
| ... | @@ -234,6 +234,7 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { | ... | @@ -234,6 +234,7 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { |
| 234 | 234 | ||
| 235 | switch (instruction.type()) { | 235 | switch (instruction.type()) { |
| 236 | case DROP: | 236 | case DROP: |
| 237 | + case NOACTION: | ||
| 237 | case OUTPUT: | 238 | case OUTPUT: |
| 238 | case GROUP: | 239 | case GROUP: |
| 239 | case L0MODIFICATION: | 240 | case L0MODIFICATION: |
| ... | @@ -259,9 +260,23 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { | ... | @@ -259,9 +260,23 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { |
| 259 | return this; | 260 | return this; |
| 260 | } | 261 | } |
| 261 | 262 | ||
| 263 | + /** | ||
| 264 | + * Add a NOACTION when DROP instruction is explicitly specified. | ||
| 265 | + * | ||
| 266 | + * @return the traffic treatment builder | ||
| 267 | + */ | ||
| 262 | @Override | 268 | @Override |
| 263 | public Builder drop() { | 269 | public Builder drop() { |
| 264 | - return add(Instructions.createDrop()); | 270 | + return add(Instructions.createNoAction()); |
| 271 | + } | ||
| 272 | + | ||
| 273 | + /** | ||
| 274 | + * Add a NOACTION when no instruction is specified. | ||
| 275 | + * | ||
| 276 | + * @return the traffic treatment builder | ||
| 277 | + */ | ||
| 278 | + private Builder noAction() { | ||
| 279 | + return add(Instructions.createNoAction()); | ||
| 265 | } | 280 | } |
| 266 | 281 | ||
| 267 | @Override | 282 | @Override |
| ... | @@ -459,14 +474,10 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { | ... | @@ -459,14 +474,10 @@ public final class DefaultTrafficTreatment implements TrafficTreatment { |
| 459 | 474 | ||
| 460 | @Override | 475 | @Override |
| 461 | public TrafficTreatment build() { | 476 | public TrafficTreatment build() { |
| 462 | - //Don't add DROP instruction by default when instruction | 477 | + if (deferred.size() == 0 && immediate.size() == 0 |
| 463 | - //set is empty. This will be handled in DefaultSingleTablePipeline | 478 | + && table == null && !clear) { |
| 464 | - //driver. | 479 | + noAction(); |
| 465 | - | 480 | + } |
| 466 | - //if (deferred.size() == 0 && immediate.size() == 0 | ||
| 467 | - // && table == null && !clear) { | ||
| 468 | - // drop(); | ||
| 469 | - //} | ||
| 470 | return new DefaultTrafficTreatment(deferred, immediate, table, clear, meta, meter); | 481 | return new DefaultTrafficTreatment(deferred, immediate, table, clear, meta, meter); |
| 471 | } | 482 | } |
| 472 | 483 | ... | ... |
| ... | @@ -27,9 +27,18 @@ public interface Instruction { | ... | @@ -27,9 +27,18 @@ public interface Instruction { |
| 27 | /** | 27 | /** |
| 28 | * Signifies that the traffic should be dropped. | 28 | * Signifies that the traffic should be dropped. |
| 29 | */ | 29 | */ |
| 30 | + @Deprecated | ||
| 30 | DROP, | 31 | DROP, |
| 31 | 32 | ||
| 32 | /** | 33 | /** |
| 34 | + * Signifies that the traffic requires no action. | ||
| 35 | + * | ||
| 36 | + * In OF10, the behavior of NOACTION is DROP. | ||
| 37 | + * In OF13, the behavior depends on current Action Set. | ||
| 38 | + */ | ||
| 39 | + NOACTION, | ||
| 40 | + | ||
| 41 | + /** | ||
| 33 | * Signifies that the traffic should be output to a port. | 42 | * Signifies that the traffic should be output to a port. |
| 34 | */ | 43 | */ |
| 35 | OUTPUT, | 44 | OUTPUT, | ... | ... |
| ... | @@ -68,11 +68,21 @@ public final class Instructions { | ... | @@ -68,11 +68,21 @@ public final class Instructions { |
| 68 | * | 68 | * |
| 69 | * @return drop instruction | 69 | * @return drop instruction |
| 70 | */ | 70 | */ |
| 71 | + @Deprecated | ||
| 71 | public static DropInstruction createDrop() { | 72 | public static DropInstruction createDrop() { |
| 72 | return new DropInstruction(); | 73 | return new DropInstruction(); |
| 73 | } | 74 | } |
| 74 | 75 | ||
| 75 | /** | 76 | /** |
| 77 | + * Creates a no action instruction. | ||
| 78 | + * | ||
| 79 | + * @return no action instruction | ||
| 80 | + */ | ||
| 81 | + public static NoActionInstruction createNoAction() { | ||
| 82 | + return new NoActionInstruction(); | ||
| 83 | + } | ||
| 84 | + | ||
| 85 | + /** | ||
| 76 | * Creates a group instruction. | 86 | * Creates a group instruction. |
| 77 | * | 87 | * |
| 78 | * @param groupId Group Id | 88 | * @param groupId Group Id |
| ... | @@ -450,6 +460,7 @@ public final class Instructions { | ... | @@ -450,6 +460,7 @@ public final class Instructions { |
| 450 | /** | 460 | /** |
| 451 | * Drop instruction. | 461 | * Drop instruction. |
| 452 | */ | 462 | */ |
| 463 | + @Deprecated | ||
| 453 | public static final class DropInstruction implements Instruction { | 464 | public static final class DropInstruction implements Instruction { |
| 454 | 465 | ||
| 455 | private DropInstruction() {} | 466 | private DropInstruction() {} |
| ... | @@ -482,6 +493,40 @@ public final class Instructions { | ... | @@ -482,6 +493,40 @@ public final class Instructions { |
| 482 | } | 493 | } |
| 483 | 494 | ||
| 484 | /** | 495 | /** |
| 496 | + * No Action instruction. | ||
| 497 | + */ | ||
| 498 | + public static final class NoActionInstruction implements Instruction { | ||
| 499 | + | ||
| 500 | + private NoActionInstruction() {} | ||
| 501 | + | ||
| 502 | + @Override | ||
| 503 | + public Type type() { | ||
| 504 | + return Type.NOACTION; | ||
| 505 | + } | ||
| 506 | + | ||
| 507 | + @Override | ||
| 508 | + public String toString() { | ||
| 509 | + return toStringHelper(type().toString()).toString(); | ||
| 510 | + } | ||
| 511 | + | ||
| 512 | + @Override | ||
| 513 | + public int hashCode() { | ||
| 514 | + return Objects.hash(type().ordinal()); | ||
| 515 | + } | ||
| 516 | + | ||
| 517 | + @Override | ||
| 518 | + public boolean equals(Object obj) { | ||
| 519 | + if (this == obj) { | ||
| 520 | + return true; | ||
| 521 | + } | ||
| 522 | + if (obj instanceof NoActionInstruction) { | ||
| 523 | + return true; | ||
| 524 | + } | ||
| 525 | + return false; | ||
| 526 | + } | ||
| 527 | + } | ||
| 528 | + | ||
| 529 | + /** | ||
| 485 | * Output Instruction. | 530 | * Output Instruction. |
| 486 | */ | 531 | */ |
| 487 | public static final class OutputInstruction implements Instruction { | 532 | public static final class OutputInstruction implements Instruction { | ... | ... |
| ... | @@ -339,6 +339,7 @@ public final class KryoNamespaces { | ... | @@ -339,6 +339,7 @@ public final class KryoNamespaces { |
| 339 | Criterion.Type.class, | 339 | Criterion.Type.class, |
| 340 | DefaultTrafficTreatment.class, | 340 | DefaultTrafficTreatment.class, |
| 341 | Instructions.DropInstruction.class, | 341 | Instructions.DropInstruction.class, |
| 342 | + Instructions.NoActionInstruction.class, | ||
| 342 | Instructions.OutputInstruction.class, | 343 | Instructions.OutputInstruction.class, |
| 343 | Instructions.GroupInstruction.class, | 344 | Instructions.GroupInstruction.class, |
| 344 | Instructions.TableTypeTransition.class, | 345 | Instructions.TableTypeTransition.class, | ... | ... |
| ... | @@ -221,11 +221,6 @@ public class FlowEntryBuilder { | ... | @@ -221,11 +221,6 @@ public class FlowEntryBuilder { |
| 221 | 221 | ||
| 222 | private TrafficTreatment buildTreatment() { | 222 | private TrafficTreatment buildTreatment() { |
| 223 | TrafficTreatment.Builder builder = DefaultTrafficTreatment.builder(); | 223 | TrafficTreatment.Builder builder = DefaultTrafficTreatment.builder(); |
| 224 | - // If this is a drop rule | ||
| 225 | - if (instructions.size() == 0) { | ||
| 226 | - builder.drop(); | ||
| 227 | - return builder.build(); | ||
| 228 | - } | ||
| 229 | for (OFInstruction in : instructions) { | 224 | for (OFInstruction in : instructions) { |
| 230 | switch (in.getType()) { | 225 | switch (in.getType()) { |
| 231 | case GOTO_TABLE: | 226 | case GOTO_TABLE: | ... | ... |
providers/openflow/flow/src/main/java/org/onosproject/provider/of/flow/impl/FlowModBuilderVer10.java
| ... | @@ -142,7 +142,7 @@ public class FlowModBuilderVer10 extends FlowModBuilder { | ... | @@ -142,7 +142,7 @@ public class FlowModBuilderVer10 extends FlowModBuilder { |
| 142 | for (Instruction i : treatment.immediate()) { | 142 | for (Instruction i : treatment.immediate()) { |
| 143 | switch (i.type()) { | 143 | switch (i.type()) { |
| 144 | case DROP: | 144 | case DROP: |
| 145 | - log.warn("Saw drop action; assigning drop action"); | 145 | + case NOACTION: |
| 146 | return Collections.emptyList(); | 146 | return Collections.emptyList(); |
| 147 | case L2MODIFICATION: | 147 | case L2MODIFICATION: |
| 148 | act = buildL2Modification(i); | 148 | act = buildL2Modification(i); | ... | ... |
providers/openflow/flow/src/main/java/org/onosproject/provider/of/flow/impl/FlowModBuilderVer13.java
| ... | @@ -215,6 +215,7 @@ public class FlowModBuilderVer13 extends FlowModBuilder { | ... | @@ -215,6 +215,7 @@ public class FlowModBuilderVer13 extends FlowModBuilder { |
| 215 | for (Instruction i : treatments) { | 215 | for (Instruction i : treatments) { |
| 216 | switch (i.type()) { | 216 | switch (i.type()) { |
| 217 | case DROP: | 217 | case DROP: |
| 218 | + case NOACTION: | ||
| 218 | return Collections.emptyList(); | 219 | return Collections.emptyList(); |
| 219 | case L0MODIFICATION: | 220 | case L0MODIFICATION: |
| 220 | actions.add(buildL0Modification(i)); | 221 | actions.add(buildL0Modification(i)); | ... | ... |
-
Please register or login to post a comment