On Thu, Jan 21, 2016 at 1:29 PM, Jesse Gross <je...@kernel.org> wrote:
> On Sun, Jan 10, 2016 at 11:18 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> diff --git a/lib/packets.c b/lib/packets.c
>> index d82341d..a910f50 100644
>> --- a/lib/packets.c
>> +++ b/lib/packets.c
>> +void
>> +IP_ECN_set_ce(struct dp_packet *pkt, bool is_ipv6)
>> +{
>> +    if (is_ipv6) {
>> +        struct ovs_16aligned_ip6_hdr *ip6 = dp_packet_l3(pkt);
>> +
>> +        put_16aligned_be32((ovs_16aligned_be32 *)ip6, htonl(IP_ECN_CE << 
>> 20));
>
> Isn't this going to blow away the rest of the things that are already
> in that word?
>
right, I will fix it.

>> +    } else {
>> +        struct ip_header *nh = dp_packet_l4(pkt);
>
> This should be the L3 header, right?
>
yes.

>> +        uint8_t tos = nh->ip_tos;
>> +
>> +        tos |= IP_ECN_CE;
>> +        if (nh->ip_tos != tos) {
>> +            uint8_t *field = &nh->ip_tos;
>> +
>> +            nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) 
>> *field),
>> +                                        htons((uint16_t) tos));
>> +            *field = tos;
>
> Can we just pass the original nh->ip_tos in instead of using the field
> pointer? It seems simpler.
>
ok.

> What about checking to see if the packet is originally ECT or even IP?
> It doesn't inherently have to be done in this function but skipping
> ahead to the STT patch I don't see that we check when it is called. (I
> see the check for IPv6 but it seems to me that it will interpret all
> non-IP traffic as IPv4.)
>
I do not think stt pop can receive any other protocol packet than IPv6
or IPv4. That is how the packet filter is set in tnl-port.c.

 _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to