On Wed, May 13, 2015 at 07:51:21AM -0700, Gurucharan Shetty wrote:
> When containers are running inside VMs and the openflow flows
> are added in the hypervisor, the physical to logical translation
> (and vice versa) needs to handle the VLAN tags that the packet
> comes with.

Thanks for implementing this!  I apologize that it took me a few days to
review it; I'm behind on all "real work".

> Signed-off-by: Gurucharan Shetty <shet...@nicira.com>

> +        int tag = 0;
> +        ofp_port_t ofport;
> +        if (binding->parent_port) {
> +            ofport = u16_to_ofp(simap_get(&lport_to_ofport,
> +                                          binding->parent_port));
> +            if (ofport) {

I don't think that it is safe to rely on 'tag' being nonnull here.  It
should be, but the database constraints can't ensure it:

> +                tag = *binding->tag;
> +            }
> +        } else {
> +            ofport = u16_to_ofp(simap_get(&lport_to_ofport,
> +                                          binding->logical_port));
> +        }
> +
>          bool local = ofport != 0;
>          if (!local) {
>              ofport = u16_to_ofp(simap_get(&chassis_to_ofport,
> @@ -122,10 +136,20 @@ physical_run(struct controller_ctx *ctx)
>               *
>               * For packets that arrive from a vif: set MFF_LOG_INPORT to the
>               * logical input port, MFF_METADATA to the logical datapath, and
> -             * resubmit into the logical pipeline starting at table 16. */
> +             * resubmit into the logical pipeline starting at table 16.
> +             *
> +             * Containers sitting behind a vif, come with VLAN tags.
> +             * Match on the tags and then strip it before resubmitting to
> +             * the next tables. */
>              match_init_catchall(&match);
>              ofpbuf_clear(&ofpacts);
>              match_set_in_port(&match, ofport);
> +            if (tag) {
> +                match_set_dl_vlan(&match, htons(tag));
> +            } else {
> +                match_set_dl_vlan(&match, htons(OFP10_VLAN_NONE));
> +            }

Above, I would rather avoid matching on the VLAN at all in the case
where there is no tag.  That gives the logical pipeline a chance to use
the VLAN tag in such a case.  (Currently, the logical pipeline just
discards packets that have VLAN tags, but I'd like to leave room for
expansion in the future, especially since there seems to be some ongoing
work on QinQ support.)

Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to