On Thu, May 24, 2012 at 11:58:53PM +0200, ravi.ke...@telekom.com wrote:
> Ben Pfaff writes:
> > get_l3_ttl_and_tos()
> > --------------------
> >
> > Why does get_l3_ttl_and_tos() consider an unknown IP version as
> > success, with a default? This seems odd.
>
> To handle non-IP traffic i.e push/pop MPLS over non-IP traffic
> e.g. ARP(practical?), VPLS. I had also mentioned in previous
> discussion that MPLS ttl actions will not work for these type of
> traffic but other actions lke push_mpls,pop_mpls,
> set_mpls_label/set_mpls_tc would be. In this function I keep them to
> default for non-IP traffic.
But that only comes up in one situation: copying the TTL inward or
outward when there is exactly one MPLS label. In other situations
(pushing an MPLS label or popping one) we know the correct inner
Ethertype either because it's in the packet or because it's an
argument to the action. When we know it, we should use it.
> In get_l3_ttl_and_tos(), I'm pretty sure that "ih->tos & 0x07" is
> wrong. By my reading, it extracts the two ECN bits and the
> lowest-order DSCP bit. Is that really what you want?
>
> I will check again. I thought IP DSCP bits are 0 - 5 and 6,7 for
> ECN. Here the goal is to copy lower 3 bits of DSCP/TOS or copy from
> mapping table(based on Service Level Agreement) or configuration
> from the controller. Copying from SLA mapping table can be added
> later, but configuration can override EXP bits now using
> "set_mpls_tc" action.
Please do check again. We have
#define IP_ECN_MASK 0x03
#define IP_DSCP_MASK 0xfc
in packets.h and I'm sure that's correct.
> > In get_l3_ttl_and_tos(), I wonder about "ih6->ip6_vfc >> 4", too.
> > By my reading, this will always equal 6, since we know that this is
> > IPv6. I don't think the "6" in IPv6 means that it always has to
> > have a TOS of 6.
>
> IPV6 tos bits are 8 bits long(starting from bit 21), I just take
> lower 3 bits from there. Same argument as above.
Please check again. We have
#define IP_VER(ip_ihl_ver) ((ip_ihl_ver) >> 4)
and
#define IP6_VER(ip6_vfc) ((ip6_vfc) >> 4)
in packets.h and I'm sure that's correct.
> > In get_l3_ttl_and_tos(), I don't see any check that the payload is
> > long enough to hold the entire IPv4 or IPV6 header.
>
> I don't see the need, but will fix it if needed.
By omitting the check we could incur a segfault. Please check the
length.
> > lib/packets.c
> > -------------
> >
> > I see many uses of __be<N> in this file. Those are kernel types.
> > In userspace, use ovs_be<N>.
>
> I double checked I don't see any __be*
Hmm, I must have been looked elsewhere. Sorry.
> > Why is it still not allowed to push an MPLS header onto non-IP data?
> > I think that it should be.
>
> It does push MPLS onto non-IP data. I have tested it on ARP traffic.
push_mpls() calls push_mpls_lse() in this case. push_mpls_lse()
starts out this:
/* neither ipv4 or ipv6 return; */
which implies that it does nothing to non-IP. Please remove the
comment.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
