On Mon, Dec 02, 2013 at 01:20:20PM -0800, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff <b...@nicira.com>
Reviewed-by: Simon Horman <ho...@verge.net.au>
> ---
> DESIGN | 47
> +++++++++++++++++++++++++++++++++++++++
> include/openflow/openflow-1.1.h | 7 +-----
> lib/ofp-util.c | 41 +++++++++++++++++++++++++++++++++-
> tests/ofp-print.at | 15 +++++++++++++
> tests/ofproto.at | 33 +++++++++++++++++++++++++++
> 5 files changed, 136 insertions(+), 7 deletions(-)
>
> diff --git a/DESIGN b/DESIGN
> index d36b025..4d654d0 100644
> --- a/DESIGN
> +++ b/DESIGN
> @@ -266,6 +266,53 @@ OpenFlow 1.3 makes these changes:
> The table for 1.3 is the same as the one shown above for 1.2.
>
>
> +OFPT_PACKET_IN
> +==============
> +
> +The OpenFlow 1.1 specification for OFPT_PACKET_IN is confusing. The
> +definition in OF1.1 openflow.h is[*]:
> +
> + /* Packet received on port (datapath -> controller). */
> + struct ofp_packet_in {
> + struct ofp_header header;
> + uint32_t buffer_id; /* ID assigned by datapath. */
> + uint32_t in_port; /* Port on which frame was received. */
> + uint32_t in_phy_port; /* Physical Port on which frame was received.
> */
> + uint16_t total_len; /* Full length of frame. */
> + uint8_t reason; /* Reason packet is being sent (one of OFPR_*)
> */
> + uint8_t table_id; /* ID of the table that was looked up */
> + uint8_t data[0]; /* Ethernet frame, halfway through 32-bit word,
> + so the IP header is 32-bit aligned. The
> + amount of data is inferred from the length
> + field in the header. Because of padding,
> + offsetof(struct ofp_packet_in, data) ==
> + sizeof(struct ofp_packet_in) - 2. */
> + };
> + OFP_ASSERT(sizeof(struct ofp_packet_in) == 24);
> +
> +The confusing part is the comment on the data[] member. This comment
> +is a leftover from OF1.0 openflow.h, in which the comment was correct:
> +sizeof(struct ofp_packet_in) is 20 in OF1.0 and offsetof(struct
> +ofp_packet_in, data) is 18. When OF1.1 was written, the structure
> +members were changed but the comment was carelessly not updated, and
> +the comment became wrong: sizeof(struct ofp_packet_in) and
> +offsetof(struct ofp_packet_in, data) are both 24 in OF1.1.
> +
> +That leaves the question of how to implement ofp_packet_in in OF1.1.
> +The OpenFlow reference implementation for OF1.1 does not include any
> +padding, that is, the first byte of the encapsulated frame immediately
> +follows the 'table_id' member without a gap. Open vSwitch therefore
> +implements it the same way for compatibility.
That seems entirely sensible.
> +
> +For an earlier discussion, please see the thread archived at:
> +https://mailman.stanford.edu/pipermail/openflow-discuss/2011-August/002604.html
> +
> +[*] The quoted definition is directly from OF1.1. Definitions used
> + inside OVS omit the 8-byte ofp_header members, so the sizes in
> + this discussion are 8 bytes larger than those declared in OVS
> + header files.
> +
> +
> VLAN Matching
> =============
>
> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
> index 4ee9c5c..004e61b 100644
> --- a/include/openflow/openflow-1.1.h
> +++ b/include/openflow/openflow-1.1.h
> @@ -753,12 +753,7 @@ struct ofp11_packet_in {
> ovs_be16 total_len; /* Full length of frame. */
> uint8_t reason; /* Reason packet is being sent (one of OFPR_*) */
> uint8_t table_id; /* ID of the table that was looked up */
> - /* uint8_t data[0]; Ethernet frame, halfway through 32-bit word,
> - so the IP header is 32-bit aligned. The
> - amount of data is inferred from the length
> - field in the header. Because of padding,
> - offsetof(struct ofp_packet_in, data) ==
> - sizeof(struct ofp_packet_in) - 2. */
> + /* Followed by Ethernet frame. */
> };
> OFP_ASSERT(sizeof(struct ofp11_packet_in) == 16);
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index b4723b8..7fc4c7c 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3196,6 +3196,23 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
> pin->reason = opi->reason;
> pin->buffer_id = ntohl(opi->buffer_id);
> pin->total_len = ntohs(opi->total_len);
> + } else if (raw == OFPRAW_OFPT11_PACKET_IN) {
> + const struct ofp11_packet_in *opi;
> + enum ofperr error;
> +
> + opi = ofpbuf_pull(&b, sizeof *opi);
> +
> + pin->packet = b.data;
> + pin->packet_len = b.size;
> +
> + pin->buffer_id = ntohl(opi->buffer_id);
> + error = ofputil_port_from_ofp11(opi->in_port, &pin->fmd.in_port);
> + if (error) {
> + return error;
> + }
> + pin->total_len = ntohs(opi->total_len);
> + pin->reason = opi->reason;
> + pin->table_id = opi->table_id;
> } else if (raw == OFPRAW_NXT_PACKET_IN) {
> const struct nx_packet_in *npi;
> struct match match;
> @@ -3310,6 +3327,27 @@ ofputil_encode_nx_packet_in(const struct
> ofputil_packet_in *pin)
> }
>
> static struct ofpbuf *
> +ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
> +{
> + struct ofp11_packet_in *opi;
> + struct ofpbuf *packet;
> +
> + packet = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
> + htonl(0), pin->packet_len);
> + opi = ofpbuf_put_zeros(packet, sizeof *opi);
> + opi->buffer_id = htonl(pin->buffer_id);
> + opi->in_port = ofputil_port_to_ofp11(pin->fmd.in_port);
> + opi->in_phy_port = opi->in_port;
> + opi->total_len = htons(pin->total_len);
> + opi->reason = pin->reason;
> + opi->table_id = pin->table_id;
> +
> + ofpbuf_put(packet, pin->packet, pin->packet_len);
> +
> + return packet;
> +}
> +
> +static struct ofpbuf *
> ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
> enum ofputil_protocol protocol)
> {
> @@ -3373,7 +3411,8 @@ ofputil_encode_packet_in(const struct ofputil_packet_in
> *pin,
> break;
>
> case OFPUTIL_P_OF11_STD:
> - NOT_REACHED();
> + packet = ofputil_encode_ofp11_packet_in(pin);
> + break;
>
> case OFPUTIL_P_OF12_OXM:
> case OFPUTIL_P_OF13_OXM:
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> index 82e8c3d..2f09c1b 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -469,6 +469,21 @@
> tcp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:0
> ])
> AT_CLEANUP
>
> +AT_SETUP([OFPT_PACKET_IN - OF1.1])
> +AT_KEYWORDS([ofp-print])
> +AT_CHECK([ovs-ofctl ofp-print "\
> +02 0a 00 54 00 00 00 00 00 00 01 11 00 00 00 03 \
> +00 00 00 03 00 3c 00 00 \
> +50 54 00 00 00 06 50 54 00 00 00 05 08 00 \
> +45 00 00 28 bd 12 00 00 40 06 3c 6a c0 a8 00 01 \
> +c0 a8 00 02 27 2f 00 00 78 50 cc 5b 57 af 42 1e \
> +50 02 02 00 26 e8 00 00 00 00 00 00 00 00 \
> +"], [0], [dnl
> +OFPT_PACKET_IN (OF1.1) (xid=0x0): total_len=60 in_port=3 (via no_match)
> data_len=60 buffer=0x00000111
> +tcp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:06,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=10031,tp_dst=0,tcp_flags=0x002
> tcp_csum:26e8
> +])
> +AT_CLEANUP
> +
> AT_SETUP([OFPT_PACKET_IN - OF1.2])
> AT_KEYWORDS([ofp-print])
> AT_CHECK([ovs-ofctl ofp-print "\
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 27b6b34..be7387d 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1757,6 +1757,39 @@ OFPT_BARRIER_REPLY (OF1.2):
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +dnl This test checks that OFPT_PACKET_OUT accepts both OFPP_NONE (as
> +dnl specified by OpenFlow 1.1) and OFPP_CONTROLLER (used by some
> +dnl controllers despite the spec) as meaning a packet that was generated
> +dnl by the controller.
> +AT_SETUP([ofproto - packet-out from controller (OpenFlow 1.1)])
> +OVS_VSWITCHD_START
> +
> +# Start a monitor listening for packet-ins.
> +AT_CHECK([ovs-ofctl -O OpenFlow11 monitor br0 --detach --no-chdir --pidfile])
> +ovs-appctl -t ovs-ofctl ofctl/send 0209000c0123456700000080
> +ovs-appctl -t ovs-ofctl ofctl/barrier
> +ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
> +AT_CAPTURE_FILE([monitor.log])
> +
> +# Send some packet-outs with OFPP_NONE and OFPP_CONTROLLER (65533) as
> in_port.
> +AT_CHECK([ovs-ofctl -O OpenFlow11 packet-out br0 none controller
> '0001020304050010203040501234'])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 packet-out br0 4294967293 controller
> '0001020304050010203040505678'])
> +
> +# Stop the monitor and check its output.
> +ovs-appctl -t ovs-ofctl ofctl/barrier
> +ovs-appctl -t ovs-ofctl exit
> +
> +AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
> +OFPT_PACKET_IN (OF1.1): total_len=14 in_port=ANY (via action) data_len=14
> (unbuffered)
> +metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
> +OFPT_PACKET_IN (OF1.1): total_len=14 in_port=CONTROLLER (via action)
> data_len=14 (unbuffered)
> +metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x5678
> +OFPT_BARRIER_REPLY (OF1.1):
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> dnl This test checks that metadata is encoded in packet_in structures,
> dnl supported by NXAST.
> AT_SETUP([ofproto - packet-out with metadata (NXM)])
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
