> I agree its cleaner, but I am still inclined to keep it as is because sizeof
> *ip does not contain the options which should also be included in computing
> the IP checksum. IP_IHL takes the header length and uses that which is what
> the IP Checksum calculations should use. What do you think ?
Well, this function doesn't use options, but let's suppose for a
second that it did. It feels very strange to me that we would be
telling a function (csum()) to operate beyond the end of a structure.
I.E. sizeof *ip == 20 but we're telling csum to operate on 24 bytes of
*ip. That doesn't feel like good memory management to me. If there
were options I would be inclined to calculate the checksum of the
struct ip_header first, and then fold in the checksum of the options
as a second step.
Luckily this function doesn't use options so simply doing the first
step seems sufficient to me.
Does that reasoning sound reasonable?
Also, I noticed a bug in this function which existed before this
patch. I'll send out a fix.
Ethan
>
> thanx!
> mehak
>
>
> On Wed, Aug 1, 2012 at 12:05 PM, Ethan Jackson <et...@nicira.com> wrote:
>>
>> > + ip->ip_csum = csum(ip, IP_IHL(ip->ip_ihl_ver) * 4);
>>
>> I think what you have here is correct, but would be a bit
>> safer/cleaner if we changed it to:
>>
>> ip->ip_csum = csum(ip, sizeof *ip);
>>
>> Ethan
>>
>>
>> > } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
>> > /* XXX */
>> > } else if (flow->dl_type == htons(ETH_TYPE_ARP)) {
>> > --
>> > 1.7.2.5
>> >
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > http://openvswitch.org/mailman/listinfo/dev
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
