Sounds good.
You may want to wait a bit to resend it, I sent out a bug fix patch
which should probably go in first.
Ethan
On Wed, Aug 1, 2012 at 1:19 PM, Mehak Mahajan <mmaha...@nicira.com> wrote:
> Sure ... Makes sense.
> I will modify it, and since practically the complete patch is changed, I
> will sent out another patch.
>
> Thanks!
> Mehak
>
>
> On Wed, Aug 1, 2012 at 12:57 PM, Ethan Jackson <et...@nicira.com> wrote:
>>
>> > 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
