I've fixed your quoting.
On Thu, May 24, 2012 at 06:38:58PM +0200, ravi.ke...@telekom.com
wrote:
> Ben Pfaff writes:
> > OF1.3 adds the ability to match on the MPLS "BOS" bit. Should we add that
> > here too?
>
> My plan was to first get to 1.1 and later on to 1.2/1.3. I
> think it is good to have a reference code for 1.1 just in case there
> is a request for that. I haven't followed the discussion initiated
> by Zoltan on this, will look into it. Do you think match capability
> for "mpls ttl" should be added as well since it provides similar
> purpose as layer-3?
OK, I'm fine with deferring this. It should only be a small
incremental change.
As for the TTL, I don't think that any OpenFlow version allows for
matching on an IP TTL, so there is no support-by-analogy for matching
on the MPLS TTL. Is there another reason to support matching on the
MPLS TTL?
> > It occurs to me that we could drop the NXAST_SET_MPLS_LABEL and
> > NXAST_SET_MPLS_TC actions because these can be implemented through
> > "register load" actions. OpenFlow 1.2 also dropped the similar
> > OFPAT_ actions, for the same reason, so maybe we shouldn't bother
> > with NXAST_ actions for them as they don't have any additional
> > benefit for OpenFlow 1.0 with Nicira extensions.
>
> Same argument as above. In this case it isn't quite clear to me how
> set_mpls_label and set_mpls_tc be implemented via reg_load. Any
> pointers?
In ovs-ofctl syntax, the "load" actions would look like this:
load:123->NXM_OF_MPLS_TC[]
load:5->NXM_OF_MPLS_LABEL[]
> > I think that you can greatly simplify parse_remaining_mpls() to
> > just this (not tested):
> >
> > while (b->size >= sizeof(struct mpls_hdr)) {
> > struct mpls_hdr *mh = ofpbuf_pull(b, sizeof *mh);
> > if (mh->mpls_lse & htonl(MPLS_STACK_MASK)) {
> > break;
> > }
> > }
>
> I quite understand vlan implementation part, though mpls parsing
> implementation is similar to vlan, the reason I had it was for ovs
> to reject packets if there is nothing following mpls
> header. Initially I checked for ipv4/ipv6 header version in
> parse_mpls and later removed after discussion on mpls and non-IP
> packets. Though 2 bytes isn't enough for sanity check, I just left
> it as it is.
OK, as-is it looks like a mistake, could you do something to indicate
that it is not? For example, if there's a real minimum MPLS payload
size, you could use that. Or otherwise add a comment.
> > I don't think we should consider the MPLS label in
> > flow_hash_symmetric_l4(), because there's no assurance that the MPLS
> > labels would be the same in both directions. (Bruce Davie tells me
> > that in fact that's unlikely.)
>
> I don't have strong opinion either way.
Can we go with Bruce's opinion, then, and drop it? Thanks.
> > In commit_mpls_push_action() and commit_mpls_pop_action(), I'm
> > surprised that we can possibly log errors at this late stage.
> > Shouldn't we catch these errors earlier?
>
> checks are performed in validate_action as well.
OK, so we know at this point that it's correct? Can we drop the
check, then, or switch it to an assertion?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
