Looks good, thanks. Ethan
On Tue, May 1, 2012 at 4:07 PM, Ben Pfaff <[email protected]> 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 <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
