[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