Thanks for the reviews, I applied these to master.
On Thu, May 09, 2013 at 12:21:12PM +0000, Rajahalme, Jarno (NSN - FI/Espoo)
wrote:
> Looks good on all three counts,
>
> Jarno
>
> On May 8, 2013, at 23:23 , ext Ben Pfaff wrote:
>
> > The tunnel and non-tunnel paths were pretty much the same anyway, so this
> > commit simplifies by merging them.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > ofproto/ofproto-dpif.c | 80
> > +++++++++++++++++++++---------------------------
> > 1 files changed, 35 insertions(+), 45 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index a42625b..4d37500 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -3921,51 +3921,41 @@ ofproto_receive(const struct dpif_backer *backer,
> > struct ofpbuf *packet,
> > *odp_in_port = flow->in_port;
> > }
> >
> > - if (tnl_port_should_receive(flow)) {
> > - const struct ofport *ofport = tnl_port_receive(flow);
> > - if (!ofport) {
> > - flow->in_port = OFPP_NONE;
> > - goto exit;
> > - }
> > - flow->in_port = ofport->ofp_port;
> > - port = ofport_dpif_cast(ofport);
> > + port = (tnl_port_should_receive(flow)
> > + ? ofport_dpif_cast(tnl_port_receive(flow))
> > + : odp_port_to_ofport(backer, flow->in_port));
> > + flow->in_port = port ? port->up.ofp_port : OFPP_NONE;
> > + if (!port) {
> > + goto exit;
> > + }
> >
> > - /* XXX: Since the tunnel module is not scoped per backer, it's
> > - * theoretically possible that we'll receive an ofport belonging
> > to an
> > - * entirely different datapath. In practice, this can't happen
> > because
> > - * no platforms has two separate datapaths which each support
> > - * tunneling. */
> > - ovs_assert(ofproto_dpif_cast(port->up.ofproto)->backer == backer);
> > - } else {
> > - port = odp_port_to_ofport(backer, flow->in_port);
> > - if (!port) {
> > - flow->in_port = OFPP_NONE;
> > - goto exit;
> > - }
> > + /* XXX: Since the tunnel module is not scoped per backer, for a tunnel
> > port
> > + * it's theoretically possible that we'll receive an ofport belonging
> > to an
> > + * entirely different datapath. In practice, this can't happen
> > because no
> > + * platforms has two separate datapaths which each support tunneling.
> > */
> > + ovs_assert(ofproto_dpif_cast(port->up.ofproto)->backer == backer);
> >
> > - flow->in_port = port->up.ofp_port;
> > - if (vsp_adjust_flow(ofproto_dpif_cast(port->up.ofproto), flow)) {
> > - if (packet) {
> > - /* Make the packet resemble the flow, so that it gets sent
> > to
> > - * an OpenFlow controller properly, so that it looks
> > correct
> > - * for sFlow, and so that flow_extract() will get the
> > correct
> > - * vlan_tci if it is called on 'packet'.
> > - *
> > - * The allocated space inside 'packet' probably also
> > contains
> > - * 'key', that is, both 'packet' and 'key' are probably
> > part of
> > - * a struct dpif_upcall (see the large comment on that
> > - * structure definition), so pushing data on 'packet' is in
> > - * general not a good idea since it could overwrite 'key'
> > or
> > - * free it as a side effect. However, it's OK in this
> > special
> > - * case because we know that 'packet' is inside a Netlink
> > - * attribute: pushing 4 bytes will just overwrite the
> > 4-byte
> > - * "struct nlattr", which is fine since we don't need that
> > - * header anymore. */
> > - eth_push_vlan(packet, flow->vlan_tci);
> > - }
> > - /* We can't reproduce 'key' from 'flow'. */
> > - fitness = fitness == ODP_FIT_PERFECT ? ODP_FIT_TOO_MUCH :
> > fitness;
> > - }
> > + if (vsp_adjust_flow(ofproto_dpif_cast(port->up.ofproto), flow)) {
> > + if (packet) {
> > + /* Make the packet resemble the flow, so that it gets sent to
> > + * an OpenFlow controller properly, so that it looks correct
> > + * for sFlow, and so that flow_extract() will get the correct
> > + * vlan_tci if it is called on 'packet'.
> > + *
> > + * The allocated space inside 'packet' probably also contains
> > + * 'key', that is, both 'packet' and 'key' are probably part of
> > + * a struct dpif_upcall (see the large comment on that
> > + * structure definition), so pushing data on 'packet' is in
> > + * general not a good idea since it could overwrite 'key' or
> > + * free it as a side effect. However, it's OK in this special
> > + * case because we know that 'packet' is inside a Netlink
> > + * attribute: pushing 4 bytes will just overwrite the 4-byte
> > + * "struct nlattr", which is fine since we don't need that
> > + * header anymore. */
> > + eth_push_vlan(packet, flow->vlan_tci);
> > + }
> > + /* We can't reproduce 'key' from 'flow'. */
> > + fitness = fitness == ODP_FIT_PERFECT ? ODP_FIT_TOO_MUCH : fitness;
> > }
> > error = 0;
> >
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
