[ovs-dev] [PATCH 06/10] ofproto-dpif: New function ofproto_receive().

[Jesse Gross]

On Wed, Dec 19, 2012 at 10:11 PM, Ethan Jackson <ethan at nicira.com> wrote:
> Before translating a datapath flow key into actions, ofproto-dpif
> must parse it, tweak it, and figure out what ofproto_dpif it
> belongs to.  This patch brings all this logic into one place where
> it will be easier to extend in the future.
>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

It's definitely an improvement to consolidate this logic.

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index d5155cf..186203d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> +ofproto_receive(const struct dpif_backer *backer, struct ofpbuf *packet,
> +                const struct nlattr *key, size_t key_len,
> +                struct flow *flow, struct ofproto_dpif **ofproto,
> +                uint32_t *odp_in_port, ovs_be16 *initial_tci)
> +
[...]
> +    port = odp_port_to_ofport(backer, flow->in_port);
> +    if (port) {
> +        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);
> +            }
> +            fitness = fitness == ODP_FIT_PERFECT ? ODP_FIT_TOO_MUCH : fitness;
>          }
> +    } else {
> +        flow->in_port = OFPP_NONE;
> +    }

I'm not sure that OFPP_NONE is the right value to return here, at
least not without additional checks.  Before both
handle_miss_upcalls() and handle_sflow_upcalls() explicitly checked
for not being able to find an ofport before and would abort.  However,
now I think they will silently continue with a different meaning.  At
the very least, we've lost some logging of this situation in
handle_miss_upcalls().

The behavior in ofproto_unixctl_trace() is the same as before but I'm
not sure that was correct.

> @@ -7193,13 +7186,10 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char *argv[],
>                  goto exit;
>              }
>
> -            fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
> -            flow.in_port = odp_port_to_ofp_port(ofproto, flow.in_port);
> -
> -            /* Convert odp_key to flow. */
> -            error = ofproto_dpif_vsp_adjust(ofproto, fitness, &flow,
> -                                            &initial_tci, NULL);
> -            if (error == ODP_FIT_ERROR) {
> +            fitness = ofproto_receive(ofproto->backer, NULL, odp_key.data,
> +                                      odp_key.size, &flow, NULL, NULL,
> +                                      &initial_tci);
> +            if (fitness == ODP_FIT_ERROR) {
>                  unixctl_command_reply_error(conn, "Invalid flow");
>                  goto exit;
>              }

The one remaining direct user of vsp_adjust_flow() is now in the half
of ofproto trace that handles OpenFlow style flows.  This seems stuck
between two possible interpretations of the input port:
 * If it's an odp port then it could require vlan splinters
translation but it would also require single datapath port lookup.
 * If it's an ofp port then it shouldn't require any translation.

I think #2 is the correct interpretation (especially since that's what
you'll get out of our logs).  There shouldn't be any middle ground now
that we're doing all translation in one place, which is good.