[ovs-dev] [PATCH v2] ofp-util: Treat a packet-out in_port of OFPP_CONTROLLER as OFPP_NONE.
Ben Pfaff
blp at nicira.com
Mon May 7 19:31:28 UTC 2012
Thanks, I pushed this.
On Fri, May 04, 2012 at 02:47:41PM -0700, Ethan Jackson wrote:
> Looks good, thanks.
>
> Ethan
>
> On Tue, May 1, 2012 at 4:07 PM, Ben Pfaff <blp at nicira.com> wrote:
> > Some OpenFlow 1.0 controllers incorrectly use OPFP_CONTROLLER as the
> > in_port in packet-out messages, when OFPP_NONE is their intent. Until now,
> > Open vSwitch has rejected such requests with an error message. This commit
> > makes Open vSwitch instead treat OFPP_CONTROLLER the same as OFPP_NONE for
> > compatibility with those controllers.
> >
> > Suggested-by: Rob Sherwood <rob.sherwood at bigswitch.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > This version of the patch takes a different approach that preserves
> > OFPP_CONTROLLER as the input port all the way down to the ofproto
> > provider, which becomes responsible for implementing it the same
> > way as OFPP_NONE.
> >
> > lib/odp-util.c | 2 +-
> > lib/ofp-util.c | 2 +-
> > lib/ofp-util.h | 7 +++++--
> > ofproto/ofproto-provider.h | 23 +++++++++++++++++++++--
> > tests/ofproto.at | 33 +++++++++++++++++++++++++++++++++
> > 5 files changed, 61 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 8fa3359..ba76cad 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -1170,7 +1170,7 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
> > nl_msg_put_be64(buf, OVS_KEY_ATTR_TUN_ID, flow->tun_id);
> > }
> >
> > - if (flow->in_port != OFPP_NONE) {
> > + if (flow->in_port != OFPP_NONE && flow->in_port != OFPP_CONTROLLER) {
> > nl_msg_put_u32(buf, OVS_KEY_ATTR_IN_PORT,
> > ofp_port_to_odp_port(flow->in_port));
> > }
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index ae9b30d..457aeac 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -2224,7 +2224,7 @@ ofputil_decode_packet_out(struct ofputil_packet_out *po,
> > po->buffer_id = ntohl(opo->buffer_id);
> > po->in_port = ntohs(opo->in_port);
> > if (po->in_port >= OFPP_MAX && po->in_port != OFPP_LOCAL
> > - && po->in_port != OFPP_NONE) {
> > + && po->in_port != OFPP_NONE && po->in_port != OFPP_CONTROLLER) {
> > VLOG_WARN_RL(&bad_ofmsg_rl, "packet-out has bad input port %#"PRIx16,
> > po->in_port);
> > return OFPERR_NXBRC_BAD_IN_PORT;
> > diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> > index fd76eac..3f051a4 100644
> > --- a/lib/ofp-util.h
> > +++ b/lib/ofp-util.h
> > @@ -314,12 +314,15 @@ const char *ofputil_packet_in_reason_to_string(enum ofp_packet_in_reason);
> > bool ofputil_packet_in_reason_from_string(const char *,
> > enum ofp_packet_in_reason *);
> >
> > -/* Abstract packet-out message. */
> > +/* Abstract packet-out message.
> > + *
> > + * ofputil_decode_packet_out() will ensure that 'in_port' is a physical port
> > + * (OFPP_MAX or less) or one of OFPP_LOCAL, OFPP_NONE, or OFPP_CONTROLLER. */
> > struct ofputil_packet_out {
> > const void *packet; /* Packet data, if buffer_id == UINT32_MAX. */
> > size_t packet_len; /* Length of packet data in bytes. */
> > uint32_t buffer_id; /* Buffer id or UINT32_MAX if no buffer. */
> > - uint16_t in_port; /* Packet's input port or OFPP_NONE. */
> > + uint16_t in_port; /* Packet's input port. */
> > union ofp_action *actions; /* Actions. */
> > size_t n_actions; /* Number of elements in 'actions' array. */
> > };
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 7b0e478..654b4a9 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -915,8 +915,27 @@ struct ofproto_class {
> > *
> > * 'flow' reflects the flow information for 'packet'. All of the
> > * information in 'flow' is extracted from 'packet', except for
> > - * flow->in_port, which is taken from the OFPT_PACKET_OUT message.
> > - * flow->tun_id and its register values are zeroed.
> > + * flow->in_port (see below). flow->tun_id and its register values are
> > + * zeroed.
> > + *
> > + * flow->in_port comes from the OpenFlow OFPT_PACKET_OUT message. The
> > + * implementation should reject invalid flow->in_port values by returning
> > + * OFPERR_NXBRC_BAD_IN_PORT. For consistency, the implementation should
> > + * consider valid for flow->in_port any value that could possibly be seen
> > + * in a packet that it passes to connmgr_send_packet_in(). Ideally, even
> > + * an implementation that never generates packet-ins (e.g. due to hardware
> > + * limitations) should still allow flow->in_port values for every possible
> > + * physical port and OFPP_LOCAL. The only virtual ports (those above
> > + * OFPP_MAX) that the caller will ever pass in as flow->in_port, other than
> > + * OFPP_LOCAL, are OFPP_NONE and OFPP_CONTROLLER. The implementation
> > + * should allow both of these, treating each of them as packets generated
> > + * by the controller as opposed to packets originating from some switch
> > + * port.
> > + *
> > + * (Ordinarily the only effect of flow->in_port is on output actions that
> > + * involve the input port, such as actions that output to OFPP_IN_PORT,
> > + * OFPP_FLOOD, or OFPP_ALL. flow->in_port can also affect Nicira extension
> > + * "resubmit" actions.)
> > *
> > * 'packet' is not matched against the OpenFlow flow table, so its
> > * statistics should not be included in OpenFlow flow statistics.
> > diff --git a/tests/ofproto.at b/tests/ofproto.at
> > index 82f2f9c..8926427 100644
> > --- a/tests/ofproto.at
> > +++ b/tests/ofproto.at
> > @@ -529,3 +529,36 @@ check_async 7 OFPR_ACTION OFPPR_ADD
> > ovs-appctl -t ovs-ofctl exit
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> > +
> > +dnl This test checks that OFPT_PACKET_OUT accepts both OFPP_NONE (as
> > +dnl specified by OpenFlow 1.0) 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])
> > +OVS_VSWITCHD_START
> > +
> > +# Start a monitor listening for packet-ins.
> > +AT_CHECK([ovs-ofctl -P openflow10 monitor br0 --detach --no-chdir --pidfile])
> > +ovs-appctl -t ovs-ofctl ofctl/send 0109000c0123456700000080
> > +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 packet-out br0 none controller '0001020304050010203040501234'])
> > +AT_CHECK([ovs-ofctl packet-out br0 65533 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: total_len=14 in_port=NONE (via action) data_len=14 (unbuffered)
> > +priority:0,tunnel:0,in_port:0000,tci(0) mac(00:10:20:30:40:50->00:01:02:03:04:05) type:1234 proto:0 tos:0 ttl:0 ip(0.0.0.0->0.0.0.0)
> > +OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
> > +priority:0,tunnel:0,in_port:0000,tci(0) mac(00:10:20:30:40:50->00:01:02:03:04:05) type:5678 proto:0 tos:0 ttl:0 ip(0.0.0.0->0.0.0.0)
> > +OFPT_BARRIER_REPLY:
> > +])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list