Merge branch 'master' of github.com:bigswitch/bigswitchcontroller

src/main/java/net/floodlightcontroller/forwarding/Forwarding.java

@@ -343,13 +343,11 @@ public class Forwarding extends ForwardingBase implements IFloodlightModule {
 
         // set buffer-id, in-port and packet-data based on packet-in
         short poLength = (short)(po.getActionsLength() + OFPacketOut.MINIMUM_LENGTH);
-        po.setBufferId(pi.getBufferId());
+        po.setBufferId(OFPacketOut.BUFFER_ID_NONE);
         po.setInPort(pi.getInPort());
-        if (pi.getBufferId() == OFPacketOut.BUFFER_ID_NONE) {
-            byte[] packetData = pi.getPacketData();
-            poLength += packetData.length;
-            po.setPacketData(packetData);
-        }
+        byte[] packetData = pi.getPacketData();
+        poLength += packetData.length;
+        po.setPacketData(packetData);
         po.setLength(poLength);
         
         try {

src/main/java/net/floodlightcontroller/routing/ForwardingBase.java

@@ -284,7 +284,7 @@ public abstract class ForwardingBase
                 if (sw.getId() == pinSwitch) {
                     // TODO: Instead of doing a packetOut here we could also 
                     // send a flowMod with bufferId set.... 
-                    pushPacket(sw, match, pi, outPort, cntx);
+                    pushPacket(sw, pi, false, outPort, cntx);
                     srcSwitchIncluded = true;
                 }
             } catch (IOException e) {
@@ -312,9 +312,9 @@ public abstract class ForwardingBase
     /**
      * Pushes a packet-out to a switch. If bufferId != BUFFER_ID_NONE we 
      * assume that the packetOut switch is the same as the packetIn switch
-     * and we will use the bufferId 
+     * and we will use the bufferId. In this case the packet can be null
      * Caller needs to make sure that inPort and outPort differs
-     * @param packet    packet data to send
+     * @param packet    packet data to send.
      * @param sw        switch from which packet-out is sent
      * @param bufferId  bufferId
      * @param inPort    input port
@@ -397,12 +397,15 @@ public abstract class ForwardingBase
      * port of the packet-in and the outport are the same, the function will not 
      * push the packet-out.
      * @param sw        switch that generated the packet-in, and from which packet-out is sent
-     * @param match     OFmatch
      * @param pi        packet-in
+     * @param useBufferId  if true, use the bufferId from the packet in and 
+     * do not add the packetIn's payload. If false set bufferId to 
+     * BUFFER_ID_NONE and use the packetIn's payload 
      * @param outport   output port
      * @param cntx      context of the packet
      */
-    protected void pushPacket(IOFSwitch sw, OFMatch match, OFPacketIn pi, 
+    protected void pushPacket(IOFSwitch sw, OFPacketIn pi, 
+                           boolean useBufferId, 
                            short outport, FloodlightContext cntx) {
 
         if (pi == null) {
@@ -416,15 +419,15 @@ public abstract class ForwardingBase
             if (log.isDebugEnabled()) {
                 log.debug("Attempting to do packet-out to the same " + 
                           "interface as packet-in. Dropping packet. " + 
-                          " SrcSwitch={}, match = {}, pi={}", 
-                          new Object[]{sw, match, pi});
+                          " SrcSwitch={}, pi={}", 
+                          new Object[]{sw, pi});
                 return;
             }
         }
 
         if (log.isTraceEnabled()) {
-            log.trace("PacketOut srcSwitch={} match={} pi={}", 
-                      new Object[] {sw, match, pi});
+            log.trace("PacketOut srcSwitch={} pi={}", 
+                      new Object[] {sw, pi});
         }
 
         OFPacketOut po =
@@ -440,26 +443,19 @@ public abstract class ForwardingBase
         short poLength =
                 (short) (po.getActionsLength() + OFPacketOut.MINIMUM_LENGTH);
 
-        // If the switch doens't support buffering set the buffer id to be none
-        // otherwise it'll be the the buffer id of the PacketIn
-        if (sw.getBuffers() == 0) {
-            // We set the PI buffer id here so we don't have to check again below
-            pi.setBufferId(OFPacketOut.BUFFER_ID_NONE);
-            po.setBufferId(OFPacketOut.BUFFER_ID_NONE);
-        } else {
+        if (useBufferId) {
             po.setBufferId(pi.getBufferId());
+        } else {
+            po.setBufferId(OFPacketOut.BUFFER_ID_NONE);
         }
-
-        po.setInPort(pi.getInPort());
-
-        // If the buffer id is none or the switch doesn's support buffering
-        // we send the data with the packet out
-        if (pi.getBufferId() == OFPacketOut.BUFFER_ID_NONE) {
+        
+        if (po.getBufferId() == OFPacketOut.BUFFER_ID_NONE) {
             byte[] packetData = pi.getPacketData();
             poLength += packetData.length;
             po.setPacketData(packetData);
         }
 
+        po.setInPort(pi.getInPort());
         po.setLength(poLength);
 
         try {
@@ -629,7 +625,7 @@ public abstract class ForwardingBase
              .setWildcards(OFMatch.OFPFW_ALL & ~OFMatch.OFPFW_DL_SRC
                      & ~OFMatch.OFPFW_IN_PORT);
         fm.setCookie(cookie)
-          .setHardTimeout((short) hardTimeout)
+          .setHardTimeout(hardTimeout)
           .setIdleTimeout(FLOWMOD_DEFAULT_IDLE_TIMEOUT)
           .setHardTimeout(FLOWMOD_DEFAULT_HARD_TIMEOUT)
           .setBufferId(OFPacketOut.BUFFER_ID_NONE)

src/main/java/org/openflow/protocol/OFPacketOut.java

 /**
 *    Copyright (c) 2008 The Board of Trustees of The Leland Stanford Junior
 *    University
-* 
+*
 *    Licensed under the Apache License, Version 2.0 (the "License"); you may
 *    not use this file except in compliance with the License. You may obtain
 *    a copy of the License at
@@ -47,6 +47,7 @@ public class OFPacketOut extends OFMessage implements OFActionFactoryAware {
         super();
         this.type = OFType.PACKET_OUT;
         this.length = U16.t(MINIMUM_LENGTH);
+        this.bufferId = BUFFER_ID_NONE;
     }
 
     /**
@@ -62,6 +63,10 @@ public class OFPacketOut extends OFMessage implements OFActionFactoryAware {
      * @param bufferId
      */
     public OFPacketOut setBufferId(int bufferId) {
+        if (packetData != null && packetData.length > 0 && bufferId != BUFFER_ID_NONE) {
+            throw new IllegalArgumentException(
+                    "PacketOut should not have both bufferId and packetData set");
+        }
         this.bufferId = bufferId;
         return this;
     }
@@ -79,6 +84,10 @@ public class OFPacketOut extends OFMessage implements OFActionFactoryAware {
      * @param packetData
      */
     public OFPacketOut setPacketData(byte[] packetData) {
+        if (packetData != null && packetData.length > 0 && bufferId != BUFFER_ID_NONE) {
+            throw new IllegalArgumentException(
+                    "PacketOut should not have both bufferId and packetData set");
+        }
         this.packetData = packetData;
         return this;
     }
@@ -167,10 +176,12 @@ public class OFPacketOut extends OFMessage implements OFActionFactoryAware {
         this.actions = this.actionFactory.parseActions(data, getActionsLengthU());
         this.packetData = new byte[getLengthU() - MINIMUM_LENGTH - getActionsLengthU()];
         data.readBytes(this.packetData);
+        validate();
     }
 
     @Override
     public void writeTo(ChannelBuffer data) {
+        validate();
         super.writeTo(data);
         data.writeInt(bufferId);
         data.writeShort(inPort);
@@ -182,6 +193,14 @@ public class OFPacketOut extends OFMessage implements OFActionFactoryAware {
             data.writeBytes(this.packetData);
     }
 
+    /** validate the invariants of this OFMessage hold */
+    public void validate() {
+        if (!((bufferId != BUFFER_ID_NONE) ^ (packetData != null && packetData.length > 0))) {
+            throw new IllegalStateException(
+                    "OFPacketOut must have exactly one of (bufferId, packetData) set (not one, not both)");
+        }
+    }
+
     @Override
     public int hashCode() {
         final int prime = 293;

src/test/java/org/openflow/protocol/OFPacketOutTest.java

+package org.openflow.protocol;
+
+import org.junit.Test;
+
+public class OFPacketOutTest {
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testBothBufferIdAndPayloadSet() {
+        OFPacketOut packetOut = new OFPacketOut();
+        packetOut.setBufferId(12);
+        packetOut.setPacketData(new byte[] { 1, 2, 3 });
+    }
+
+    @Test
+    public void testOnlyBufferIdSet() {
+        OFPacketOut packetOut = new OFPacketOut();
+        packetOut.setBufferId(12);
+        packetOut.setPacketData(null);
+        packetOut.setPacketData(new byte[] {});
+        packetOut.validate();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testNeitherBufferIdNorPayloadSet() {
+        OFPacketOut packetOut = new OFPacketOut();
+        packetOut.setBufferId(OFPacketOut.BUFFER_ID_NONE);
+        packetOut.setPacketData(null);
+        packetOut.validate();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testNeitherBufferIdNorPayloadSet2() {
+        OFPacketOut packetOut = new OFPacketOut();
+        packetOut.setBufferId(OFPacketOut.BUFFER_ID_NONE);
+        packetOut.setPacketData(new byte[] {});
+        packetOut.validate();
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testNeitherBufferIdNorPayloadSet3() {
+        OFPacketOut packetOut = new OFPacketOut();
+        packetOut.validate();
+    }
+
+}