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 <[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
